Bug 210296 - [Cocoa] The function WebCore::systemHasBattery() should cache the result.
Summary: [Cocoa] The function WebCore::systemHasBattery() should cache the result.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-09 12:39 PDT by Per Arne Vollan
Modified: 2020-04-09 14:04 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2020-04-09 12:42 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (2.93 KB, patch)
2020-04-09 12:58 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-04-09 12:39:49 PDT
The function WebCore::systemHasBattery() should cache the result, since the return value of this function will be the same on a specific device.
Comment 1 Per Arne Vollan 2020-04-09 12:40:18 PDT
rdar://problem/61331536
Comment 2 Per Arne Vollan 2020-04-09 12:42:46 PDT
Created attachment 395991 [details]
Patch
Comment 3 Darin Adler 2020-04-09 12:49:47 PDT
Comment on attachment 395991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395991&action=review

> Source/WebCore/platform/cocoa/SystemBattery.mm:45
> +    static bool hasBattery = [] {

Very confusing that we have two different global booleans of different types and scopes, but the same names.

I suggest not defining a second boolean here, and just assigning a value to the first one.

> Source/WebCore/platform/cocoa/SystemBattery.mm:61
> +    return hasBattery;

This would then be:

    return *hasBattery;

Or you could write the function as:

    if (!hasBattery) {
        hasBattery = [] {
            <....>
        }();
    }
    return *hasBattery;
Comment 4 Per Arne Vollan 2020-04-09 12:58:41 PDT
Created attachment 395992 [details]
Patch
Comment 5 Per Arne Vollan 2020-04-09 12:59:22 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 395991 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395991&action=review
> 
> > Source/WebCore/platform/cocoa/SystemBattery.mm:45
> > +    static bool hasBattery = [] {
> 
> Very confusing that we have two different global booleans of different types
> and scopes, but the same names.
> 
> I suggest not defining a second boolean here, and just assigning a value to
> the first one.
> 
> > Source/WebCore/platform/cocoa/SystemBattery.mm:61
> > +    return hasBattery;
> 
> This would then be:
> 
>     return *hasBattery;
> 
> Or you could write the function as:
> 
>     if (!hasBattery) {
>         hasBattery = [] {
>             <....>
>         }();
>     }
>     return *hasBattery;

Very good point, thanks for reviewing!
Comment 6 EWS 2020-04-09 14:04:52 PDT
Committed r259827: <https://trac.webkit.org/changeset/259827>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395992 [details].