[webkit-reviews] review granted: [Bug 131453] Update SPI for managing tabs : [Attachment 228983] Fix

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 131453: Update SPI for managing tabs
https://bugs.webkit.org/show_bug.cgi?id=131453

Attachment 228983: Fix
https://bugs.webkit.org/attachment.cgi?id=228983&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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().