Bug 131453 - Update SPI for managing tabs
Summary: Update SPI for managing tabs
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: 131860
  Show dependency tree / graph
 
Reported: 2014-04-09 14:26 PDT by Gavin Barraclough
Modified: 2014-04-18 15:50 PDT (History)
3 users (show)

See Also:

Attachments
Fix (11.83 KB, patch)
2014-04-09 14:32 PDT, Gavin Barraclough 		ap: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2014-04-09 14:26:25 PDT 
.
Comment 1 Gavin Barraclough 2014-04-09 14:32:42 PDT 
Created attachment 228983 [details]
Fix
Comment 2 Alexey Proskuryakov 2014-04-09 15:01:15 PDT 
Comment on attachment 228983 [details]
Fix

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

Python script changes in the patch seem unrelated.

> Source/WebKit2/UIProcess/WebProcessProxy.h:226
> +    bool m_foreground;

I think that "m_assertionFlagsAreForeground" would be a more descriptive name for this.

Or perhaps we can ask m_assertion about whether it's taken?

> Source/WebKit2/UIProcess/ios/WebProcessProxyIOS.mm:35
> +#define BACKGROUND_TAB_FLAGS (BKSProcessAssertionAllowIdleSleep)
> +#define FOREGROUND_TAB_FLAGS (BKSProcessAssertionAllowIdleSleep | BKSProcessAssertionPreventTaskSuspend | BKSProcessAssertionWantsForegroundResourcePriority | BKSProcessAssertionPreventTaskThrottleDown)

I'd use a const, not a #define.

> Source/WebKit2/UIProcess/ios/WebProcessProxyIOS.mm:100
> +                LOG_ERROR("Unable to aquire assertion for WebContent process %d", pid);

Typo: aquire.

> Source/WebKit2/UIProcess/ios/WebProcessProxyIOS.mm:104
> +        m_assertion = [[BKSProcessAssertion alloc] initWithPID:pid flags:flags reason:BKSProcessAssertionReasonExtension name:@"Web content visible" withHandler:handler];

This leaks, please use adoptNS().
Comment 3 Gavin Barraclough 2014-04-09 15:13:36 PDT 
Transmitting file data ....
Committed revision 167039.
Comment 4 mitz 2014-04-09 21:10:36 PDT 
(In reply to comment #2)
> (From update of attachment 228983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228983&action=review
> 
> Python script changes in the patch seem unrelated.
> 
> > Source/WebKit2/UIProcess/WebProcessProxy.h:226
> > +    bool m_foreground;
> 
> I think that "m_assertionFlagsAreForeground" would be a more descriptive name for this.
> 
> Or perhaps we can ask m_assertion about whether it's taken?
> 
> > Source/WebKit2/UIProcess/ios/WebProcessProxyIOS.mm:35
> > +#define BACKGROUND_TAB_FLAGS (BKSProcessAssertionAllowIdleSleep)
> > +#define FOREGROUND_TAB_FLAGS (BKSProcessAssertionAllowIdleSleep | BKSProcessAssertionPreventTaskSuspend | BKSProcessAssertionWantsForegroundResourcePriority | BKSProcessAssertionPreventTaskThrottleDown)
> 
> I'd use a const, not a #define.

…and constants should be named like local variables, starting with a lowercase letter.