Attachment 361029 Details for Bug 194104 – Patch

[patch] Patch

bug-194104-20190203141844.patch (text/plain), 18.53 KB, created by John Wilander on 2019-02-03 14:18:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: John Wilander

Created: 2019-02-03 14:18:45 PST

Size: 18.53 KB

patch

obsolete

>Subversion Revision: 240906
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 98128cdcd1a884195fe29eddb4c521d5870aee26..984bd9cf410d6478d473506757b9d07ae50af46f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-02-03  John Wilander  <wilander@apple.com>
>+
>+        Parse and handle Ad Click Attribution attributes in HTMLAnchorElement::handleClick()
>+        https://bugs.webkit.org/show_bug.cgi?id=194104
>+        <rdar://problem/47649991>
>+
>+        Reviewed by Chris Dumez, Daniel Bates, and Darin Adler.
>+
>+        Test: http/tests/adClickAttribution/anchor-tag-attributes-validation.html
>+
>+        This patch adds parsing and validation of the two new Ad Click Attribution
>+        attributes in anchor elements: adcampaignid and addestination. The data is
>+        not yet forwarded into the loader.
>+
>+        * html/HTMLAnchorElement.cpp:
>+        (WebCore::HTMLAnchorElement::parseAdClickAttribution const):
>+        (WebCore::HTMLAnchorElement::handleClick):
>+            Now calls HTMLAnchorElement::parseAdClickAttribution().
>+        * html/HTMLAnchorElement.h:
>+        * loader/AdClickAttribution.h:
>+            Made WebCore::AdClickAttribution copyable since it's needed to have it be
>+            WTF::Optional. Also made AdClickAttribution::MaxEntropy public. Changed
>+            numeric types from unsigned short to uint32_t.
>+        (WebCore::AdClickAttribution::Campaign::isValid const):
>+        (WebCore::AdClickAttribution::Conversion::isValid const):
>+
> 2019-02-03  Antti Koivisto  <antti@apple.com>
> 
>         Don't include ScrollCoordinator.h from Element.h
>diff --git a/Source/WebCore/html/HTMLAnchorElement.cpp b/Source/WebCore/html/HTMLAnchorElement.cpp
>index e7e167c95b4177513432f96c6f41e385afb62e44..071870c472a08da10ef2b891fc389cd0f7c3ee33 100644
>--- a/Source/WebCore/html/HTMLAnchorElement.cpp
>+++ b/Source/WebCore/html/HTMLAnchorElement.cpp
>@@ -24,6 +24,7 @@
> #include "config.h"
> #include "HTMLAnchorElement.h"
> 
>+#include "AdClickAttribution.h"
> #include "DOMTokenList.h"
> #include "ElementIterator.h"
> #include "EventHandler.h"
>@@ -50,8 +51,10 @@
> #include "SecurityPolicy.h"
> #include "Settings.h"
> #include "URLUtils.h"
>+#include "UserGestureIndicator.h"
> #include <wtf/IsoMallocInlines.h>
> #include <wtf/text/StringBuilder.h>
>+#include <wtf/text/StringConcatenateNumbers.h>
> 
> namespace WebCore {
> 
>@@ -394,6 +397,52 @@ bool HTMLAnchorElement::isSystemPreviewLink() const
> }
> #endif
> 
>+Optional<AdClickAttribution> HTMLAnchorElement::parseAdClickAttribution() const
>+{
>+    using Campaign = AdClickAttribution::Campaign;
>+    using Source = AdClickAttribution::Source;
>+    using Destination = AdClickAttribution::Destination;
>+
>+    if (!RuntimeEnabledFeatures::sharedFeatures().adClickAttributionEnabled() || !UserGestureIndicator::processingUserGesture())
>+        return WTF::nullopt;
>+
>+    if (!hasAttributeWithoutSynchronization(adcampaignidAttr) && !hasAttributeWithoutSynchronization(addestinationAttr))
>+        return WTF::nullopt;
>+    
>+    auto adCampaignIDAttr = attributeWithoutSynchronization(adcampaignidAttr);
>+    auto adDestinationAttr = attributeWithoutSynchronization(addestinationAttr);
>+    
>+    if (adCampaignIDAttr.isEmpty() || adDestinationAttr.isEmpty()) {
>+        document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, "Both adcampaignid and addestination need to be set for Ad Click Attribution to work."_s);
>+        return WTF::nullopt;
>+    }
>+
>+    RefPtr<Frame> frame = document().frame();
>+    if (!frame || !frame->isMainFrame()) {
>+        document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, "Ad Click Attribution is only supported in the main frame."_s);
>+        return WTF::nullopt;
>+    }
>+    
>+    auto adCampaignID = parseHTMLNonNegativeInteger(adCampaignIDAttr);
>+    if (!adCampaignID) {
>+        document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, "adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution."_s);
>+        return WTF::nullopt;
>+    }
>+    
>+    if (adCampaignID.value() >= AdClickAttribution::MaxEntropy) {
>+        document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, makeString("adcampaignid must have a non-negative value less than ", AdClickAttribution::MaxEntropy, " for Ad Click Attribution."));
>+        return WTF::nullopt;
>+    }
>+
>+    URL adDestinationURL { URL(), adDestinationAttr };
>+    if (!adDestinationURL.isValid() || !adDestinationURL.protocolIsInHTTPFamily()) {
>+        document().addConsoleMessage(MessageSource::Other, MessageLevel::Warning, "adddestination could not be converted to a valid HTTP-family URL."_s);
>+        return WTF::nullopt;
>+    }
>+
>+    return AdClickAttribution { Campaign(adCampaignID.value()), Source(document().domain()), Destination(adDestinationURL.host().toString()) };
>+}
>+
> void HTMLAnchorElement::handleClick(Event& event)
> {
>     event.setDefaultHandled();
>@@ -438,6 +487,13 @@ void HTMLAnchorElement::handleClick(Event& event)
>     else if (hasRel(Relation::NoOpener) || (RuntimeEnabledFeatures::sharedFeatures().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(effectiveTarget, "_blank")))
>         newFrameOpenerPolicy = NewFrameOpenerPolicy::Suppress;
> 
>+    auto adClickAttribution = parseAdClickAttribution();
>+    // FIXME: The adClickAttribution should be forwarded to the loader and handled down the pipe. See
>+    // rdar://problem/47650118
>+    // A matching conversion event needs to happen before the complete ad click attributionURL can be
>+    // created. Thus, it should be empty for now.
>+    ASSERT_UNUSED(adClickAttribution, !adClickAttribution || adClickAttribution->url().isNull());
>+    
>     frame->loader().urlSelected(completedURL, effectiveTarget, &event, LockHistory::No, LockBackForwardList::No, shouldSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), newFrameOpenerPolicy, downloadAttribute, systemPreviewInfo);
> 
>     sendPings(completedURL);
>diff --git a/Source/WebCore/html/HTMLAnchorElement.h b/Source/WebCore/html/HTMLAnchorElement.h
>index 6b02c9ab4d462685036e170396e45b81e8d329e8..61315ced17fa4d6e06526cd90bb588aadfa2af97 100644
>--- a/Source/WebCore/html/HTMLAnchorElement.h
>+++ b/Source/WebCore/html/HTMLAnchorElement.h
>@@ -28,9 +28,11 @@
> #include "SharedStringHash.h"
> #include "URLUtils.h"
> #include <wtf/OptionSet.h>
>+#include <wtf/Optional.h>
> 
> namespace WebCore {
> 
>+class AdClickAttribution;
> class DOMTokenList;
> 
> // Link relation bitmask values.
>@@ -95,6 +97,8 @@ private:
> 
>     void sendPings(const URL& destinationURL);
> 
>+    Optional<AdClickAttribution> parseAdClickAttribution() const;
>+
>     void handleClick(Event&);
> 
>     enum EventType {
>diff --git a/Source/WebCore/loader/AdClickAttribution.h b/Source/WebCore/loader/AdClickAttribution.h
>index 7df05ee5eb3ea87f9f9a2640538196e610d54733..fe06a1bf2faea81a0ab55ba266095ac348c61c9c 100644
>--- a/Source/WebCore/loader/AdClickAttribution.h
>+++ b/Source/WebCore/loader/AdClickAttribution.h
>@@ -37,14 +37,13 @@ class URL;
> 
> namespace WebCore {
> 
>-constexpr unsigned short maxEntropy = 64;
>-
> class AdClickAttribution {
>-    WTF_MAKE_NONCOPYABLE(AdClickAttribution);
> public:
>-    using CampaignId = unsigned short;
>-    using ConversionData = unsigned short;
>-    using PriorityValue = unsigned short;
>+    using CampaignId = uint32_t;
>+    using ConversionData = uint32_t;
>+    using PriorityValue = uint32_t;
>+
>+    static constexpr uint32_t MaxEntropy = 64;
> 
>     struct Campaign {
>         explicit Campaign(CampaignId id)
>@@ -54,7 +53,7 @@ public:
>         
>         bool isValid() const
>         {
>-            return id < maxEntropy;
>+            return id < MaxEntropy;
>         }
>         
>         CampaignId id;
>@@ -104,7 +103,7 @@ public:
> 
>         bool isValid() const
>         {
>-            return data < maxEntropy && priority < maxEntropy;
>+            return data < MaxEntropy && priority < MaxEntropy;
>         }
>         
>         ConversionData data;
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index c0ae8eb2d5fa6fb49c035aacc3af4e9f28577d4a..103927854cdb7d94c8278451fe5d81883a542c7b 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-02-03  John Wilander  <wilander@apple.com>
>+
>+        Parse and handle Ad Click Attribution attributes in HTMLAnchorElement::handleClick()
>+        https://bugs.webkit.org/show_bug.cgi?id=194104
>+        <rdar://problem/47649991>
>+
>+        Reviewed by Chris Dumez, Daniel Bates, and Darin Adler.
>+
>+        * TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp:
>+        (TestWebKitAPI::TEST):
>+            Changed numeric types from unsigned short to uint32_t.
>+
> 2019-02-02  David Kilzer  <ddkilzer@apple.com>
> 
>         Leak of NSArray (4.25 Kbytes) in com.apple.WebKit.WebContent running WebKit layout tests on iOS Simulator
>diff --git a/Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp b/Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp
>index 57ce28170f566fd939af8c363d7c386040729540..a9ca75d71ef380d52f48c5c01472d8af26083faf 100644
>--- a/Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebCore/AdClickAttribution.cpp
>@@ -33,8 +33,8 @@ using namespace WebCore;
> 
> namespace TestWebKitAPI {
> 
>-constexpr unsigned short min6BitValue { 0 };
>-constexpr unsigned short max6BitValue { 63 };
>+constexpr uint32_t min6BitValue { 0 };
>+constexpr uint32_t max6BitValue { 63 };
> 
> // Positive test cases.
> 
>@@ -52,8 +52,8 @@ TEST(AdClickAttribution, ValidMinValues)
> 
> TEST(AdClickAttribution, ValidMidValues)
> {
>-    AdClickAttribution attribution(AdClickAttribution::Campaign((unsigned short)12), AdClickAttribution::Source("webkit.org"), AdClickAttribution::Destination("example.com"));
>-    attribution.setConversion(AdClickAttribution::Conversion((unsigned short)44, AdClickAttribution::Priority((unsigned short)22)));
>+    AdClickAttribution attribution(AdClickAttribution::Campaign((uint32_t)12), AdClickAttribution::Source("webkit.org"), AdClickAttribution::Destination("example.com"));
>+    attribution.setConversion(AdClickAttribution::Conversion((uint32_t)44, AdClickAttribution::Priority((uint32_t)22)));
> 
>     auto attributionURL = attribution.url();
>     auto referrerURL = attribution.referrer();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 3c0ced349ead9d67f764d7fed4974f95c944cf38..b5c77e9284c453b21de9297b6e325b0d6a6476ac 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-02-03  John Wilander  <wilander@apple.com>
>+
>+        Parse and handle Ad Click Attribution attributes in HTMLAnchorElement::handleClick()
>+        https://bugs.webkit.org/show_bug.cgi?id=194104
>+        <rdar://problem/47649991>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This test case makes sure invalid data triggers console warnings.
>+
>+        * http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt: Added.
>+        * http/tests/adClickAttribution/anchor-tag-attributes-validation.html: Added.
>+        * platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt: Added.
>+            Console line numbers are not emitted when running iOS tests so this -expected.txt file doesn't have them.
>+
> 2019-02-03  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Unable to move selection into editable roots with 0 height
>diff --git a/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt b/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..311084e6d34ff2348d5dd98a7a4ec29e833e2921
>--- /dev/null
>+++ b/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt
>@@ -0,0 +1,33 @@
>+CONSOLE MESSAGE: line 107: adcampaignid must have a non-negative value less than 64 for Ad Click Attribution.
>+CONSOLE MESSAGE: line 107: adcampaignid must have a non-negative value less than 64 for Ad Click Attribution.
>+CONSOLE MESSAGE: line 107: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: line 107: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: line 107: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: line 107: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: line 107: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: line 107: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: line 107: Both adcampaignid and addestination need to be set for Ad Click Attribution to work.
>+CONSOLE MESSAGE: line 107: Both adcampaignid and addestination need to be set for Ad Click Attribution to work.
>+Test for validity of ad click attribution attributes on anchor tags.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+Link1
>+Link2
>+Link3
>+Link4
>+Link5
>+Link6
>+Link7
>+Link8
>+Link9
>+Link10
>+Link11
>+Link12
>+Link13
>+Link14
>+
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation.html b/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..1da45788c7d0c41e02e7224e2e2d9828f0852d13
>--- /dev/null
>+++ b/LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation.html
>@@ -0,0 +1,79 @@
>+<!DOCTYPE html> 
>+<html lang="en">
>+<head>
>+    <meta charset="UTF-8">
>+    <meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
>+    <script src="/js-test-resources/js-test.js"></script>
>+    <script src="/js-test-resources/ui-helper.js"></script>
>+</head>
>+<body onload="runAllTests()">
>+<div id="description"></div>
>+<div id="output"></div><br>
>+<div id="console"></div>
>+<script>
>+    description("Test for validity of ad click attribution attributes on anchor tags.");
>+    jsTestIsAsync = true;
>+
>+    function createAdClickAttributionAnchorElement(elementID, adCampaignID, adDestination) {
>+        let anchorElement = document.createElement("a");
>+        anchorElement.id = elementID;
>+        anchorElement.adcampaignid = adCampaignID;
>+        anchorElement.addestination = adDestination;
>+        anchorElement.href = "#";
>+        anchorElement.innerText = "Link" + currentTest;
>+        return anchorElement;
>+    }
>+
>+    function activateElement(elementID, callback) {
>+        var element = document.getElementById(elementID);
>+        var centerX = element.offsetLeft + element.offsetWidth / 2;
>+        var centerY = element.offsetTop + element.offsetHeight / 2;
>+        UIHelper.activateAt(centerX, centerY).then(
>+            function () {
>+                callback();
>+            },
>+            function () {
>+                testFailed("Promise rejected.");
>+                finishJSTest();
>+            }
>+        );
>+    }
>+
>+    let currentTest = 0;
>+    function runOneTest(adCampaignID, adDestination, callback) {
>+        const currentElementID = "test" + currentTest++;
>+        const anchorElement = createAdClickAttributionAnchorElement(currentElementID, adCampaignID, adDestination);
>+        output.appendChild(anchorElement);
>+        const brElement = document.createElement("br");
>+        output.appendChild(brElement);
>+        activateElement(currentElementID, callback);
>+    }
>+
>+    const validAdCampaignID = "03";
>+    const validAdDestination = "http://webkit.org";
>+    const testCases = [
>+        [ validAdCampaignID, validAdDestination ],
>+        [ "100", validAdDestination ],           // Too many characters.
>+        [ "1", validAdDestination ],             // Too few characters.
>+        [ "98", validAdDestination ],            // Too high value.
>+        [ "-1", validAdDestination ],            // Negative value.
>+        [ "ab", validAdDestination ],            // Non-digits.
>+        [ "ï£¿1", validAdDestination ],            // Non-ASCII.
>+        [ " 1", validAdDestination ],            // Leading space.
>+        [ "1 ", validAdDestination ],            // Trailing space.
>+        [ validAdCampaignID, "webkit.org" ],     // Missing protocol.
>+        [ validAdCampaignID, "://webkit.org" ],  // Partially missing protocol.
>+        [ validAdCampaignID, "ï£¿ï£¿ï£¿" ],           // Non-ASCII characters as destination.
>+        [ "", validAdDestination ],              // Empty campaign ID.
>+        [ validAdCampaignID, "" ]                // Empty destination.
>+    ];
>+
>+    function runAllTests() {
>+        if (currentTest < testCases.length)
>+            runOneTest(testCases[currentTest][0], testCases[currentTest][1], runAllTests);
>+        else
>+            finishJSTest();
>+    }
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt b/LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..2ca10c55eec5190d4f8a48a812925903dacb1182
>--- /dev/null
>+++ b/LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt
>@@ -0,0 +1,33 @@
>+CONSOLE MESSAGE: adcampaignid must have a non-negative value less than 64 for Ad Click Attribution.
>+CONSOLE MESSAGE: adcampaignid must have a non-negative value less than 64 for Ad Click Attribution.
>+CONSOLE MESSAGE: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: adcampaignid can not be converted to a non-negative integer which is required for Ad Click Attribution.
>+CONSOLE MESSAGE: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: adddestination could not be converted to a valid HTTP-family URL.
>+CONSOLE MESSAGE: Both adcampaignid and addestination need to be set for Ad Click Attribution to work.
>+CONSOLE MESSAGE: Both adcampaignid and addestination need to be set for Ad Click Attribution to work.
>+Test for validity of ad click attribution attributes on anchor tags.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+Link1
>+Link2
>+Link3
>+Link4
>+Link5
>+Link6
>+Link7
>+Link8
>+Link9
>+Link10
>+Link11
>+Link12
>+Link13
>+Link14
>+
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 194104: 360752 | 360754 | 360783 | 360790 | 360803 | 360816 | 360820 | 360894 | 360900 | 360904 | 361029 | 361031