Bug 236739

Created attachment 452257 [details]
Patch

<rdar://problem/89056721>

Created attachment 452302 [details]
Patch

Comment on attachment 452302 [details]
Patch

This seems fine. Ping me when the bots are green and I'll give it the nit-pick look, but it'll be r+

Created attachment 452334 [details]
Patch

Created attachment 452335 [details]
Patch

Created attachment 452336 [details]
Patch

Created attachment 452366 [details]
Patch

Created attachment 452486 [details]
Patch

Comment on attachment 452486 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushManager.cpp:134
> +                promise.reject(Exception { NotAllowedError, "User denied push permission"_s });

We may want to provide a clearer explanation here of WHY this is failing.

> 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 }`.

> Source/WebCore/Modules/push-api/PushManager.cpp:170
> +        if (permission == NotificationPermission::Default)

This screams for a switch statement IMO.

Comment on attachment 452486 [details]
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

Comment on attachment 452486 [details]
Patch

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

>>> 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.

Well, they are all green because of an ugly #if !PLATFORM(WIN_CAIRO)

Created attachment 452590 [details]
Patch for landing

Found 1 new test failure: http/wpt/push-api/onpush-disabled.html

Created attachment 452617 [details]
Patch for landing

Committed r290198 (247526@main): <https://commits.webkit.org/247526@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452617 [details].