Bug 159284

Summary: ProcessAssertion shouldn't keep UI app runnable if BKSProcessAssertion is invalid
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebKit2Assignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 159280    
Attachments:
Description Flags
WIP
none
Fix cdumez: review+

Description Gavin Barraclough 2016-06-29 16:29:02 PDT
First cut of this will only handle the case where the assertion is invalid from the start; Chris is going to add code to listen for the assertion becoming invalid.
Comment 1 Gavin Barraclough 2016-06-29 16:36:41 PDT
Created attachment 282391 [details]
WIP
Comment 2 Chris Dumez 2016-06-29 16:42:13 PDT
Comment on attachment 282391 [details]
WIP

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

> Source/WebKit2/UIProcess/ProcessAssertion.h:61
> +    bool isValid();

Could be const?

> Source/WebKit2/UIProcess/ProcessAssertion.h:85
> +    bool m_isHoldingBackgroundAssertionOnApp : false;

{ false }

Also, do we need the "OnApp" suffix?

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:177
> +    return m_assertion->valid;

m_assertion.get().valid
Comment 3 Chris Dumez 2016-06-29 16:54:58 PDT
Comment on attachment 282391 [details]
WIP

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

>> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:177
>> +    return m_assertion->valid;
> 
> m_assertion.get().valid

Also, for this to build with public SDK,  I think we'll need to add the following to BKSProcessAssertion @interface in ApplicationServicesSPI.h:
@property (nonatomic, readonly) BOOL valid;
Comment 4 Chris Dumez 2016-06-29 18:49:55 PDT
Comment on attachment 282391 [details]
WIP

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

>>> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:177
>>> +    return m_assertion->valid;
>> 
>> m_assertion.get().valid
> 
> Also, for this to build with public SDK,  I think we'll need to add the following to BKSProcessAssertion @interface in ApplicationServicesSPI.h:
> @property (nonatomic, readonly) BOOL valid;

Unfortunately, this causes some problems because valid is false until the assertion is actually acquired.
Comment 5 Chris Dumez 2016-06-29 18:54:56 PDT
Comment on attachment 282391 [details]
WIP

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

>>>> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:177
>>>> +    return m_assertion->valid;
>>> 
>>> m_assertion.get().valid
>> 
>> Also, for this to build with public SDK,  I think we'll need to add the following to BKSProcessAssertion @interface in ApplicationServicesSPI.h:
>> @property (nonatomic, readonly) BOOL valid;
> 
> Unfortunately, this causes some problems because valid is false until the assertion is actually acquired.

In particular, isValid() always returns false when updateRunInBackgroundCount() is called from the ProcessAndUIAssertion() constructor.
Comment 6 Gavin Barraclough 2016-06-30 10:34:25 PDT
Created attachment 282450 [details]
Fix
Comment 7 Chris Dumez 2016-06-30 10:43:22 PDT
Comment on attachment 282450 [details]
Fix

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

r=me

> Source/WebKit2/ChangeLog:3
> +        ProcessAssertion shouldn't keep UI app runnable if BKSProcessAssertion is invalid

Maybe we want to update the title given the smaller scope of the change now?

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:181
> +    if (m_isHoldingBackgroundAssertion && !shouldHoldBackgroundAssertion)

else if ?
Comment 8 Gavin Barraclough 2016-06-30 11:16:19 PDT
Committed revision 202690.