Summary: | Preserve information about the origin of the app highlight request | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, ews-watchlist, japhet, jiewen_tan, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2021-02-19 22:36:44 PST
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
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 |