Bug 194104 – Parse and handle Ad Click Attribution attributes in HTMLAnchorElement::handleClick()

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

[John Wilander]

The experimental feature Ad Click Attribution adds two new attributes to anchor elements: adcampaignid and addestination. These needs to be parsed and validated as part of HTMLAnchorElement::handleClick().

[John Wilander]

<rdar://problem/47649991>

[John Wilander]

Created attachment 360752 [details]
Patch

[John Wilander]

There's a missing "in" in the Changelog entry title. I will fix that before landing anything.

[John Wilander]

Created attachment 360754 [details]
Patch

[John Wilander]

An attempt to fix the test timeout on Mac (went from actual link navigations to just "#" as href).

[John Wilander]

The ios-sim test failure might be because of the event sender. I've had problems with it before on ios-sim. :(

[John Wilander]

Wenson gave me a tip on how I might fix the ios-sim test.

[EWS Watchlist]

Comment on attachment 360754 [details]
Patch

Attachment 360754 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10977848

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-validation.html

[EWS Watchlist]

Created attachment 360783 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 360754 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:483
> +    ASSERT(!adClickAttribution || adClickAttribution->url().isEmpty());

It'd be nice to have a FIXME comment here to indicate you plan to do something with this variable in the future. Also, this should probably be an ASSERT_UNUSED().

[John Wilander]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 360754 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360754&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:483
> > +    ASSERT(!adClickAttribution || adClickAttribution->url().isEmpty());
> 
> It'd be nice to have a FIXME comment here to indicate you plan to do
> something with this variable in the future. Also, this should probably be an
> ASSERT_UNUSED().

Thanks. Will fix. And also, thanks for the tip about iOS-specific expect file.

[John Wilander]

Created attachment 360790 [details]
Patch

[Daniel Bates]

Comment on attachment 360790 [details]
Patch

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

I’m done too many things found so far...

> Source/WebCore/html/HTMLAnchorElement.cpp:408
> +        return WTF::nullopt;

Really? Just return? Makes sense when both are null, but why *no* warning when both empty? When both empty your warning below can be used as well.

> Source/WebCore/html/HTMLAnchorElement.cpp:411
> +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "Both the campaign ID attribute and the destination attribute need to be set for Ad Click Attribution to work.");

MessageSource::Storage? Nothing better?

> Source/WebCore/html/HTMLAnchorElement.cpp:412
> +        return WTF::nullopt;

Developer has to turn English to HTML attrivute namws to understand this message. Why the hoops? Why not write in terms of the attributes *not* there English description? Or is there precedent for this kind of obscurity.

> Source/WebCore/html/HTMLAnchorElement.cpp:417
> +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, convertedAdCampaignID.releaseException().message());

Storage? You do this several times in this patch.

> Source/WebCore/html/HTMLAnchorElement.cpp:424
> +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "The destination attribute for Ad Click Attribution could not be converted to a valid HTTP-family URL.");

Same comments  about obscurity in wording as above.

> Source/WebCore/html/HTMLAnchorElement.cpp:429
> +    if (!frame || !frame->isMainFrame()) {

Wow, we do so much work up to this point only to bail out if we don’t have a frame or we are a subframe. Why not do these checks first?

> Source/WebCore/html/HTMLAnchorElement.cpp:434
> +    return AdClickAttribution(AdClickAttribution::Campaign(adCampaignID), AdClickAttribution::Source(document().domain()), AdClickAttribution::Destination(adDestinationURL.host().toString()));

What a mouthful. Clean this up by using “using” inside this function to avoid the class qualification. Uniform intializer syntax please.

> Source/WebCore/html/HTMLAnchorElement.cpp:482
> +    // FIXME: rdar://problem/47650118

Style for this comment does not look right. grep for other comments that reference a bug. I think we usually put the bug URL at the end of the comment.maybe even write “See ...”.

> Source/WebCore/html/HTMLAnchorElement.cpp:484
> +    ASSERT_UNUSED(adClickAttribution);

Why are we adding this dead code? Defer to another patch?

> LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:22
> +Link1

This is printed AFTER “TEST COMPLETE”.

[John Wilander]

(In reply to Daniel Bates from comment #13)
> Comment on attachment 360790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360790&action=review
> 
> I’m done too many things found so far...
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:408
> > +        return WTF::nullopt;
> 
> Really? Just return? Makes sense when both are null, but why *no* warning
> when both empty? When both empty your warning below can be used as well.

I was trying to handle the no attributes case. Switched to a conditional based on hasAttributeWithoutSynchronization().

> > Source/WebCore/html/HTMLAnchorElement.cpp:411
> > +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "Both the campaign ID attribute and the destination attribute need to be set for Ad Click Attribution to work.");
> 
> MessageSource::Storage? Nothing better?

We've got:
    XML,
    JS,
    Network,
    ConsoleAPI,
    Storage,
    AppCache,
    Rendering,
    CSS,
    Security,
    ContentBlocker,
    Other,
    Media,
    WebRTC

My thinking was that the page is trying to store ad click attribution information. What would you pick? Add a new category?

> > Source/WebCore/html/HTMLAnchorElement.cpp:412
> > +        return WTF::nullopt;
> 
> Developer has to turn English to HTML attrivute namws to understand this
> message. Why the hoops? Why not write in terms of the attributes *not* there
> English description? Or is there precedent for this kind of obscurity.

I did it for the sake of readability since the attributes are all lower case. But I'll change it to the real attributes.

> > Source/WebCore/html/HTMLAnchorElement.cpp:417
> > +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, convertedAdCampaignID.releaseException().message());
> 
> Storage? You do this several times in this patch.

See above.

> > Source/WebCore/html/HTMLAnchorElement.cpp:424
> > +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "The destination attribute for Ad Click Attribution could not be converted to a valid HTTP-family URL.");
> 
> Same comments  about obscurity in wording as above.

See above.

> > Source/WebCore/html/HTMLAnchorElement.cpp:429
> > +    if (!frame || !frame->isMainFrame()) {
> 
> Wow, we do so much work up to this point only to bail out if we don’t have a
> frame or we are a subframe. Why not do these checks first?

Sure. I would think it's the rare case but I'll pull it up.

> > Source/WebCore/html/HTMLAnchorElement.cpp:434
> > +    return AdClickAttribution(AdClickAttribution::Campaign(adCampaignID), AdClickAttribution::Source(document().domain()), AdClickAttribution::Destination(adDestinationURL.host().toString()));
> 
> What a mouthful. Clean this up by using “using” inside this function to
> avoid the class qualification. Uniform intializer syntax please.

Will fix.

> > Source/WebCore/html/HTMLAnchorElement.cpp:482
> > +    // FIXME: rdar://problem/47650118
> 
> Style for this comment does not look right. grep for other comments that
> reference a bug. I think we usually put the bug URL at the end of the
> comment.maybe even write “See ...”.

Will fix.

> > Source/WebCore/html/HTMLAnchorElement.cpp:484
> > +    ASSERT_UNUSED(adClickAttribution);
> 
> Why are we adding this dead code? Defer to another patch?

This is what the FIXME is about. The next patch will use the adClickAttribution object and pass it on to the loader.

> > LayoutTests/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:22
> > +Link1
> 
> This is printed AFTER “TEST COMPLETE”.

These are innerText labels for the rendered links. I've created them that way to make them have a width to target with the click. Should I do it some other way?

[John Wilander]

Created attachment 360803 [details]
Patch

[John Wilander]

I don't know what's up with the Mac bots and the text-diff failures. Will have to wait for the archives.

[EWS Watchlist]

Comment on attachment 360803 [details]
Patch

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

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-validation.html

[EWS Watchlist]

Created attachment 360816 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360803 [details]
Patch

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

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-validation.html

[EWS Watchlist]

Created attachment 360820 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Daniel Bates]

Comment on attachment 360803 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:409
> +    if (!frame || !frame->isMainFrame()) {

C’mon. These checks are now too early. Think when does this function get called and when it makes sense to show a message.

> Source/WebCore/html/HTMLAnchorElement.cpp:485
> +    auto adClickAttribution = parseAdClickAttribution();

I don’t think this is the right division of labor.

> Source/WebCore/html/HTMLAnchorElement.cpp:486
> +    // FIXME: The adClickAttribution should be forwarded to the loader and handled down the pipe.

Why do you make every engineer building debug pay for something that is not being used? Each day that goes by with this unfixed FIXME wastes engineers time when they read through  this dead code. I have seen my fair share of FIXMEs that never are fixed, but I can’t recall ever seeing a FIXME *AND* accompanying dead code. It’s sloppy, but if it makes you feel good then keep it. The FIXME seems sufficient.

> Source/WebCore/html/HTMLAnchorElement.cpp:487
> +    // See rdar://problem/47650118

Great! Thanks for the reference, but excessive to put this on its own line. Just amend to the previous line and wrap the entire comment so it looks nice. Take a look at other comments in the code if non-obvious.

> Source/WebCore/loader/AdClickAttribution.cpp:98
> +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains too many characters to be converted to a value allowed for Ad Click Attribution.") };

First makeString arg should be a character *not* a string.

> Source/WebCore/loader/AdClickAttribution.cpp:101
> +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains too few characters to be converted to a value allowed for Ad Click Attribution.") };

Ditto. I don’t feel that it is necessary to have two error messages for < 2 and > 2. I also think it’s a tease to a person. Like “haha you messed up, too few, too many, but i’m not going to tell you how many I expect” kind of tease.I think with the right wording, e.g. “exactly two” it will be clear to anyone the mistake they made, how to fix it, and we can use one message for both cases. But you do as you like.

> Source/WebCore/loader/AdClickAttribution.cpp:103
> +    if (!isASCIIDigit(toConvert.characterAt(0)) || !isASCIIDigit(toConvert.characterAt(1)))

Why the characterAt()s()? You already ensured the length! I’m stopping here.... too many issues so far.

> Source/WebCore/loader/AdClickAttribution.cpp:104
> +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains at least one non-digit character and can not be converted to a value allowed for Ad Click Attribution.") };

First makeString arg sub-optimal. See above comment.

> LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:16
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

What kind of test is this? Where are the series of PASS messages? This test is not a good PASS/FAIL test. Grep some other PASS/FAIL tests in the tree to get ideas or talk with me in person tomorrow.

> LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:22
> +Link1

This is sloppy compared to other PASS/FAIL tests. Hide this output. Make the output of a test logical. This output is illogical. It’s like when the professor saids “test complete, pencils down” and you keep writing kind of logical.

[Daniel Bates]

Comment on attachment 360803 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:414
> +    if (!hasAttributeWithoutSynchronization(adcampaignidAttr) && !hasAttributeWithoutSynchronization(addestinationAttr))

I can’t help myself to comment... I’m unclear if has() is a fast path. If it is then this code is fine. If not, then I would think calling attributeWithoutSynch...() storing that it a local L and calling L.isNull() would be equivalent and more efficient.

[Daniel Bates]

Comment on attachment 360803 [details]
Patch

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

>> Source/WebCore/html/HTMLAnchorElement.cpp:485
>> +    auto adClickAttribution = parseAdClickAttribution();
> 
> I don’t think this is the right division of labor.

Just when I think I am getting out this patch pulls me back in... something bothers me about the placement of this code.  We do not typically parse attributes in event handlers and doing so delays when the error message is reported. This does not seem correct. Why is this correct?

> LayoutTests/ChangeLog:1
> [phone]  John Wilander  <[e-mail]>

Double change log.

[John Wilander]

Thanks for your comments, Dan! I will look into them later today.

You don’t have to be polite, but could you please stop with the “wow”s, “really?”s, and “c’mon”s? It’s annoying and unprofessional, at least for me. Let’s just focus on the technical stuff. Thanks.

Regarding the placement of the code, I need to validate the input before I send it to the loader and further on to the network process. Would you rather do this validation in a setter and store the result? Will that risk a race condition? By doing it in the event handler, I know that any data I pass on to the loader is validated.

[Daniel Bates]

(In reply to John Wilander from comment #24)
> Thanks for your comments, Dan! I will look into them later today.
> 
> You don’t have to be polite, but could you please stop with the “wow”s,
> “really?”s, and “c’mon”s? It’s annoying and unprofessional, at least for me.
> Let’s just focus on the technical stuff. Thanks.
> 

I apologize. I was going for more of a free-style, casual tone. Doesn’t work over text. It will never happen again.

[John Wilander]

(In reply to Daniel Bates from comment #21)
> Comment on attachment 360803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360803&action=review
> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:409
> > +    if (!frame || !frame->isMainFrame()) {
> 
> C’mon. These checks are now too early. Think when does this function get
> called and when it makes sense to show a message.

I'll place it before the call to AdClickAttribution::convertToMaxEntropyValue().

> > Source/WebCore/html/HTMLAnchorElement.cpp:485
> > +    auto adClickAttribution = parseAdClickAttribution();
> 
> I don’t think this is the right division of labor.

Any suggestions? See my earlier question with my line of thought.

> > Source/WebCore/html/HTMLAnchorElement.cpp:486
> > +    // FIXME: The adClickAttribution should be forwarded to the loader and handled down the pipe.
> 
> Why do you make every engineer building debug pay for something that is not
> being used? Each day that goes by with this unfixed FIXME wastes engineers
> time when they read through  this dead code. I have seen my fair share of
> FIXMEs that never are fixed, but I can’t recall ever seeing a FIXME *AND*
> accompanying dead code. It’s sloppy, but if it makes you feel good then keep
> it. The FIXME seems sufficient.

The variable is currently unused. That's what the FIXME is for. The assert is still valid since I want to make sure, and signal to other WebKit engineers, that the AdClickAttribution object is not complete at this point.

> > Source/WebCore/html/HTMLAnchorElement.cpp:487
> > +    // See rdar://problem/47650118
> 
> Great! Thanks for the reference, but excessive to put this on its own line.
> Just amend to the previous line and wrap the entire comment so it looks
> nice. Take a look at other comments in the code if non-obvious.
> 
> > Source/WebCore/loader/AdClickAttribution.cpp:98
> > +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains too many characters to be converted to a value allowed for Ad Click Attribution.") };
> 
> First makeString arg should be a character *not* a string.

Will fix.

> > Source/WebCore/loader/AdClickAttribution.cpp:101
> > +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains too few characters to be converted to a value allowed for Ad Click Attribution.") };
> 
> Ditto. I don’t feel that it is necessary to have two error messages for < 2
> and > 2. I also think it’s a tease to a person. Like “haha you messed up,
> too few, too many, but i’m not going to tell you how many I expect” kind of
> tease.I think with the right wording, e.g. “exactly two” it will be clear to
> anyone the mistake they made, how to fix it, and we can use one message for
> both cases. But you do as you like.

Good suggestion. I'll make it one message saying the attribute needs exactly two digits.

> > Source/WebCore/loader/AdClickAttribution.cpp:103
> > +    if (!isASCIIDigit(toConvert.characterAt(0)) || !isASCIIDigit(toConvert.characterAt(1)))
> 
> Why the characterAt()s()? You already ensured the length! I’m stopping
> here.... too many issues so far.

Is there a more convenient or efficient way to do this? Yes, I've checked that the string has exactly two characters, which I believe is a prerequisite for accessing characters at position 0 and 1 directly. Then I check that both characters are digits.

> > Source/WebCore/loader/AdClickAttribution.cpp:104
> > +        return Exception { ConstraintError, makeString("\"", toConvert, "\" contains at least one non-digit character and can not be converted to a value allowed for Ad Click Attribution.") };
> 
> First makeString arg sub-optimal. See above comment.

Will fix.

> > LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:16
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> What kind of test is this? Where are the series of PASS messages? This test
> is not a good PASS/FAIL test. Grep some other PASS/FAIL tests in the tree to
> get ideas or talk with me in person tomorrow.

This is a test of the developer feedback in the form of console warnings. It makes sure input validation works.

> > LayoutTests/platform/ios-wk2/http/tests/adClickAttribution/anchor-tag-attributes-validation-expected.txt:22
> > +Link1
> 
> This is sloppy compared to other PASS/FAIL tests. Hide this output. Make the
> output of a test logical. This output is illogical. It’s like when the
> professor saids “test complete, pencils down” and you keep writing kind of
> logical.

OK, I'll look for a way to suppress the output.

[John Wilander]

Created attachment 360894 [details]
Patch

[John Wilander]

I couldn't find a TestRunner function to suppress the link output. I tried appending the link elements to a div inside the body, above the script tag, but the output still appears after "TEST COMPLETE." I also tried hiding the div which did suppress the link output but also broke the UI Helper's clicks.

[John Wilander]

Created attachment 360900 [details]
Patch

[John Wilander]

Chris was kind enough to show how I could get the link output above the test output. Should be find now.

[John Wilander]

Created attachment 360904 [details]
Patch

[Chris Dumez]

Comment on attachment 360900 [details]
Patch

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

> Source/WebCore/loader/AdClickAttribution.cpp:97
> +    if (toConvert.length() != 2)

We normally use the methods in HTMLParserIdioms.h to parse attributes as per the HTML specification, e.g. you may want parseValidHTMLNonNegativeInteger(), or clampHTMLNonNegativeIntegerToRange().
HTMLTableCellElement::colSpan() could be one example. Note that the validation could also be reflected via IDL reflection. There are tons on HTML tests settings an attribute to a given value and then querying the corresponding IDL value to determine what was actually parsed.

[Chris Dumez]

Comment on attachment 360904 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:415
> +        document().addConsoleMessage(MessageSource::Storage, MessageLevel::Warning, "Both the adcampaignid attribute and the addestination attribute need to be set for Ad Click Attribution to work.");

It's not really "Storage", I would prefer "Other". Also, your String literals passed as last parameter to addConsoleMessage() should have _s prefix (This is the modern ASCIILiteral()).
Ditto below.

> Source/WebCore/html/HTMLAnchorElement.cpp:488
> +    ASSERT_UNUSED(adClickAttribution, !adClickAttribution || adClickAttribution->url().string().isEmpty());

adClickAttribution->url().isNull() ?

[Chris Dumez]

Comment on attachment 360904 [details]
Patch

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

r=me with suggestions.

> Source/WebCore/loader/AdClickAttribution.cpp:97
> +    if (toConvert.length() != 2)

Basically, here I'd do something like:
auto parsed = parseHTMLNonNegativeInteger(toConvert);
if (!parsed)
    return Exception { ConstraintError, makeString('\"', toConvert, "\" can not be converted to an unsigned integer which is required for Ad Click Attribution.") };

if (parsed.value() >= maxEntropy)
        return Exception { ConstraintError, makeString('\"', toConvert, "\" is greater than or equal to ", String::number(maxEntropy), " and is not allowed for Ad Click Attribution.") };

return parsed.value();


Code would be shorter and we'd parse the number following the rules defined in the HTML spec.

[John Wilander]

Thanks, Chris! I will take a stab at these suggestions before landing.

[Darin Adler]

Comment on attachment 360904 [details]
Patch

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

>> Source/WebCore/loader/AdClickAttribution.cpp:97
>> +    if (toConvert.length() != 2)
> 
> Basically, here I'd do something like:
> auto parsed = parseHTMLNonNegativeInteger(toConvert);
> if (!parsed)
>     return Exception { ConstraintError, makeString('\"', toConvert, "\" can not be converted to an unsigned integer which is required for Ad Click Attribution.") };
> 
> if (parsed.value() >= maxEntropy)
>         return Exception { ConstraintError, makeString('\"', toConvert, "\" is greater than or equal to ", String::number(maxEntropy), " and is not allowed for Ad Click Attribution.") };
> 
> return parsed.value();
> 
> 
> Code would be shorter and we'd parse the number following the rules defined in the HTML spec.

Can use '"' rather than '\"'.

Should not use String::number; instead just list the number and makeString will convert it to a string, more efficiently than String::number, as long as we make sure we include StringConcatenateNumbers.h.

[Daniel Bates]

Comment on attachment 360904 [details]
Patch

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

In all the message we refer to this feature as a proper noun. Is that correct?

> Source/WebCore/html/HTMLAnchorElement.cpp:485
> +    auto adClickAttribution = parseAdClickAttribution();

I still think this is the wrong place to do this. No one will see the error messages because clicking a hyperlink navigates. I am exaggerating...only those developers that create a dummy hyperlink like in your test or preventDefault() will see your message, but that may be the audience you intended. Are those the developers you are targeting? Personally, I think that audience is so small It's not worth acknowledging. Go for the majority audience in my opinion.

> Source/WebCore/loader/AdClickAttribution.cpp:95
> +ExceptionOr<unsigned short> AdClickAttribution::convertToMaxEntropyValue(const String& toConvert)

Name of this function feels off. Doesn't seem like it converts to max entropy value. It can convert to other values. Reads like it converts to the same value every time.

> Source/WebCore/loader/AdClickAttribution.cpp:100
> +    if (!isASCIIDigit(toConvert.characterAt(0)) || !isASCIIDigit(toConvert.characterAt(1)))

operator[] should be more efficient last I recall. It's only safe to use if you bounds check beforehand, which you did.

> Source/WebCore/loader/AdClickAttribution.cpp:101
> +        return Exception { ConstraintError, makeString('\"', toConvert, "\" contains at least one non-digit character and can not be converted to a value allowed for Ad Click Attribution.") };

This message sounds robotic. Maybe: <attribute> must only contain digits for Ad Click Attribution.

> Source/WebCore/loader/AdClickAttribution.cpp:106
> +        return Exception { ConstraintError, makeString('\"', toConvert, "\" can not be converted to an unsigned integer which is required for Ad Click Attribution.") };

This message reads like code and sounds robotic or like a compiler. I don't think JavaScript dev think in terms of unsigned integers. I don't think that is part of the lingo last I was in that space (typed arrays were nascent). The term "non-negative integer" may be more understandable and is how the HTML spec describes this. Maybe a more down to earth message would be: <attribute> must have a non-negative integer value.

> Source/WebCore/loader/AdClickAttribution.cpp:108
> +    unsigned short result = static_cast<unsigned short>(unsignedResult);

Cast seems unnecessary. Just change auto above to unsigned short as you clearly do not want auto and remove this line. Nothing more fine grained that toUIntStrict(), right? Like no toUShortIntStrict() or something?

> Source/WebCore/loader/AdClickAttribution.cpp:110
> +        return Exception { ConstraintError, makeString('\"', toConvert, "\" is greater than or equal to ", String::number(maxEntropy), " and is not allowed for Ad Click Attribution.") };

This message reads like code. Something less robotic would be better. Take advantage of the fact that line numbers are part of the error message to avoid hinting what line the bug is on. If the caller told you the attribute then you could emit something like: <attribute> must have a value less than <maxEntropy> for Ad Click Attribution.

The dev. can infer that there value must be greater or equal to <maxEntropy> from that message.

> Source/WebCore/loader/AdClickAttribution.h:127
> +    static ExceptionOr<unsigned short> convertToMaxEntropyValue(const String& toConvert);

Remove param name.

[Darin Adler]

Do we really need to use "unsigned short"? The reason I ask is that it’s a poorly supported type since sometimes it’s the same type as "UChar" and sometimes not. We use it for port numbers, but it would be best not to use it for anything new unless there’s a compelling reason that we we need to use that type. Could we just use 32-bit unsigned instead? Worth considering.

[John Wilander]

Created attachment 361029 [details]
Patch

[John Wilander]

I've worked through all of your comments Chris, Dan, and Darin. As for the proper noun in the console messages, yes, it's intentional.

Running a pass on EWS to make sure everything is green.

[John Wilander]

And of course, thanks for all the comments!

[John Wilander]

Dan, Chris and I discussed the right time to do the parsing and decided to go the handler path since there may be many links with ad attribution information to parse but only a few that the user will actually click, often just one that navigates elsewhere. By not parsing all of them up front we avoid the performance cost.

[WebKit Commit Bot]

Comment on attachment 361029 [details]
Patch

Rejecting attachment 361029 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 361029, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/11019064

[John Wilander]

Missed reviewer(s) in LayoutTests/Changelog. Will fix.

[John Wilander]

Created attachment 361031 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 361031 [details]
Patch for landing

Clearing flags on attachment: 361031

Committed r240911: <https://trac.webkit.org/changeset/240911>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to John Wilander from comment #42)
> Dan, Chris and I discussed the right time to do the parsing and decided to
> go the handler path since there may be many links with ad attribution
> information to parse but only a few that the user will actually click, often
> just one that navigates elsewhere. By not parsing all of them up front we
> avoid the performance cost.

Ok. But then web dev’s won’t see your error messsage. So, what’s the point of the messages? Just so you can write a test? If so, that is silly as a unit test would suffice.

[John Wilander]

(In reply to Daniel Bates from comment #48)
> (In reply to John Wilander from comment #42)
> > Dan, Chris and I discussed the right time to do the parsing and decided to
> > go the handler path since there may be many links with ad attribution
> > information to parse but only a few that the user will actually click, often
> > just one that navigates elsewhere. By not parsing all of them up front we
> > avoid the performance cost.
> 
> Ok. But then web dev’s won’t see your error messsage. So, what’s the point
> of the messages? Just so you can write a test? If so, that is silly as a
> unit test would suffice.

I had a chat with Joe about this when developing the patch and he showed me the “Preserve Log” feature in Web Inspector which works great for this. That is something we can explain in a blog post for instance.

[Daniel Bates]

(In reply to John Wilander from comment #42)
> Dan, Chris and I discussed the right time to do the parsing and decided to
> go the handler path since there may be many links with ad attribution
> information to parse but only a few that the user will actually click, often
> just one that navigates elsewhere. By not parsing all of them up front we
> avoid the performance cost.

Not a deal breaking the way it landed. I don't believe this line of reasoning as I don't think this code is that hot. This is a likely a hot code path, but I don't think it will be as hot as you guys claim. Profile or the problem doesn't exist. If it is hot then I am sure we can find a solution just like the test problem of emitting text after TEST COMPLETE.

[Daniel Bates]

(In reply to John Wilander from comment #49)
> (In reply to Daniel Bates from comment #48)
> > (In reply to John Wilander from comment #42)
> > > Dan, Chris and I discussed the right time to do the parsing and decided to
> > > go the handler path since there may be many links with ad attribution
> > > information to parse but only a few that the user will actually click, often
> > > just one that navigates elsewhere. By not parsing all of them up front we
> > > avoid the performance cost.
> > 
> > Ok. But then web dev’s won’t see your error messsage. So, what’s the point
> > of the messages? Just so you can write a test? If so, that is silly as a
> > unit test would suffice.
> 
> I had a chat with Joe about this when developing the patch and he showed me
> the “Preserve Log” feature in Web Inspector which works great for this. That
> is something we can explain in a blog post for instance.

Okay :/ Any precedent from this kinda of debugging requirement to web devs? Does this preserve messages from subframes? If not, then your "not main frame" message will never be seen.