Bug 194510 – Store Ad Click Attribution requests in the network process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should forward Ad Click Attribution requests from the NavigationAction to the network process.

[John Wilander]

<rdar://problem/47650118>

[John Wilander]

Created attachment 361703 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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?

[Daniel Bates]

(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.

[Daniel Bates]

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().

[Daniel Bates]

(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.

[John Wilander]

(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?

[Daniel Bates]

(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.

[John Wilander]

(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.

[Daniel Bates]

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.

[Daniel Bates]

(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.

[Daniel Bates]

(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?

[John Wilander]

Created attachment 361876 [details]
Patch

[John Wilander]

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.

[John Wilander]

Created attachment 361922 [details]
Patch

[John Wilander]

Windows build fix. The rest is good to go. R+? :)

[Alex Christensen]

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.

[John Wilander]

(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!

[John Wilander]

Created attachment 361931 [details]
Patch

[John Wilander]

Comment on attachment 361931 [details]
Patch

Just making sure the Windows bot is happy before landing.

[John Wilander]

Actually, the previous EWS run already successfully built on Windows. Yay.

[WebKit Commit Bot]

Comment on attachment 361931 [details]
Patch

Clearing flags on attachment: 361931

Committed r241451: <https://trac.webkit.org/changeset/241451>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

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

[Alex Christensen]

http://trac.webkit.org/r241755

[John Wilander]

This was resolved again through patches tracked in the related bugs ("See Also").