Bug 134563 – ProcessAssertion should also prevent UIApp suspension

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Gavin Barraclough]

If the application suspends then the child processes will, too.

[Gavin Barraclough]

Created attachment 234283 [details]
Fix

[WebKit Commit Bot]

Attachment 234283 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:105:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:111:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 234283 [details]
Fix

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
> +@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.

[Gavin Barraclough]

(In reply to comment #3)
> (From update of attachment 234283 [details])
> 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
> > +@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).

Done!
 
> > 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_.

Done!

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

Done!

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

Fixed!

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

Fixed!

> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:57
> > +    m_needsToRunInBackgroundCount = 0;
> 
> No need to initialize ivars to 0 in Objective-C.

Removed!

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

Yay!

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

Fixed!

> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:92
> > +    if (shouldHoldTask && m_bgTask != UIBackgroundTaskInvalid) {
> 
> Shouldn’t this condition be (!shouldHoldTask && m_bgTask != UIBackgroundTaskInvalid)?

Oops, last minute refactoring. <embarrassed-face-emoji>

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

Fixed!

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

Fixed!

> > 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.

Bleh, okay. :-/

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

Okay!

> > 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.

Okay.

[Gavin Barraclough]

Created attachment 234297 [details]
Fix review comments

[WebKit Commit Bot]

Attachment 234297 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:47:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 234297 [details]
Fix review comments

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

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:33
> +#import <wtf/NeverDestroyed.h>

Seems unused.

[Tim Horton]

Comment on attachment 234297 [details]
Fix review comments

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

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:50
> +    UIBackgroundTaskIdentifier _bgTask;

please expand "bg"

[Gavin Barraclough]

(In reply to comment #7)
> (From update of attachment 234297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234297&action=review
> 
> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:33
> > +#import <wtf/NeverDestroyed.h>
> 
> Seems unused.

I just caught that too!

(In reply to comment #8)
> (From update of attachment 234297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234297&action=review
> 
> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:50
> > +    UIBackgroundTaskIdentifier _bgTask;
> 
> please expand "bg"

Okay!

[Gavin Barraclough]

Transmitting file data ....
Committed revision 170737.