Bug 222223 – Preserve information about the origin of the app highlight request

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Preserve information about the origin of the app highlight request

[Megan Gardner]

Created attachment 421082 [details]
Patch

[Wenson Hsieh]

Comment on attachment 421082 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1437
> +    [delegate _webView:self storeAppHighlight:wkHighlight.get() inNewGroup:highlight.isNewGroup == WebCore::CreateNewGroupForHighlight::Yes ? YES : NO requestOriginatedInApp:highlight.requestOrigin == WebCore::HighlightRequestOriginatedInApp::Yes ? YES : NO];

Nit - `highlight.isNewGroup == WebCore::CreateNewGroupForHighlight::Yes ? YES : NO` can just be `highlight.isNewGroup == WebCore::CreateNewGroupForHighlight::Yes `.

Also, `requestOrigin` should like it would be a `SecurityOrigin` or `String`, rather than a boolean flag. Can we rename this to something like `requestOriginatedInApp` or `originatedInApp` to match the API selector name?

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightDelegate.h:38
> +- (void)_webView:(WKWebView *)webView storeAppHighlight:(_WKAppHighlight *)highlight inNewGroup:(BOOL)inNewGroup requestOriginatedInApp:(BOOL)requestOrigin WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

(Here too, w.r.t. `requestOrigin`)

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:1257
> +void WebChromeClient::storeAppHighlight(WebCore::AppHighlight& highlight)

If we make the getters below `const`, then I think `storeAppHighlight` can go back to being `const`.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1405
> +    WebCore::CreateNewGroupForHighlight highlightIsNewGroup() { return m_highlightIsNewGroup; };
> +    WebCore::HighlightRequestOriginatedInApp highlightRequestOrigin() { return m_highlightRequestOrigin; };

Nit - these should be marked `const`.

[Wenson Hsieh]

> Also, `requestOrigin` should like it would be a `SecurityOrigin` or
> `String`, rather than a boolean flag. Can we rename this to something like
> `requestOriginatedInApp` or `originatedInApp` to match the API selector name?

(Sorry, I meant to write "sounds like", not "should like")

[Megan Gardner]

Created attachment 421229 [details]
Patch

[Wenson Hsieh]

Comment on attachment 421229 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:84
> +    auto requestOriginatedInApp = WebCore::SecurityOrigin::create(request.url());
> +    return requestOriginatedInApp->isSameOriginAs(WebCore::SecurityOrigin::create(response.url()).get());

(It looks like this one is an actual SecurityOrigin)

[Wenson Hsieh]

Comment on attachment 421229 [details]
Patch

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

r=mews

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:350
> +    void storeAppHighlight(WebCore::AppHighlight&) const final;

Nit - IMO, it's a tad strange for this to take a AppHighlight lvalue reference here, since it's not clear from the name (`storeAppHighlight`) that the method will additionally mutate the given highlight data.

I think this might be better as an rvalue reference instead (WebCore::AppHighlight&&), with `WTFMove`-ing to plumb the highlight along through the client layer, since none of the apparent call sites seem to require the passed-in AppHighlight afterwards.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 421229 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421229&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:84
> > +    auto requestOriginatedInApp = WebCore::SecurityOrigin::create(request.url());
> > +    return requestOriginatedInApp->isSameOriginAs(WebCore::SecurityOrigin::create(response.url()).get());
> 
> (It looks like this one is an actual SecurityOrigin)

...to clarify, we should revert this change before landing.

[Megan Gardner]

Created attachment 421238 [details]
Patch

[Chris Dumez]

Comment on attachment 421238 [details]
Patch

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

> Source/WebCore/Modules/highlight/AppHighlightStorage.cpp:217
> +void AppHighlightStorage::storeAppHighlight(StaticRange& range)

StaticRange&& (and WTFMove() at the call site)

> Source/WebCore/Modules/highlight/AppHighlightStorage.h:51
> +    WEBCORE_EXPORT void storeAppHighlight(StaticRange&);

StaticRange&&

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:350
> +    void storeAppHighlight(WebCore::AppHighlight&) const final;

WebCore::AppHighlight&&

> Source/WebKit/WebProcess/WebPage/WebPage.h:1401
> +    WebCore::CreateNewGroupForHighlight m_highlightIsNewGroup { WebCore::CreateNewGroupForHighlight::No };

Let's not put data members with the functions. Also, it should probably be private.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1402
> +    WebCore::HighlightRequestOriginatedInApp m_highlightRequestOriginatedInApp { WebCore::HighlightRequestOriginatedInApp::No };

ditto.

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.h:155
> +    void storeAppHighlight(WebCore::AppHighlight&) const final;

WebCore::AppHighlight&&

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:718
> +void WebChromeClient::storeAppHighlight(WebCore::AppHighlight&) const

WebCore::AppHighlight&&

[Megan Gardner]

Created attachment 421253 [details]
Patch

[Megan Gardner]

Created attachment 421271 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74821306>

[Megan Gardner]

Created attachment 422026 [details]
Patch

[Wenson Hsieh]

Comment on attachment 422026 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightDelegate.h:36
>  - (void)_webView:(WKWebView *)webView storeAppHighlight:(_WKAppHighlight *)highlight inNewGroup:(BOOL)inNewGroup WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Since this never shipped, I wonder if we can just remove this version of the UI delegate hook.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1402
> +    WebCore::CreateNewGroupForHighlight highlightIsNewGroup() const { return m_highlightIsNewGroup; };
> +    WebCore::HighlightRequestOriginatedInApp highlightRequestOriginatedInApp() const { return m_highlightRequestOriginatedInApp; };

Nit - don't need semicolons after the two inline functions here.

[Megan Gardner]

Created attachment 422108 [details]
Patch for landing

[EWS]

Committed r273826: <https://commits.webkit.org/r273826>

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

[EWS]

Found 1 new test failure: media/media-fragments/TC0051.html