Bug 128067 – Adopt URLUtils interface and template in HTMLAnchorElement and HTMLAreaElement

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: New Bugs

[Maciej Stachowiak]

Adopt URLUtils interface and template in HTMLAnchorElement and HTMLAreaElement

[Maciej Stachowiak]

Created attachment 222913 [details]
Patch

[WebKit Commit Bot]

Attachment 222913 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 222913 [details]
Patch

Unflagging; posted for EWS purposes only.

[Build Bot]

Comment on attachment 222913 [details]
Patch

Attachment 222913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5656720835608576

New failing tests:
fast/dom/element-attribute-js-null.html
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 222916 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 222913 [details]
Patch

Attachment 222913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5938195812319232

New failing tests:
fast/dom/element-attribute-js-null.html
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 222917 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

[Build Bot]

Comment on attachment 222913 [details]
Patch

Attachment 222913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6223781072732160

New failing tests:
fast/dom/element-attribute-js-null.html
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 222920 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Maciej Stachowiak]

Created attachment 222929 [details]
Patch

Comment on attachment 222929 [details]
Patch

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

Looks OK, but still doesn’t build on Windows.

> Source/WebCore/html/HTMLAnchorElement.h:66
>      void setHref(const AtomicString&);
> +    void setHref(const AtomicString& value, ExceptionCode&) { setHref(value); }

Is it valid to call this in a manner that throws an exception? If not, shouldn’t we just use the ASSERT_NO_EXCEPTION mechanism?

> LayoutTests/js/dom/dom-static-property-for-in-iteration.html:9
> +    <div id="console"></div>

Did you intend to make this change? It seems like this is responsible for many of the changes to the result above.

[Antti Koivisto]

Comment on attachment 222929 [details]
Patch

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

Looks good except for the strange test change.

>> Source/WebCore/html/HTMLAnchorElement.h:66
>> +    void setHref(const AtomicString& value, ExceptionCode&) { setHref(value); }
> 
> Is it valid to call this in a manner that throws an exception? If not, shouldn’t we just use the ASSERT_NO_EXCEPTION mechanism?

If argument-free version is for internal use only and not part of the DOM then it is much better to keep it separate rather than uglify our code with ASSERT_NO_EXCEPTION.

[Darin Adler]

Comment on attachment 222929 [details]
Patch

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

>>> Source/WebCore/html/HTMLAnchorElement.h:66
>>> +    void setHref(const AtomicString& value, ExceptionCode&) { setHref(value); }
>> 
>> Is it valid to call this in a manner that throws an exception? If not, shouldn’t we just use the ASSERT_NO_EXCEPTION mechanism?
> 
> If argument-free version is for internal use only and not part of the DOM then it is much better to keep it separate rather than uglify our code with ASSERT_NO_EXCEPTION.

Normally, I would say that I think the right fix here is to change the IDL file, since setting href can’t raise an exception.

But HTMLAnchorElement’s IDL file doesn’t create any calls to setHref, because href is a “reflect” attribute, so it calls setURLAttribute directly. Thus I don’t understand why this is needed at all? Where is the call site?

[Maciej Stachowiak]

(In reply to comment #11)
> (From update of attachment 222929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222929&action=review
> 
> Looks OK, but still doesn’t build on Windows.
> 
> > Source/WebCore/html/HTMLAnchorElement.h:66
> >      void setHref(const AtomicString&);
> > +    void setHref(const AtomicString& value, ExceptionCode&) { setHref(value); }
> 
> Is it valid to call this in a manner that throws an exception? If not, shouldn’t we just use the ASSERT_NO_EXCEPTION mechanism?

Not sure what you mean. The exception code is passed in, not out. This method will never throw an exception itself. The exception argument is only because the href setter is declared as possibly throwing in URLUtils.idl, but the HTMLAnchorElement version will never actually throw.

> 
> > LayoutTests/js/dom/dom-static-property-for-in-iteration.html:9
> > +    <div id="console"></div>
> 
> Did you intend to make this change? It seems like this is responsible for many of the changes to the result above.

Yes. I wanted to move the link above the output, so that its position would not change solely due to a new property being added (since its hard to tell if the new offset is correct).

[Maciej Stachowiak]

(In reply to comment #13)
> (From update of attachment 222929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222929&action=review
> 
> >>> Source/WebCore/html/HTMLAnchorElement.h:66
> >>> +    void setHref(const AtomicString& value, ExceptionCode&) { setHref(value); }
> >> 
> >> Is it valid to call this in a manner that throws an exception? If not, shouldn’t we just use the ASSERT_NO_EXCEPTION mechanism?
> > 
> > If argument-free version is for internal use only and not part of the DOM then it is much better to keep it separate rather than uglify our code with ASSERT_NO_EXCEPTION.
> 
> Normally, I would say that I think the right fix here is to change the IDL file, since setting href can’t raise an exception.
> 
> But HTMLAnchorElement’s IDL file doesn’t create any calls to setHref, because href is a “reflect” attribute, so it calls setURLAttribute directly. Thus I don’t understand why this is needed at all? Where is the call site?

HTMLAnchorElement now no longer defines href itself; instead it is defined in URLUtils.idl. Officially per the URL and HTML specs, HTMLAnchorElement is now supposed to get its href by implementing URLUtils rather than directly. This also means that it can't be a "reflect" attribute now, because URLUtil is also used by interfaces that are not elements. Specifically by DOMURL, which can throw from the href setter, which is why it has the possibility of exceptions which don't actually apply here.

[Maciej Stachowiak]

Committed r163299: <http://trac.webkit.org/changeset/163299>

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed r163299: <http://trac.webkit.org/changeset/163299>

FYI: It broke the Apple Windows build, as the EWS noticed it before landing.

[Csaba Osztrogonác]

And it broke the build on the 32 bit Apple Mountain Lion bot too:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%2832-bit%20Build%29/builds/8846

[Jessie Berlin]

I am going to roll out this patch, since it broke a number of builds. Output for the ML 32-bit release build:

/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:248:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:267:30: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:287:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:248:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:267:30: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
/Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:287:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')

[Jessie Berlin]

(In reply to comment #19)
> I am going to roll out this patch, since it broke a number of builds. Output for the ML 32-bit release build:
> 
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:248:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:267:30: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:287:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:248:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:267:30: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')
> /Volumes/Data/slave/mountainlion-32bit-release/build/Source/WebCore/html/URLUtils.h:287:14: error: use of overloaded operator '[]' is ambiguous (with operand types 'const WTF::String' and 'int')

Rollout done in http://trac.webkit.org/changeset/163309

[Jessie Berlin]

Re-opening due to the rollout.

[Maciej Stachowiak]

This has subsequently been done in a different, likely more modern way (as HTMLHyperlinkElementUtils).