We should forward Ad Click Attribution requests from the NavigationAction to the network process.
<rdar://problem/47650118>
Created attachment 361703 [details] Patch
Comment on attachment 361703 [details] Patch Attachment 361703 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11112916 New failing tests: http/tests/adClickAttribution/store-ad-click-attribution.html
Created attachment 361708 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 361703 [details] Patch Attachment 361703 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11113016 New failing tests: http/tests/adClickAttribution/store-ad-click-attribution.html
Created attachment 361713 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 361703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361703&action=review As I read through this patch this thought repeatedly hit me, "how can I write this code to shoot myself in the face." Please think about encapsulation. How can we avoid taking a shotgun to the face, foot, arm, torso. There is the potential for good here. Polish it up. > Source/WebCore/loader/AdClickAttribution.h:133 > + Source& source() { return m_source; }; Why return a lvalue ref? > Source/WebCore/loader/AdClickAttribution.h:134 > + Destination& destination() { return m_destination; }; Why return a lvalue ref? > Source/WebCore/loader/NavigationAction.h:138 > + Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; }; Why change the return value? > Source/WebKit/UIProcess/API/APINavigation.h:147 > + Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; } return lvalue ref?
(In reply to Daniel Bates from comment #7) > Comment on attachment 361703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361703&action=review > > As I read through this patch this thought repeatedly hit me, "how can I > write this code to shoot myself in the face." Please think about > encapsulation. How can we avoid taking a shotgun to the face, foot, arm, > torso. There is the potential for good here. Polish it up. > Thinking about this comment some more. It sounds negative. What I meant to say: this patch has potential and it would be better if it can be tightened up.
Comment on attachment 361703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361703&action=review > Source/WebCore/loader/AdClickAttribution.h:120 > + AdClickAttribution() = default; Why? > Source/WebCore/loader/AdClickAttribution.h:130 > + bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; }; Something feels weird about this convenience function. I think it's because you also exposed destination. It's confusing because why would a person use this function when they can just compare destination themselves since you exposed it. Yeah, I get that this helper does a bit more. Maybe making this a static, non-member function would help. Maybe tightening the encapsulation of source and destination. Design feels off. > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:36 > +HashMap<String, AdClickAttribution>& NetworkAdClickAttribution::ensureAttributionsForAdClickSource(const AdClickAttribution::Source& source) Use an alias for the return type so we don't have to repeat it everywhere. No need for AdClick in name. It's redundant with the class name > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:38 > + return m_adClickAttributionMap.ensure(source.registrableDomain, [] { Code formatting looks weird. Grep the code to look for other examples for how we write this. > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:69 > + if (builder.isEmpty()) Move this check to the top (obviously can't be in terms of builder then - do not find a way to write it in terms of builder), call the completion handler, and return. > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:45 > + HashMap<String, AdClickAttribution>& ensureAttributionsForAdClickSource(const AdClickAttribution::Source&); Write using Source *NOT* String. Use type alias see below for more details. > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:47 > + HashMap<String, HashMap<String, AdClickAttribution>> m_adClickAttributionMap; Write this in terms of Source and Destination for type safety. Otherwise, what is the point of having Source and Destination classes? Use type alias to avoid repeativeness see my remake in .cpp > LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:10 > +WebCore::AdClickAttribution 1 Stop doing this! This is a insult to me because I already told you this before in a previous bug (I will look it up when I have a moment). I am now taking this insult personally. I spend my time to provide review feedback and this is a slap in the face and reads that you do not care about such feedback. Stop printing text after TEST COMPLETE. Here's one solution to this problem: stop using js-test.js's description().
(In reply to Daniel Bates from comment #9) > Stop doing this! This is a insult to me because I already told you this > before in a previous bug (I will look it up when I have a moment). I am now > taking this insult personally. I spend my time to provide review feedback > and this is a slap in the face and reads that you do not care about such > feedback. Stop printing text after TEST COMPLETE. Here's one solution to > this problem: stop using js-test.js's description(). Actually, it's not just the text emitted after TEST COMPLETE it's also the missing PASS/FAIL messages and the extraneous text "Link", both isssues I also told you about in the same previous bug.
(In reply to Daniel Bates from comment #7) > Comment on attachment 361703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361703&action=review > > As I read through this patch this thought repeatedly hit me, "how can I > write this code to shoot myself in the face." Please think about > encapsulation. How can we avoid taking a shotgun to the face, foot, arm, > torso. There is the potential for good here. Polish it up. > > > Source/WebCore/loader/AdClickAttribution.h:133 > > + Source& source() { return m_source; }; > > Why return a lvalue ref? > > > Source/WebCore/loader/AdClickAttribution.h:134 > > + Destination& destination() { return m_destination; }; > > Why return a lvalue ref? > > > Source/WebCore/loader/NavigationAction.h:138 > > + Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; }; > > Why change the return value? > > > Source/WebKit/UIProcess/API/APINavigation.h:147 > > + Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; } > > return lvalue ref? These are structured this way because WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() takes a const NavigationAction& argument, calls navigationAction.adClickAttribution(), which returns m_lastNavigationAction.adClickAttribution. That chain has to be declared const and returning lvalue refs forces me to drop the const. Is there some other way to do this?
(In reply to John Wilander from comment #11) > (In reply to Daniel Bates from comment #7) > > Comment on attachment 361703 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=361703&action=review > > > > As I read through this patch this thought repeatedly hit me, "how can I > > write this code to shoot myself in the face." Please think about > > encapsulation. How can we avoid taking a shotgun to the face, foot, arm, > > torso. There is the potential for good here. Polish it up. > > > > > Source/WebCore/loader/AdClickAttribution.h:133 > > > + Source& source() { return m_source; }; > > > > Why return a lvalue ref? > > > > > Source/WebCore/loader/AdClickAttribution.h:134 > > > + Destination& destination() { return m_destination; }; > > > > Why return a lvalue ref? > > > > > Source/WebCore/loader/NavigationAction.h:138 > > > + Optional<AdClickAttribution> adClickAttribution() const { return m_adClickAttribution; }; > > > > Why change the return value? > > > > > Source/WebKit/UIProcess/API/APINavigation.h:147 > > > + Optional<WebCore::AdClickAttribution> adClickAttribution() const { return m_lastNavigationAction.adClickAttribution; } > > > > return lvalue ref? > > These are structured this way because > WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() takes a > const NavigationAction& argument, calls > navigationAction.adClickAttribution(), which returns > m_lastNavigationAction.adClickAttribution. That chain has to be declared > const and returning lvalue refs forces me to drop the const. > > Is there some other way to do this? All of the above should return const lvalue ref from a const function.
(In reply to Daniel Bates from comment #9) > Comment on attachment 361703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361703&action=review > > > Source/WebCore/loader/AdClickAttribution.h:120 > > + AdClickAttribution() = default; > > Why? The generated IPC code needs the default constructors added. Chris helped me figure this out. > > Source/WebCore/loader/AdClickAttribution.h:130 > > + bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; }; > > Something feels weird about this convenience function. I think it's because > you also exposed destination. It's confusing because why would a person use > this function when they can just compare destination themselves since you > exposed it. Yeah, I get that this helper does a bit more. Maybe making this > a static, non-member function would help. Maybe tightening the encapsulation > of source and destination. Design feels off. OK, I can do a regular comparison in WebKit::WebPageProxy instead, like so: Destination { frame->url().host().toString() } == adClickAttribution->destination() But it doesn't read as well as adClickAttribution->matchesDestination(frame->url()) … in my veiw. > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:36 > > +HashMap<String, AdClickAttribution>& NetworkAdClickAttribution::ensureAttributionsForAdClickSource(const AdClickAttribution::Source& source) > > Use an alias for the return type so we don't have to repeat it everywhere. Sure. > No need for AdClick in name. It's redundant with the class name > > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:38 > > + return m_adClickAttributionMap.ensure(source.registrableDomain, [] { > > Code formatting looks weird. Grep the code to look for other examples for > how we write this. > > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:69 > > + if (builder.isEmpty()) > > Move this check to the top (obviously can't be in terms of builder then - > do not find a way to write it in terms of builder), call the completion > handler, and return. Good idea. > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:45 > > + HashMap<String, AdClickAttribution>& ensureAttributionsForAdClickSource(const AdClickAttribution::Source&); > > Write using Source *NOT* String. Use type alias see below for more details. OK, I'll introduce the necessary support for hashes. > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.h:47 > > + HashMap<String, HashMap<String, AdClickAttribution>> m_adClickAttributionMap; > > Write this in terms of Source and Destination for type safety. Otherwise, > what is the point of having Source and Destination classes? Use type alias > to avoid repeativeness see my remake in .cpp OK, I'll introduce the necessary support for hashes. > > LayoutTests/http/tests/adClickAttribution/store-ad-click-attribution-expected.txt:10 > > +WebCore::AdClickAttribution 1 > > Stop doing this! This is a insult to me because I already told you this > before in a previous bug (I will look it up when I have a moment). I am now > taking this insult personally. I spend my time to provide review feedback > and this is a slap in the face and reads that you do not care about such > feedback. Stop printing text after TEST COMPLETE. Here's one solution to > this problem: stop using js-test.js's description(). Going forward, you can assume I'm not trying to insult anyone with my patches. The benefit of the doubt is a much better approach in my view. In this particular case, I'm using a dump function that appends to the test output. As discussed offline, I can avoid using js-test and output test results manually.
Look at Source/WebCore/page/csp/ContentSecurityPolicyHash.h in <https://bugs.webkit.org/attachment.cgi?id=273515&action=prettypatch> for an example of defining a HashSet<> compatible object. In that patch I make ContentSecurityPolicyHash HashSet<> compatible.
(In reply to Daniel Bates from comment #14) > Look at Source/WebCore/page/csp/ContentSecurityPolicyHash.h in > <https://bugs.webkit.org/attachment.cgi?id=273515&action=prettypatch> for an > example of defining a HashSet<> compatible object. In that patch I make > ContentSecurityPolicyHash HashSet<> compatible. Maybe looking at StringHash in HashTraits.h may help as well. Grepping the code for more examples.
(In reply to John Wilander from comment #13) > (In reply to Daniel Bates from comment #9) > > Comment on attachment 361703 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=361703&action=review > > > > > Source/WebCore/loader/AdClickAttribution.h:120 > > > + AdClickAttribution() = default; > > > > Why? > > The generated IPC code needs the default constructors added. Chris helped me > figure this out. > > > > Source/WebCore/loader/AdClickAttribution.h:130 > > > + bool matchesDestination(const URL& url) const { return m_destination == Destination { topPrivatelyControlledDomain(url.host().toString()) }; }; > > > > Something feels weird about this convenience function. I think it's because > > you also exposed destination. It's confusing because why would a person use > > this function when they can just compare destination themselves since you > > exposed it. Yeah, I get that this helper does a bit more. Maybe making this > > a static, non-member function would help. Maybe tightening the encapsulation > > of source and destination. Design feels off. > > OK, I can do a regular comparison in WebKit::WebPageProxy instead, like so: > Destination { frame->url().host().toString() } == > adClickAttribution->destination() > > But it doesn't read as well as > adClickAttribution->matchesDestination(frame->url()) > … in my veiw. > What about putting this matching function on Destination itself?
Created attachment 361876 [details] Patch
I've hopefully addressed all issues including the build errors on non-Apple ports (all missing header includes). Dan, the one thing I couldn't address was the alleged weird formatting of: return m_adClickAttributionMap.ensure(source.registrableDomain, [] { … in WebKit::NetworkAdClickAttribution. I looked at a handful of other ensure() calls and they seem formatted this way and the style checker doesn't complain.
Created attachment 361922 [details] Patch
Windows build fix. The rest is good to go. R+? :)
Comment on attachment 361922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361922&action=review r=me, lots of nits > Source/WebCore/loader/AdClickAttribution.cpp:96 > + builder.appendLiteral("Source: "); We might find it useful in the future to have this in JSON format. We can always change it if needed, but just a thought. > Source/WebCore/loader/AdClickAttribution.h:100 > String registrableDomain; > + bool isDeleted { false }; String has a constructor that takes HashTableDeletedValueType, so we don't need a separate bool for this. > Source/WebCore/loader/AdClickAttribution.h:167 > String registrableDomain; > + bool isDeleted { false }; ditto > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:67 > + builder.appendLiteral("WebCore::AdClickAttribution "); NetworkAdClickAttribution? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2301 > + completionHandler(emptyString()); I'm not sure if it's important to use emptyString here or if a null string would suffice. > Source/WebKit/UIProcess/WebPageProxy.cpp:4106 > + if (auto adClickAttribution = navigation->adClickAttribution()) { auto& No need to copy the Optional. > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2731 > + WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("dumpAdClickAttribution")); This is old style WKRetainPtr use. Try this instead: auto messageName = adoptWK(WKStringCreateWithUTF8CString(...)); > Tools/WebKitTestRunner/TestInvocation.h:148 > + String m_savedAdClickAttribution; This is never set. We can remove it.
(In reply to Alex Christensen from comment #21) > Comment on attachment 361922 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361922&action=review > > r=me, lots of nits > > > Source/WebCore/loader/AdClickAttribution.cpp:96 > > + builder.appendLiteral("Source: "); > > We might find it useful in the future to have this in JSON format. We can > always change it if needed, but just a thought. I see. Yeah, I'll fix that when/if needed. > > Source/WebCore/loader/AdClickAttribution.h:100 > > String registrableDomain; > > + bool isDeleted { false }; > > String has a constructor that takes HashTableDeletedValueType, so we don't > need a separate bool for this. Will fix. > > Source/WebCore/loader/AdClickAttribution.h:167 > > String registrableDomain; > > + bool isDeleted { false }; > > ditto Will fix. > > Source/WebKit/NetworkProcess/NetworkAdClickAttribution.cpp:67 > > + builder.appendLiteral("WebCore::AdClickAttribution "); > > NetworkAdClickAttribution? No, actually WebCore::AdClickAttribution. It'll output one of these per WebCore::AdClickAttribution object in the map and enumerate them. > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2301 > > + completionHandler(emptyString()); > > I'm not sure if it's important to use emptyString here or if a null string > would suffice. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:4106 > > + if (auto adClickAttribution = navigation->adClickAttribution()) { > > auto& > No need to copy the Optional. Will fix. > > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2731 > > + WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("dumpAdClickAttribution")); > > This is old style WKRetainPtr use. Try this instead: > auto messageName = adoptWK(WKStringCreateWithUTF8CString(...)); Will fix. > > Tools/WebKitTestRunner/TestInvocation.h:148 > > + String m_savedAdClickAttribution; > > This is never set. We can remove it. Will fix. Thanks Alex and Dan for all of the review comments!
Created attachment 361931 [details] Patch
Comment on attachment 361931 [details] Patch Just making sure the Windows bot is happy before landing.
Actually, the previous EWS run already successfully built on Windows. Yay.
Comment on attachment 361931 [details] Patch Clearing flags on attachment: 361931 Committed r241451: <https://trac.webkit.org/changeset/241451>
All reviewed patches have been landed. Closing bug.
Given https://bugs.webkit.org/show_bug.cgi?id=194774 and the fact that it was still asserting after that, and the test started failing when I disabled safe browsing, this is fundamentally flawed somehow. I reverted the WebPageProxy part of this patch in https://trac.webkit.org/changeset/241754/webkit
http://trac.webkit.org/r241755
This was resolved again through patches tracked in the related bugs ("See Also").