[webkit-reviews] review granted: [Bug 236739] Hook up PushManager to notification permission state : [Attachment 452486] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has granted Ben Nham <nham at apple.com>'s
request for review:
Bug 236739: Hook up PushManager to notification permission state
https://bugs.webkit.org/show_bug.cgi?id=236739

Attachment 452486: Patch

https://bugs.webkit.org/attachment.cgi?id=452486&action=review




--- Comment #11 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 452486
  --> https://bugs.webkit.org/attachment.cgi?id=452486
Patch

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

R+ with review feedback addressed

>> Source/WebCore/Modules/push-api/PushManager.cpp:139
>> +		// Ref { *this } triggers a compiler bug on wincairo where it
tries to ref the lambda instead of PushManager.
> 
> MSVC has trouble with nested lambdas. I think the trick to get this building
in MSVC is to capture `protectedThis = WTFMove(protectedThis)` below instead of
`protectedThis = Ref { *this }`.

I think the bot bubbles are all green, which is good.

>> Source/WebCore/Modules/push-api/PushManager.cpp:170
>> +	    if (permission == NotificationPermission::Default)
> 
> This screams for a switch statement IMO.

Agreed: Switch plz