Bug 238319 – Implement ServiceWorkerWindowClient.focus

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

<rdar://90616490>

[youenn fablet]

Created attachment 455626 [details]
WIP

[Ben Nham]

Comment on attachment 455626 [details]
WIP

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

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:247
> +

I did this a different way here: https://bugs.webkit.org/show_bug.cgi?id=238364

But maybe this much simpler implementation that you've done here is what we should be doing, as it doesn't require any changes to UserGestureIndicator.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:850
>      if (!m_uiDelegate)

Why not call UIClient::focus?

[youenn fablet]

(In reply to Ben Nham from comment #2)
> Comment on attachment 455626 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455626&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:247
> > +
> 
> I did this a different way here:
> https://bugs.webkit.org/show_bug.cgi?id=238364
> 
> But maybe this much simpler implementation that you've done here is what we
> should be doing, as it doesn't require any changes to UserGestureIndicator.

Yeah, user gesture handling in non document is very loosely defined.
A service worker dedicated approach (based on events) seems ok for now.
We can revisit this later.

> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:850
> >      if (!m_uiDelegate)
> 
> Why not call UIClient::focus?

The WebPageProxy focus term has a strict meaning (used in setFocus and takeFocus)
which has nothing to do with making sure the view becomes key and visible.
This is a bit confusing. 

Hence why the different name that I reused from some automation code.
I am fine using another term though.

[youenn fablet]

Created attachment 455744 [details]
Patch

[youenn fablet]

Created attachment 455747 [details]
Patch

[youenn fablet]

Comment on attachment 455747 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:840
> +    // FIXME: We probably need to call a delegate instead of doing this directly here.

This is the part where I am not exactly sure what we should do.
My guess is that we should let know the application we want to switch to that tab using a particular delegate (maybe with a default implementation if the delegate is not implemented).
This default implementation works for me on MacOS.

[Brady Eidson]

Comment on attachment 455747 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:840
>> +    // FIXME: We probably need to call a delegate instead of doing this directly here.
> 
> This is the part where I am not exactly sure what we should do.
> My guess is that we should let know the application we want to switch to that tab using a particular delegate (maybe with a default implementation if the delegate is not implemented).
> This default implementation works for me on MacOS.

Let's add a new UI delegate method to call if implemented.
But if the client doesn't implement it, do the default thing.

R+ with that.

[youenn fablet]

Created attachment 455842 [details]
Rebasing

[EWS]

Committed r291938 (248909@main): <https://commits.webkit.org/248909@main>

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

[Ben Nham]

*** Bug 238364 has been marked as a duplicate of this bug. ***