Bug 193685

Summary: Add Ad Click Attribution as an internal/experimental feature
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, kondapallykalyan, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193748
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Archive of layout-test-results from ews202 for win-future
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews200 for win-future
none
Patch none

Description John Wilander 2019-01-22 14:36:01 PST
We should add Ad Click Attribution as an experimental feature.
Comment 1 John Wilander 2019-01-22 14:36:24 PST
<rdar://problem/47450399>
Comment 2 John Wilander 2019-01-22 14:47:51 PST
Created attachment 359779 [details]
Patch
Comment 3 John Wilander 2019-01-22 15:44:46 PST
js/dom/dom-static-property-for-in-iteration.html is failing (text diff) because it's iterating over all attributes and now there are two more. I'll update the -expected.txt file once the bots are done with the rest.
Comment 4 EWS Watchlist 2019-01-22 15:57:08 PST
Comment on attachment 359779 [details]
Patch

Attachment 359779 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10846051

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
Comment 5 EWS Watchlist 2019-01-22 15:57:10 PST
Created attachment 359794 [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 6 EWS Watchlist 2019-01-22 16:13:56 PST
Comment on attachment 359779 [details]
Patch

Attachment 359779 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10846185

New failing tests:
js/dom/dom-static-property-for-in-iteration.html
Comment 7 EWS Watchlist 2019-01-22 16:13:58 PST
Created attachment 359800 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 8 John Wilander 2019-01-22 16:53:43 PST
Created attachment 359809 [details]
Patch
Comment 9 John Wilander 2019-01-22 17:06:20 PST
Created attachment 359815 [details]
Patch
Comment 10 John Wilander 2019-01-22 17:50:39 PST
The mac bot test failure may be because the test should be WK2 only.
Comment 11 EWS Watchlist 2019-01-22 18:07:09 PST
Comment on attachment 359815 [details]
Patch

Attachment 359815 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10848621

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-reflect.html
Comment 12 EWS Watchlist 2019-01-22 18:07:10 PST
Created attachment 359828 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 13 Brent Fulgham 2019-01-22 19:41:00 PST
Comment on attachment 359815 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.cpp:113
> +            adClickAttributionEnabled = parseBooleanTestHeaderValue(value);

I think you need this change in WebKitTestRunner, too.
Comment 14 EWS Watchlist 2019-01-22 20:00:14 PST
Comment on attachment 359815 [details]
Patch

Attachment 359815 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10849712

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-reflect.html
Comment 15 EWS Watchlist 2019-01-22 20:00:15 PST
Created attachment 359842 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 16 EWS Watchlist 2019-01-22 20:19:19 PST
Comment on attachment 359815 [details]
Patch

Attachment 359815 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10849995

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-reflect.html
Comment 17 EWS Watchlist 2019-01-22 20:19:30 PST
Created attachment 359843 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 18 Daniel Bates 2019-01-22 20:36:40 PST
Comment on attachment 359815 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:379
> +    return adcampaignidAttr->localName();

One word OR three words? “adClick” but here “adcampaignid”.

> Source/WebCore/html/HTMLAnchorElement.cpp:387
> +    return addestinationAttr->localName();

One word or two words? “adClick” but here “adddestination”
Comment 19 Daniel Bates 2019-01-22 20:38:05 PST
Comment on attachment 359815 [details]
Patch

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

> LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-reflect.html:1
> +<!DOCTYPE html> <!-- webkit-test-runner [ internal:AdClickAttributionEnabled=true ] -->

Internal? Did you test this?
Comment 20 Daniel Bates 2019-01-22 20:48:38 PST
Comment on attachment 359815 [details]
Patch

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

>> Source/WebCore/html/HTMLAnchorElement.cpp:379
>> +    return adcampaignidAttr->localName();
> 
> One word OR three words? “adClick” but here “adcampaignid”.

We don’t generate this function given this a reflected attribute? LocalName?

>> Source/WebCore/html/HTMLAnchorElement.cpp:387
>> +    return addestinationAttr->localName();
> 
> One word or two words? “adClick” but here “adddestination”

We don’t generate this function given this is a reflected attribute? LocalName?

> Source/WebCore/html/HTMLAnchorElement.h:72
> +    const AtomicString& adcampaignid() const;

Been away from bindings for a while... we don’t generate these for reflected IDL attributes?
Comment 21 John Wilander 2019-01-23 10:31:20 PST
(In reply to Daniel Bates from comment #19)
> Comment on attachment 359815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359815&action=review
> 
> > LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-reflect.html:1
> > +<!DOCTYPE html> <!-- webkit-test-runner [ internal:AdClickAttributionEnabled=true ] -->
> 
> Internal? Did you test this?

Yes.
Comment 22 John Wilander 2019-01-23 10:34:18 PST
(In reply to Daniel Bates from comment #20)
> Comment on attachment 359815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359815&action=review
> 
> >> Source/WebCore/html/HTMLAnchorElement.cpp:379
> >> +    return adcampaignidAttr->localName();
> > 
> > One word OR three words? “adClick” but here “adcampaignid”.
> 
> We don’t generate this function given this a reflected attribute? LocalName?

Aha. I did not know reflected attributes where auto-generated. Thanks for pointing that out. I didn't come across them when browsing existing code. Did I just miss them or are they not in some derived code?

The localName return is just a stub. Not needed since these functions are not needed.

> >> Source/WebCore/html/HTMLAnchorElement.cpp:387
> >> +    return addestinationAttr->localName();
> > 
> > One word or two words? “adClick” but here “adddestination”
> 
> We don’t generate this function given this is a reflected attribute?
> LocalName?

Ditto.

> > Source/WebCore/html/HTMLAnchorElement.h:72
> > +    const AtomicString& adcampaignid() const;
> 
> Been away from bindings for a while... we don’t generate these for reflected
> IDL attributes?

We most probably do. I'll remove my implementations of them. The plan was to place some input validation in the implementation but that'll have to be done elsewhere.

Thanks for the comments, Dan!
Comment 23 John Wilander 2019-01-23 10:35:23 PST
(In reply to Daniel Bates from comment #18)
> Comment on attachment 359815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359815&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:379
> > +    return adcampaignidAttr->localName();
> 
> One word OR three words? “adClick” but here “adcampaignid”.

I looked ad the existing attributes and they are not camel-cased. That's why I left the developer facing parts that way.

> > Source/WebCore/html/HTMLAnchorElement.cpp:387
> > +    return addestinationAttr->localName();
> 
> One word or two words? “adClick” but here “adddestination”

Ditto (but with just two 'd's in addestination).
Comment 24 John Wilander 2019-01-23 10:52:35 PST
(In reply to John Wilander from comment #21)
> (In reply to Daniel Bates from comment #19)
> > Comment on attachment 359815 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=359815&action=review
> > 
> > > LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-reflect.html:1
> > > +<!DOCTYPE html> <!-- webkit-test-runner [ internal:AdClickAttributionEnabled=true ] -->
> > 
> > Internal? Did you test this?
> 
> Yes.

I see what you're saying. It was wrong in TestOptions. But it works on my machine and WK2 bots. I'll fix TestOptions.
Comment 25 John Wilander 2019-01-23 10:55:28 PST
Created attachment 359904 [details]
Patch
Comment 26 EWS Watchlist 2019-01-23 11:59:41 PST
Comment on attachment 359904 [details]
Patch

Attachment 359904 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10861036

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-reflect.html
Comment 27 EWS Watchlist 2019-01-23 11:59:43 PST
Created attachment 359918 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 28 John Wilander 2019-01-23 12:22:18 PST
Created attachment 359921 [details]
Patch
Comment 29 EWS Watchlist 2019-01-23 15:22:45 PST
Comment on attachment 359921 [details]
Patch

Attachment 359921 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10863349

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-reflect.html
Comment 30 EWS Watchlist 2019-01-23 15:23:00 PST
Created attachment 359951 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 31 John Wilander 2019-01-23 16:52:55 PST
Created attachment 359974 [details]
Patch
Comment 32 Brent Fulgham 2019-01-24 10:49:35 PST
Comment on attachment 359974 [details]
Patch

r=me
Comment 33 John Wilander 2019-01-24 10:50:47 PST
Comment on attachment 359974 [details]
Patch

Thanks, Brent! Also, thanks Dan and Wenson.
Comment 34 Chris Dumez 2019-01-24 10:53:43 PST
Comment on attachment 359974 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +            Addeed two new experimental attributes:

Typo: Addeed

> Source/WebCore/html/HTMLAnchorElement.idl:22
> +    [CEReactions=NotNeeded, EnabledAtRuntime=AdClickAttribution, Reflect] attribute DOMString adcampaignid;

no camel case?
Comment 35 John Wilander 2019-01-24 11:03:15 PST
(In reply to Chris Dumez from comment #34)
> Comment on attachment 359974 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359974&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +            Addeed two new experimental attributes:
> 
> Typo: Addeed
> 
> > Source/WebCore/html/HTMLAnchorElement.idl:22
> > +    [CEReactions=NotNeeded, EnabledAtRuntime=AdClickAttribution, Reflect] attribute DOMString adcampaignid;
> 
> no camel case?

I would prefer camel case but looking at existing attributes, it seems they should be all lower case. Easy enough to change once/if this thing ends up in standards discussions.
Comment 36 WebKit Commit Bot 2019-01-24 11:17:15 PST
Comment on attachment 359974 [details]
Patch

Clearing flags on attachment: 359974

Committed r240444: <https://trac.webkit.org/changeset/240444>
Comment 37 WebKit Commit Bot 2019-01-24 11:17:17 PST
All reviewed patches have been landed.  Closing bug.