957431 - Remove Attr.ownerElement

Status: RESOLVED WONTFIX

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

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Remove support for Attr.ownerElement; r=bzbarsky

Comment 2

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=002210aa834f

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 5

[(no longer active)]

Addressed all of the comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/188d1e255d40

Comment 6

[(no longer active)]

Added a missing null check:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab12a8416a1d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/188d1e255d40
https://hg.mozilla.org/mozilla-central/rev/ab12a8416a1d

Comment 8

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/API/Attr
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility

Comment 9

[Sebastian Zartner [:sebo]]

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

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should revert this. See thread with subject "Change to Attr interface in light of XPath" on the webapps-public list.

Comment 11

[(no longer active)]

Hmm, agreed.  I'll prepare a backout patch.

Comment 12

[(no longer active)]

(Also commented on the blink-dev thread to notify the Blink folks.)

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 14

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=fcaa61b70602

Comment 15

[(no longer active)]

(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!

Comment 16

[Anne (:annevk)]

Are we sure we want to back this out? If we can make Attr.prototype.value readonly we could simplify attributes even more.

Comment 17

[Jean-Yves Perrier [:teoli]]

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 :-)

Comment 18

[Sebastian Zartner [:sebo]]

> 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

Comment 19

[Anne (:annevk)]

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.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 21

[Anne (:annevk)]

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.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 23

[(no longer active)]

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...

Comment 24

[(no longer active)]

Whatever we end up doing here, we should track this bug for 29+.

Comment 25

[(no longer active)]

(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?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I would back this out from everywhere. Removing .ownerElement was just a mistake.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

I mean removing .ownerElement from the spec.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with Olli. I see very little value in removing ownerElement at all.

Comment 29

[Kohei Yoshino]

Removed the obsolete flag from https://developer.mozilla.org/en-US/docs/Web/API/Attr
Removed the compatibility notice from https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility

Comment 30

[(no longer active)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0e82a3e0d7

Comment 31

[(no longer active)]

Attachment: Backout patch for branches

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.

Comment 32

[Eric Seidel]

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.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 34

[Eric Seidel]

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.

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Wait for the patch to land in m-c and I will approve it.

Comment 37

[(no longer active)]

(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.

Comment 38

[Florian Bender]

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.

Comment 39

[Florian Bender]

… forgot to mention: Regression is GGC only.

Comment 40

[(no longer active)]

(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?

Comment 41

[Florian Bender]

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?

Comment 42

[(no longer active)]

(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.)

Comment 43

[Florian Bender]

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!

Comment 44

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69e896713b11
https://hg.mozilla.org/releases/mozilla-beta/rev/4b5fc53392dc

Comment 45

[(no longer active)]

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>

Comment 46

[Eric Seidel]

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.

Comment 47

[(no longer active)]

(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.

Comment 48

[William J. Edney]

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

Comment 49

[Mehrdad]

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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Good question.  Ehsan, is there a reason we left the deprecation warning here?

Comment 51

[(no longer active)]

That was my mistake, sorry about that.  :(

Submitted a patch in bug 1267966 to remove the deprecation warning.  Thanks for catching this, Mehrdad!

Comment 52

[Mehrdad]

Super! Thanks for quick response.