[webkit-reviews] review granted: [Bug 214176] PCM: Accept ad click data when the link opens a new window : [Attachment 412569] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 214176: PCM: Accept ad click data when the link opens a new window
https://bugs.webkit.org/show_bug.cgi?id=214176

Attachment 412569: Patch

https://bugs.webkit.org/attachment.cgi?id=412569&action=review




--- Comment #18 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 412569
  --> https://bugs.webkit.org/attachment.cgi?id=412569
Patch

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

r=me

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:4659
>>> +		 adClickAttribution = navigation->adClickAttribution();
>> 
>> I'm not sure I understand this bit. It seems like the ad click attribution
in m_newPageNavigationAdClickAttribution was present at the time the page load
started. But now that the load is committed, we can check the navigation and
see if it has attribution data.
>> 
>> Is the attribution data in the member variable "fresher" than the
attribution generated as part of the navigation? Or are you concerned that the
client might manipulate or clear the attribution state known at the start of
the navigation, and that should always be the governing attribution?
>> 
>> This seems to say:
>> 1. If we had attribution at the start of the navigation, use that.
>> 2. ... but let the client possibly add attribution if it didn't exist at the
start of the navigation
>> 
>> If that's not the goal here, we could make it a debug variable, where we
have the m_newPageNavigationAdClickAttribution present in debug builds, and
ASSERT if the client clears it for some reason. That might help Chris feel
better about having to add the new member.
> 
> What you’re seeing is the gist of the bug fix.
> 
> In the case of a navigation in the same window, the NavigationAction object
will have the ad click attribution data and when the page load is committed we
send that data to the network process. That’s the existing code in trunk.
> 
> In the case of a navigation that opens a new window, we hand over to the
client using WebKit which is TestRunner when we run layout tests and the
browser or app itself in real usage. That’s when the NavigationAction is not
carried over to the new WebPageProxy. To make sure the client can’t regress
that, I now copy and ad click data to the completion handler which will execute
with access to the new WebPageProxy and save it on the proxy there.
> 
> Later when we have a committed load, I check “do I have a pending ad click
for a new window navigation? If not, do I have a pending ad click for a
same-window navigation?”

I see. Thanks for clarifying!