Closed Bug 957431 Opened 10 years ago Closed 10 years ago

Remove Attr.ownerElement

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox29 + wontfix
firefox30 + wontfix
firefox31 + wontfix

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

Remove support for Attr.ownerElement; r=bzbarsky
44.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Backout patch for branches
45.12 KB, patch
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Blink is considering removing this attribute, it would be nice if we did too:

<https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/ai6_ySyVITg>

Bug 661327 added deprecation warnings for this attribute in Firefox 7.
Attachment #8356929 - Flags: review?(bzbarsky)
Assignee: nobody → ehsan
Comment on attachment 8356929 [details] [diff] [review]
Remove support for Attr.ownerElement; r=bzbarsky

>+++ b/content/xslt/tests/mochitest/test_bug319374.xhtml

The diff here looks odd.  In particular, this bit:

>+    var anonAttr1 = anonElem1.getAttributeNode('attr');
>       getAnonymousNodes(document.getElementById('content').
>-        firstChild)[0].getAttributeNode('attr');

Why is that middle line being left there?  Seems like it should be gone...

Did this test pass on try?  If so, I wonder why...

r=me with that fixed.
Attachment #8356929 - Flags: review?(bzbarsky) → review+
Comment on attachment 8356929 [details] [diff] [review]
Remove support for Attr.ownerElement; r=bzbarsky

Review of attachment 8356929 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +6707,5 @@
>      {
>        // Remove from ownerElement.
>        nsRefPtr<Attr> adoptedAttr = static_cast<Attr*>(adoptedNode);
>  
> +      nsCOMPtr<Element> ownerElement = adoptedAttr->GetContent()->AsElement();

Followup to make GetElement() return an Element, please.

::: dom/imptests/html/dom/nodes/attributes.js
@@ -8,5 @@
>  
>  function attributes_are(el, l) {
>    for (var i = 0, il = l.length; i < il; i++) {
>      attr_is(el.attributes[i], l[i][1], l[i][0], (l[i].length < 3) ? null : l[i][2], null, l[i][0])
> -//    assert_equals(el.attributes[i].ownerElement, el)

Don't change imported tests.
Depends on: 957652
https://hg.mozilla.org/mozilla-central/rev/188d1e255d40
https://hg.mozilla.org/mozilla-central/rev/ab12a8416a1d
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You should also add that info to the Firefox 29 for developers[1] page.

Sebastian

[1] https://developer.mozilla.org/en-US/Firefox/Releases/29
I think we should revert this. See thread with subject "Change to Attr interface in light of XPath" on the webapps-public list.
Hmm, agreed.  I'll prepare a backout patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Also commented on the blink-dev thread to notify the Blink folks.)
Link to that thread: http://lists.w3.org/Archives/Public/www-dom/2014JanMar/0107.html

I assume that's the right one even though it's not public-webapps?
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #13)
> Link to that thread:
> http://lists.w3.org/Archives/Public/www-dom/2014JanMar/0107.html
> 
> I assume that's the right one even though it's not public-webapps?

Yep!
Are we sure we want to back this out? If we can make Attr.prototype.value readonly we could simplify attributes even more.
Switching back to dev-doc-needed so that once the decision to keep it or revert it is taken, I do not forget about updating the doc :-)
Keywords: dev-doc-complete → dev-doc-needed
> Are we sure we want to back this out? If we can make Attr.prototype.value readonly we could simplify 
> attributes even more.

As the mailing list thread in comment 13 indicates the actual idea was to let 'Attr' not longer inherit from 'Node'.
But 'ownerElement' is unrelated to that and necessary for some use cases (or at least making it easier to work with attributes). E.g. it broke things in Firebug[1], which uses a TreeWalker for DOM traversal.
So why was this property actually removed from 'Attr'?

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=7108
The "actual idea" is to simplify the bloated DOM API as much as possible.

That bug suggests Firebug uses DOMWalker, which I'm not familiar with. I don't think TreeWalker gives you access to attributes.
Anne: What data do we have that removing Attr.ownerElement is compatible with the web? Breaking user's websites, even for just 6 weeks, should not be taken lightly. The value of removing this property is relatively low, so we should ensure that the cost is low too.
See comment 0. If we're not happy with Blink's data I'm happy with waiting a bit to see what happens on their end though.
Sadly the data mentioned in the Blink intent-to-remove doesn't appear to be public. I can't see it on http://www.chromestatus.com/metrics/feature/popularity

I'm still not in a hurry to remove this feature given that it actually makes sense spec-wise and it's cheap to implement unless we are able to make bigger changes to attribute nodes.
One thing which we could do is to back this out from beta for now, and leave it in on aurora and nightly while we're trying to make a decision here.

The reason why _I_ think we should revert this is that I think the argument of how is one supposed to get to the owner element if they get handed off an Attr object is very convincing.

But on the other hand, the only person who has commented on my blink-dev post so far (Philip Jägenstedt) doesn't seem to be in a rush to revert the Blink change...
Whatever we end up doing here, we should track this bug for 29+.
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
tracking-firefox31: --- → ?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) (PTO on 3/20) from comment #23)
> One thing which we could do is to back this out from beta for now, and leave
> it in on aurora and nightly while we're trying to make a decision here.

Jonas, what do you think?
Flags: needinfo?(jonas)
I would back this out from everywhere. Removing .ownerElement was just a mistake.
I mean removing .ownerElement from the spec.
I agree with Olli. I see very little value in removing ownerElement at all.
Flags: needinfo?(jonas)
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0e82a3e0d7
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → WONTFIX
See comment 10 onwards for the reason behind this backout.  I tested the backout a bit locally and also on try.  This should bring us back to the code we have had for this for a while so it's not very risky.
Attachment #8394998 - Flags: approval-mozilla-beta?
Attachment #8394998 - Flags: approval-mozilla-aurora?
Jonas: I agree, we should not break web content.  However, all features have a price, and this one has very few users.  Our data shows Chrome seeing Attr.ownerElement on 55 of every 1,000,000 Chrome page loads:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/ai6_ySyVITg/WQk0Ih9v1iIJ

99.9945% of Chrome page loads don't care about this feature.  I agree that the value in removing Attr.ownerElement is small, but I do believe it moves us closer to removing Attr, and removing Attr (or at least making them immutable name/value pairs would remove a lot of Blink code, and presumably Gecko code).

I would encourage Mozilla to also check your Telemetry numbers and see what usage you all see of Attr.ownerElement, I suspect it will be similarly small.
This particular API happens to make sense as long as we have Attr objects, as
http://lists.w3.org/Archives/Public/www-dom/2014JanMar/0107.html clearly indicates.

So, if we want to get rid of XPath, we can after that remove Attr.ownerElement.
http://lists.w3.org/Archives/Public/www-dom/2014JanMar/0116.html seemed to indicate that Xpath was a separate issue and that you could select the ownerElements directly from xpath instead.
Currently XPath, the .value setter, setAttributeNode, .attributes.setNamedItem and .attributes.setNamedItemNS all depend on getting the owner element from an Attr. If we can get rid of those then I agree that it would make sense to get rid of those then it would make sense to also drop ownerElement.

The .value getter currently also uses the owner element, but we can possibly change the implementation not to do that. Though I wouldn't want that to affect performance of element.setAttribute.

I.e. if we can essentially make Attr objects be dumb "POD" objects rather than have an inherent connection back to the element, then removing ownerElement makes sense.
Wait for the patch to land in m-c and I will approve it.
(In reply to comment #36)
> Wait for the patch to land in m-c and I will approve it.

Please note that the patch to bug 957652 should be approved along side with it, since this backout depends on the backout of bug 957652.
According to AWFY, the backout regressed Kraken and Octane (both by ~5%) though improved SS slightly (~1%): http://hg.mozilla.org/integration/mozilla-inbound/rev/bd0e82a3e0d7.
… forgot to mention: Regression is GGC only.
(In reply to comment #38)
> According to AWFY, the backout regressed Kraken and Octane (both by ~5%) though
> improved SS slightly (~1%):
> http://hg.mozilla.org/integration/mozilla-inbound/rev/bd0e82a3e0d7.

Doesn't AWFY run the js shell?  This should NPOTB for the js shell.

Was there a similar improvement when this first landed?
Hmm, the regression is only visible in the 32bit shell (there's a browser build but without GGC). I don't know how to go back in time for the initial landing, there is only accumulated data for Dec 30th - Jan 17th.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #40)
> This should NPOTB for the js shell.

What does NPOTB mean?
(In reply to Florian Bender from comment #41)
> Hmm, the regression is only visible in the 32bit shell (there's a browser
> build but without GGC). I don't know how to go back in time for the initial
> landing, there is only accumulated data for Dec 30th - Jan 17th.
> 
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #40)
> > This should NPOTB for the js shell.
> 
> What does NPOTB mean?

"Not part of the build".

My point was that to the best of my knowledge, AWFY downloads the js engine shell which excludes the rest of the browser code and runs the benchmarks on the shell.  This patch and its backout are both changes to stuff outside of the js engine, so they should not even be part of the build as far as AWFY is concerned.  (Unless I'm missing something of course.)
Ah, thanks. Yeah that bugged me as well. But now I see the two points span across 7 days so that particular commit probably wasn't the cause for the slowdown. Sorry for the false alarm!
Attachment #8394998 - Flags: approval-mozilla-beta?
Attachment #8394998 - Flags: approval-mozilla-beta+
Attachment #8394998 - Flags: approval-mozilla-aurora?
Attachment #8394998 - Flags: approval-mozilla-aurora+
In order to close the loop here, Blink is adding Attr.ownerElement back: <https://codereview.chromium.org/243333003/diff/20001/Source/core/dom/Attr.idl>
Correct.  We have come to believe that we should remove Attr nodes all at once or not at all.  The slow-trickle of breakages appears to be more harmful than good.  If we removed them all at once it would be easier to develop a coherent polyfill, etc. as well.
(In reply to comment #46)
> Correct.  We have come to believe that we should remove Attr nodes all at once
> or not at all.  The slow-trickle of breakages appears to be more harmful than
> good.  If we removed them all at once it would be easier to develop a coherent
> polyfill, etc. as well.

I completely agree with your assessment, and also with the new Blink policy on removing features being proposed on blink-dev.
As a further of this discussion, I'd like to point folks to this comment I made on the W3C's Bugzilla tracker regarding removal of ownerElement:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25086#c17

Cheers,

- Bill
Looks like two years ago it was decided no to remove the ownerElement; so why is FireFox still printing warning in the console for using this Attr field?

Thanks.
Good question.  Ehsan, is there a reason we left the deprecation warning here?
Flags: needinfo?(ehsan)
That was my mistake, sorry about that.  :(

Submitted a patch in bug 1267966 to remove the deprecation warning.  Thanks for catching this, Mehrdad!
Flags: needinfo?(ehsan)
See Also: → 1267966
Super! Thanks for quick response.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: