Bug 134563 - ProcessAssertion should also prevent UIApp suspension
Summary: ProcessAssertion should also prevent UIApp suspension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-02 14:45 PDT by Gavin Barraclough
Modified: 2014-07-02 17:56 PDT (History)
6 users (show)

See Also:


Attachments
Fix (9.26 KB, patch)
2014-07-02 14:49 PDT, Gavin Barraclough
mitz: review-
Details | Formatted Diff | Diff
Fix review comments (9.39 KB, patch)
2014-07-02 17:20 PDT, Gavin Barraclough
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2014-07-02 14:45:32 PDT
If the application suspends then the child processes will, too.
Comment 1 Gavin Barraclough 2014-07-02 14:49:55 PDT
Created attachment 234283 [details]
Fix
Comment 2 WebKit Commit Bot 2014-07-02 14:52:04 PDT
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 3 mitz 2014-07-02 15:05:59 PDT
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.
Comment 4 Gavin Barraclough 2014-07-02 15:37:22 PDT
(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.
Comment 5 Gavin Barraclough 2014-07-02 17:20:19 PDT
Created attachment 234297 [details]
Fix review comments
Comment 6 WebKit Commit Bot 2014-07-02 17:21:56 PDT
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 7 mitz 2014-07-02 17:35:42 PDT
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.
Comment 8 Tim Horton 2014-07-02 17:45:24 PDT
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"
Comment 9 Gavin Barraclough 2014-07-02 17:50:53 PDT
(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!
Comment 10 Gavin Barraclough 2014-07-02 17:56:13 PDT
Transmitting file data ....
Committed revision 170737.