Adopt URLUtils interface and template in HTMLAnchorElement and HTMLAreaElement
Created attachment 222913 [details] Patch
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.
Comment on attachment 222913 [details] Patch Unflagging; posted for EWS purposes only.
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
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
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
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
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
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
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.
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.
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?
(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).
(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.
Committed r163299: <http://trac.webkit.org/changeset/163299>
(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.
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
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')
(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
Re-opening due to the rollout.
This has subsequently been done in a different, likely more modern way (as HTMLHyperlinkElementUtils).