[webkit-reviews] review denied: [Bug 134563] ProcessAssertion should also prevent UIApp suspension : [Attachment 234283] Fix

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 134563: ProcessAssertion should also prevent UIApp suspension
https://bugs.webkit.org/show_bug.cgi?id=134563

Attachment 234283: Fix
https://bugs.webkit.org/attachment.cgi?id=234283&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234283&action=review


Not r+ because of the Objective-C class naming issue and what I think is a
logic error below.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:37
> + at interface ProcessAssertionBackgroundTaskManager : NSObject

All Objective-C class names in WebKit must have either the WK or _WK prefix. In
this case, it should be WK (since it’s internal, not SPI).

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:41
> +    unsigned m_needsToRunInBackgroundCount;
> +    BOOL m_appIsBackground;
> +    UIBackgroundTaskIdentifier m_bgTask;

Objective-C ivar names use the _ prefix rather than m_.

Finally, we normally put the ivar block in the @implementation these days.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:45
> +-(void)incrementNeedsToRunInBackgroundCount;
> +-(void)decrementNeedsToRunInBackgroundCount;

Missing space between - and (void)

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:51
> +-(instancetype)init

Missing space between - and (instancetype)

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:57
> +    m_needsToRunInBackgroundCount = 0;

No need to initialize ivars to 0 in Objective-C.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:68
> +- (void)dealloc

Yay space!

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:80
> +-(void)_updateBackgroundTask

No space :(

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:92
> +    if (shouldHoldTask && m_bgTask != UIBackgroundTaskInvalid) {

Shouldn’t this condition be (!shouldHoldTask && m_bgTask !=
UIBackgroundTaskInvalid)?

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:104
> +-(void)incrementNeedsToRunInBackgroundCount

Missing space.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:110
> +-(void)decrementNeedsToRunInBackgroundCount

Ditto.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:136
> +static ProcessAssertionBackgroundTaskManager* sharedBackgroundTaskManager()

Space goes on the other side of the * because it’s an Objective-C class.

Normally when there’s a shared instance of an Objective-C, we write a class
method to return the shared instance.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:138
> +    static ProcessAssertionBackgroundTaskManager* shared =
[ProcessAssertionBackgroundTaskManager new];

Star on the wrong side. We normally use alloc] init] but I think new] is just
as good.