We should add Ad Click Attribution as an experimental feature.
<rdar://problem/47450399>
Created attachment 359779 [details] Patch
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 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
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 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
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
Created attachment 359809 [details] Patch
Created attachment 359815 [details] Patch
The mac bot test failure may be because the test should be WK2 only.
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
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 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 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
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 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
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 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 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 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?
(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.
(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!
(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).
(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.
Created attachment 359904 [details] Patch
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
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
Created attachment 359921 [details] Patch
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
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
Created attachment 359974 [details] Patch
Comment on attachment 359974 [details] Patch r=me
Comment on attachment 359974 [details] Patch Thanks, Brent! Also, thanks Dan and Wenson.
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?
(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 on attachment 359974 [details] Patch Clearing flags on attachment: 359974 Committed r240444: <https://trac.webkit.org/changeset/240444>
All reviewed patches have been landed. Closing bug.