Bug 119706

Summary: Move id attribute to parent Element interface
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, cfleizach, cmarcelo, commit-queue, darin, d-r, eoconnor, esprehn+autocc, fmalita, kangil.han, kling, koivisto, kondapallykalyan, laszlo.gombos, pdr, rniwa, schenney, syoichi, timothy
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
rniwa: review+, buildbot: commit-queue-
Attempt to fix mac build
none
Update Changelog.
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Patch
none
Patch
rniwa: review+, commit-queue: commit-queue-
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 none

Description Ryosuke Niwa 2013-08-12 17:17:50 PDT
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).
Comment 1 Chris Dumez 2013-08-13 00:15:54 PDT
Created attachment 208602 [details]
Patch
Comment 2 Ryosuke Niwa 2013-08-13 00:18:09 PDT
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 3 Chris Dumez 2013-08-13 00:20:46 PDT
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 4 Build Bot 2013-08-13 00:28:00 PDT
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
Comment 5 Chris Dumez 2013-08-13 00:39:38 PDT
Created attachment 208604 [details]
Attempt to fix mac build
Comment 6 Ryosuke Niwa 2013-08-13 00:42:09 PDT
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.
Comment 7 Chris Dumez 2013-08-13 00:50:16 PDT
Created attachment 208606 [details]
Update Changelog.
Comment 8 WebKit Commit Bot 2013-08-13 00:53:03 PDT
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 9 Build Bot 2013-08-13 07:57:17 PDT
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
Comment 10 Build Bot 2013-08-13 07:57:20 PDT
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
Comment 11 Chris Dumez 2013-08-13 08:05:57 PDT
Created attachment 208633 [details]
Patch
Comment 12 Chris Dumez 2013-08-13 08:06:29 PDT
Requesting review again due to the ObjC bindings change.
Comment 13 Darin Adler 2013-08-13 08:55:04 PDT
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.
Comment 14 Darin Adler 2013-08-13 08:56:36 PDT
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.
Comment 15 Chris Dumez 2013-08-13 09:48:21 PDT
(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.
Comment 16 Chris Dumez 2013-08-13 10:00:21 PDT
Created attachment 208646 [details]
Patch
Comment 17 Build Bot 2013-08-13 10:46:35 PDT
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
Comment 18 Build Bot 2013-08-13 10:46:39 PDT
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
Comment 19 Chris Dumez 2013-08-13 10:50:17 PDT
(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.
Comment 20 chris fleizach 2013-08-13 10:52:33 PDT
(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.
Comment 21 Chris Dumez 2013-08-13 10:54:23 PDT
(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 22 Darin Adler 2013-08-13 11:01:16 PDT
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.
Comment 23 Chris Dumez 2013-08-13 11:11:12 PDT
(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.
Comment 24 Darin Adler 2013-08-13 12:11:39 PDT
(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 25 Build Bot 2013-08-13 23:27:42 PDT
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
Comment 26 Build Bot 2013-08-13 23:27:45 PDT
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 27 WebKit Commit Bot 2013-08-14 06:17:45 PDT
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 28 WebKit Commit Bot 2013-08-14 10:27:07 PDT
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
Comment 29 Chris Dumez 2013-08-14 10:41:11 PDT
Committed r154057: <http://trac.webkit.org/changeset/154057>
Comment 30 Alexey Proskuryakov 2013-08-14 15:37:33 PDT
> New failing tests:
> platform/mac/accessibility/role-subrole-roledescription.html

Looks like this was accurate, this is failing now all over the place.
Comment 31 chris fleizach 2013-08-14 15:41:50 PDT
(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
Comment 32 Alexey Proskuryakov 2013-08-14 15:48:52 PDT
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?
Comment 33 Chris Dumez 2013-08-15 00:11:52 PDT
(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