Bug 206373 - [Cocoa] Broker access to the PowerManagement API
Summary: [Cocoa] Broker access to the PowerManagement API
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-01-16 14:16 PST by Per Arne Vollan
Modified: 2020-02-06 10:03 PST (History)
6 users (show)

See Also:


Attachments
Patch (16.82 KB, patch)
2020-01-16 14:37 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.88 KB, patch)
2020-01-16 14:57 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2020-01-17 11:25 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.01 KB, patch)
2020-01-17 17:06 PST, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2020-01-22 14:38 PST, 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-01-16 14:16:03 PST
Power management calls should be performed in the UI process.
Comment 1 Per Arne Vollan 2020-01-16 14:16:21 PST
rdar://problem/34722450
Comment 2 Per Arne Vollan 2020-01-16 14:37:58 PST
Created attachment 387961 [details]
Patch
Comment 3 Per Arne Vollan 2020-01-16 14:57:36 PST
Created attachment 387964 [details]
Patch
Comment 4 Sam Weinig 2020-01-16 21:07:14 PST
Comment on attachment 387964 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/AVAssetTrackUtilities.h:40
> +WEBCORE_EXPORT void setSystemHasBattery(bool);
> +WEBCORE_EXPORT bool systemHasBattery();

This does not seem like the right file for these. If they need to be exposed outside of the .mm file, please move them to a more appropriate place or create a new file for them.
Comment 5 Per Arne Vollan 2020-01-17 11:25:49 PST
Created attachment 388070 [details]
Patch
Comment 6 Per Arne Vollan 2020-01-17 11:26:43 PST
(In reply to Sam Weinig from comment #4)
> Comment on attachment 387964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387964&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/AVAssetTrackUtilities.h:40
> > +WEBCORE_EXPORT void setSystemHasBattery(bool);
> > +WEBCORE_EXPORT bool systemHasBattery();
> 
> This does not seem like the right file for these. If they need to be exposed
> outside of the .mm file, please move them to a more appropriate place or
> create a new file for them.

Good point! I have moved this to a new file in the new patch.

Thanks for reviewing, Sam!
Comment 7 Per Arne Vollan 2020-01-17 17:06:11 PST
Created attachment 388120 [details]
Patch
Comment 8 Darin Adler 2020-01-20 11:59:56 PST
Comment on attachment 388120 [details]
Patch

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

> Source/WebCore/platform/cocoa/PowerUtils.h:2
> +* Copyright (C) 2020 Apple Inc. All rights reserved.

File naming: A file named "XXXUtils" is against WebKit coding style. Not sure if we wrote it down or not.

I suggest the name SystemBattery.h/mm. But there are probably other good names to consider.

The vague programmer word "utilities" should be avoided, even though this rule has not been followed consistently. Lets not make another header with a name like this. Even just "Power.h" might be OK.

The abbreviation "utils" goes against our stance against unnecessary abbreviation.
Comment 9 Per Arne Vollan 2020-01-21 12:01:38 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 388120 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388120&action=review
> 
> > Source/WebCore/platform/cocoa/PowerUtils.h:2
> > +* Copyright (C) 2020 Apple Inc. All rights reserved.
> 
> File naming: A file named "XXXUtils" is against WebKit coding style. Not
> sure if we wrote it down or not.
> 
> I suggest the name SystemBattery.h/mm. But there are probably other good
> names to consider.
> 
> The vague programmer word "utilities" should be avoided, even though this
> rule has not been followed consistently. Lets not make another header with a
> name like this. Even just "Power.h" might be OK.
> 
> The abbreviation "utils" goes against our stance against unnecessary
> abbreviation.

Thanks for reviewing! I will update the patch according to the comments :) I think I will go with SystemBattery.h/mm as you suggested.
Comment 10 Per Arne Vollan 2020-01-22 14:38:10 PST
Created attachment 388471 [details]
Patch
Comment 11 WebKit Commit Bot 2020-01-23 12:10:53 PST
Comment on attachment 388471 [details]
Patch

Clearing flags on attachment: 388471

Committed r254995: <https://trac.webkit.org/changeset/254995>