Bug 193685 – Add Ad Click Attribution as an internal/experimental feature

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We should add Ad Click Attribution as an experimental feature.

[John Wilander]

<rdar://problem/47450399>

[John Wilander]

Created attachment 359779 [details]
Patch

[John Wilander]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

Created attachment 359809 [details]
Patch

[John Wilander]

Created attachment 359815 [details]
Patch

[John Wilander]

The mac bot test failure may be because the test should be WK2 only.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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”

[Daniel Bates]

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?

[Daniel Bates]

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?

[John Wilander]

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

[John Wilander]

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

[John Wilander]

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

[John Wilander]

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

[John Wilander]

Created attachment 359904 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

Created attachment 359921 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

Created attachment 359974 [details]
Patch

[Brent Fulgham]

Comment on attachment 359974 [details]
Patch

r=me

[John Wilander]

Comment on attachment 359974 [details]
Patch

Thanks, Brent! Also, thanks Dan and Wenson.

[Chris Dumez]

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?

[John Wilander]

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

[WebKit Commit Bot]

Comment on attachment 359974 [details]
Patch

Clearing flags on attachment: 359974

Committed r240444: <https://trac.webkit.org/changeset/240444>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.