Remove Attr.ownerElement

RESOLVED WONTFIX

Status

()

Core
DOM
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla29
dev-doc-complete, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29+ wontfix, firefox30+ wontfix, firefox31+ wontfix)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8356929 [details] [diff] [review]
Remove support for Attr.ownerElement; r=bzbarsky
(Assignee)

Updated

4 years ago
Attachment #8356929 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Addressed all of the comments.

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

Updated

4 years ago
Depends on: 957652
https://hg.mozilla.org/mozilla-central/rev/188d1e255d40
https://hg.mozilla.org/mozilla-central/rev/ab12a8416a1d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
https://developer.mozilla.org/en-US/docs/Web/API/Attr
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility
Keywords: dev-doc-needed → dev-doc-complete, site-compat
OS: Mac OS X → All
Hardware: x86 → All
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.
(Assignee)

Comment 11

4 years ago
Hmm, agreed.  I'll prepare a backout patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

4 years ago
(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?
(Assignee)

Comment 15

4 years ago
(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

4 years ago
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

Comment 19

4 years ago
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.

Comment 21

4 years ago
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.
(Assignee)

Comment 23

4 years ago
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...
(Assignee)

Comment 24

4 years ago
Whatever we end up doing here, we should track this bug for 29+.
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
tracking-firefox31: --- → ?
(Assignee)

Comment 25

4 years ago
(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?
(Assignee)

Updated

4 years ago
Flags: needinfo?(jonas)
tracking-firefox29: ? → +
tracking-firefox30: ? → +
tracking-firefox31: ? → +
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)
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 30

4 years ago
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0e82a3e0d7
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

4 years ago
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
(Assignee)

Comment 31

4 years ago
Created attachment 8394998 [details] [diff] [review]
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.
Attachment #8394998 - Flags: approval-mozilla-beta?
Attachment #8394998 - Flags: approval-mozilla-aurora?

Comment 32

4 years ago
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.

Comment 34

4 years ago
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.
(Assignee)

Comment 37

4 years ago
(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

4 years ago
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

4 years ago
… forgot to mention: Regression is GGC only.
(Assignee)

Comment 40

4 years ago
(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

4 years ago
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?
(Assignee)

Comment 42

4 years ago
(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

4 years ago
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+
(Assignee)

Comment 44

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/69e896713b11
https://hg.mozilla.org/releases/mozilla-beta/rev/4b5fc53392dc
status-firefox29: affected → wontfix
status-firefox30: affected → wontfix
status-firefox31: fixed → wontfix
(Assignee)

Comment 45

4 years ago
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

4 years ago
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.
(Assignee)

Comment 47

4 years ago
(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

4 years ago
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

2 years ago
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)
(Assignee)

Comment 51

2 years ago
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: → bug 1267966

Comment 52

2 years ago
Super! Thanks for quick response.
You need to log in before you can comment on or make changes to this bug.