Preserve information about the origin of the app highlight request
Created attachment 421082 [details] Patch
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`.
> 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")
Created attachment 421229 [details] Patch
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)
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.
(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.
Created attachment 421238 [details] Patch
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&&
Created attachment 421253 [details] Patch
Created attachment 421271 [details] Patch
<rdar://problem/74821306>
Created attachment 422026 [details] Patch
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.
Created attachment 422108 [details] Patch for landing
Committed r273826: <https://commits.webkit.org/r273826> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422108 [details].
Found 1 new test failure: media/media-fragments/TC0051.html