Merge https://chromium.googlesource.com/chromium/blink/+/048383b5f7c71faffb20202c64e389ac23d2e955 Move id attribute from SVGElement / HTMLElement to their Element parent interface to match the latest DOM specification and avoid duplication: http://dom.spec.whatwg.org/#dom-element-id This behavior is consistent with Firefox. There is no web-exposed behavior change because we haven't moved properties from instances to their prototype, as per the Web IDL specification (http://dev.w3.org/2006/webapi/WebIDL/#es-attributes).
Created attachment 208602 [details] Patch
Comment on attachment 208602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review > Source/WebCore/dom/Element.idl:67 > + // iht.com relies on id returning the empty string when no id is present. > + // Other browsers do this as well. So we don't convert null to JS null. I don't think this comment belongs in the code since it can get outdated. It's better to mention it in the change log since this statement is (supposedly) true as of today.
Comment on attachment 208602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review >> Source/WebCore/dom/Element.idl:67 >> + // Other browsers do this as well. So we don't convert null to JS null. > > I don't think this comment belongs in the code since it can get outdated. > It's better to mention it in the change log since this statement is (supposedly) true as of today. I merely moved the comment over from HTMLElement. Since it is not new, I don't think I should mention this in my Changelog. Do you propose we remove the comment altogether?
Comment on attachment 208602 [details] Patch Attachment 208602 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1381429
Created attachment 208604 [details] Attempt to fix mac build
Comment on attachment 208602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208602&action=review >>> Source/WebCore/dom/Element.idl:67 >>> + // Other browsers do this as well. So we don't convert null to JS null. >> >> I don't think this comment belongs in the code since it can get outdated. >> It's better to mention it in the change log since this statement is (supposedly) true as of today. > > I merely moved the comment over from HTMLElement. Since it is not new, I don't think I should mention this in my Changelog. > Do you propose we remove the comment altogether? This comment was added in http://trac.webkit.org/changeset/4417 10 years ago. The compatibility with ich.com, which now redirects to http://global.nytimes.com/?iht, is hardly relevant at this point. My preference will be to remove this comment since the current behavior is well specified in http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes.
Created attachment 208606 [details] Update Changelog.
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
Comment on attachment 208606 [details] Update Changelog. Attachment 208606 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1451463 New failing tests: platform/mac/accessibility/role-subrole-roledescription.html fast/js/dom-static-property-for-in-iteration.html
Created attachment 208631 [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.4
Created attachment 208633 [details] Patch
Requesting review again due to the ObjC bindings change.
Comment on attachment 208633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208633&action=review I really want to say review+, but the Objective-C aspect of this is wrong. We’ll probably have to leave something ugly around for Objective-C, at least for a while. > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:175 > +@property(copy) NSString *idName; This is not correct. This means that the Objective-C header file will have interface that claims you can call idName on a DOMElement in older versions of WebKit. But if a programmer tries it they will get method-not-found. Tim Hatcher should give us some advice on what to do about this. The problem, of course, is not in this header file alone, but generally in the generated code too.
The easiest way to make a land-able patch is to leave the old attribute in place in HTMLElement for Objective-C only, and have the new attribute in Element for all other languages. Then someone can follow up by doing something better for Objective-C. Too bad the Objective-C bindings are getting in the way.
(In reply to comment #14) > The easiest way to make a land-able patch is to leave the old attribute in place in HTMLElement for Objective-C only, and have the new attribute in Element for all other languages. Then someone can follow up by doing something better for Objective-C. Too bad the Objective-C bindings are getting in the way. Ok, this sounds like a plan, thanks. I'll do that and reupload soon.
Created attachment 208646 [details] Patch
Comment on attachment 208646 [details] Patch Attachment 208646 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1445785 New failing tests: platform/mac/accessibility/role-subrole-roledescription.html
Created attachment 208652 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.4
(In reply to comment #17) > (From update of attachment 208646 [details]) > Attachment 208646 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/1445785 > > New failing tests: > platform/mac/accessibility/role-subrole-roledescription.html Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail? The diff looks like: -PASS: math - AXRole: - AXSubrole: - AXRoleDescription: - +FAIL: math + AXRole: AXGroup + AXSubrole: AXDocumentMath + AXRoleDescription: math + Expected: // + I looked at the test but I don't understand it much and I don't have mac to run it.
(In reply to comment #19) > (In reply to comment #17) > > (From update of attachment 208646 [details] [details]) > > Attachment 208646 [details] [details] did not pass mac-ews (mac): > > Output: http://webkit-queues.appspot.com/results/1445785 > > > > New failing tests: > > platform/mac/accessibility/role-subrole-roledescription.html > > Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail? > > The diff looks like: > -PASS: math > - AXRole: > - AXSubrole: > - AXRoleDescription: > - > +FAIL: math > + AXRole: AXGroup > + AXSubrole: AXDocumentMath > + AXRoleDescription: math > + Expected: // > + > > I looked at the test but I don't understand it much and I don't have mac to run it. Must be a false positive. That's a strange one to fail out of the blue. It's hard for me to believe that moving the id interface around would have an effect like this. I'd be OK even if this was a real failure with your patch to fix it up after you land because clearly something is amiss if it persists.
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #17) > > > (From update of attachment 208646 [details] [details] [details]) > > > Attachment 208646 [details] [details] [details] did not pass mac-ews (mac): > > > Output: http://webkit-queues.appspot.com/results/1445785 > > > > > > New failing tests: > > > platform/mac/accessibility/role-subrole-roledescription.html > > > > Chris Fleizach, can this be a false positive? If not, do you have an idea of why it would fail? > > > > The diff looks like: > > -PASS: math > > - AXRole: > > - AXSubrole: > > - AXRoleDescription: > > - > > +FAIL: math > > + AXRole: AXGroup > > + AXSubrole: AXDocumentMath > > + AXRoleDescription: math > > + Expected: // > > + > > > > I looked at the test but I don't understand it much and I don't have mac to run it. > > Must be a false positive. That's a strange one to fail out of the blue. It's hard for me to believe that moving the id interface around would have an effect like this. I'd be OK even if this was a real failure with your patch to fix it up after you land because clearly something is amiss if it persists. Ok, thanks for checking.
Comment on attachment 208646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208646&action=review > Source/WebCore/svg/SVGElement.idl:-27 > - [Reflect] attribute DOMString id; May need to keep compiling this for Objective-C, otherwise apps calling the id method on SVGElement might fail, even though this is not technically a public ObJC DOM interface.
(In reply to comment #22) > (From update of attachment 208646 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208646&action=review > > > Source/WebCore/svg/SVGElement.idl:-27 > > - [Reflect] attribute DOMString id; > > May need to keep compiling this for Objective-C, otherwise apps calling the id method on SVGElement might fail, even though this is not technically a public ObJC DOM interface. I remember Xenon telling me you were not generating ObjC bindings for SVG anymore so I thought it was safe to remove. Please let me know if I misunderstood and if I should add it back.
(In reply to comment #23) > I remember Xenon telling me you were not generating ObjC bindings for SVG anymore so I thought it was safe to remove. Please let me know if I misunderstood and if I should add it back. Sounds right.
Comment on attachment 208646 [details] Patch Attachment 208646 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1451730 New failing tests: platform/mac/accessibility/role-subrole-roledescription.html
Created attachment 208701 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Comment on attachment 208646 [details] Patch Rejecting attachment 208646 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 0EC4.sh" (1 failure) Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65 e: *** wait: No child processes. Stop. make: *** Waiting for unfinished jobs.... make: *** wait: No child processes. Stop. Command /bin/sh failed with exit code 2 ** BUILD FAILED ** The following build commands failed: PhaseScriptExecution "Generate Derived Sources" "/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/Derived Sources.build/Script-C0CE72841247E66800BC0EC4.sh" (1 failure) Full output: http://webkit-queues.appspot.com/results/1462099
Comment on attachment 208646 [details] Patch Rejecting attachment 208646 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: sh" (1 failure) Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65 b make: *** wait: No child processes. Stop. make: *** Waiting for unfinished jobs.... make: *** wait: No child processes. Stop. Command /bin/sh failed with exit code 2 ** BUILD FAILED ** The following build commands failed: PhaseScriptExecution "Generate Derived Sources" "/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/Derived Sources.build/Script-C0CE72841247E66800BC0EC4.sh" (1 failure) Full output: http://webkit-queues.appspot.com/results/1458776
Committed r154057: <http://trac.webkit.org/changeset/154057>
> New failing tests: > platform/mac/accessibility/role-subrole-roledescription.html Looks like this was accurate, this is failing now all over the place.
(In reply to comment #30) > > New failing tests: > > platform/mac/accessibility/role-subrole-roledescription.html > > Looks like this was accurate, this is failing now all over the place. Filed https://bugs.webkit.org/show_bug.cgi?id=119821
This is failing on math, and this patch adds id to MathML elements, so it's not totally unrelated. Are they even supposed to have an id exposed?
(In reply to comment #32) > This is failing on math, and this patch adds id to MathML elements, so it's not totally unrelated. Are they even supposed to have an id exposed? According to http://www.w3.org/TR/mathml-for-css/#commatt, yes they are supposed to have an id attribute. In MathML2, they had an id as well: http://www.w3.org/TR/MathML2/appendixd.html#id.D.1.2