Bug 159284 - ProcessAssertion shouldn't keep UI app runnable if BKSProcessAssertion is invalid
Summary: ProcessAssertion shouldn't keep UI app runnable if BKSProcessAssertion is inv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks: 159280
  Show dependency treegraph
 
Reported: 2016-06-29 16:29 PDT by Gavin Barraclough
Modified: 2016-06-30 11:16 PDT (History)
0 users

See Also:


Attachments
WIP (4.75 KB, patch)
2016-06-29 16:36 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix (4.16 KB, patch)
2016-06-30 10:34 PDT, Gavin Barraclough
cdumez: 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 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.