Last Comment Bug 849661 - Remove support for Node.hasAttributes()
: Remove support for Node.hasAttributes()
Status: RESOLVED WONTFIX
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 856752
Blocks: 853865
  Show dependency treegraph
 
Reported: 2013-03-10 13:20 PDT by :Ms2ger
Modified: 2013-10-12 04:16 PDT (History)
15 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (33.32 KB, patch)
2013-03-20 07:39 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Review

Description :Ms2ger 2013-03-10 13:20:55 PDT
Node.hasAttributes() has been removed from the spec: <http://dom.spec.whatwg.org/#dom-node-hasattributes>. We should try to remove it as well.
Comment 1 :Aryeh Gregor (away until August 15) 2013-03-19 08:19:15 PDT
Tested already in <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/historical.html>.
Comment 2 :Aryeh Gregor (away until August 15) 2013-03-20 07:10:20 PDT
We actually use this method in parser/htmlparser/tests/mochitest/parser_datreader.js.  Now that .attributes is on Element instead of Node, the correct replacement of node.hasAttributes() is node.attributes && node.attributes.length, right?  That's a bit verbose, especially when "node" is the result of a method.
Comment 3 :Aryeh Gregor (away until August 15) 2013-03-20 07:39:01 PDT
Created attachment 727198 [details] [diff] [review]
Patch

I'm not totally convinced we actually want to do this, especially since .attributes is no longer on Node per spec.  But if we do, here's a patch.  WebKit seems not to support the method, so probably there's no big compat risk.

https://tbpl.mozilla.org/?tree=Try&rev=f084e2ba16a6
Comment 4 Boris Zbarsky [:bz] 2013-03-20 09:44:29 PDT
I'm not totally convinced either.  Peter?  Jonas?
Comment 5 Jonas Sicking (:sicking) 2013-03-20 10:04:17 PDT
Unless there is compat risk, or there are strong stated use cases, I think we should nuke it from orbit.
Comment 6 Boris Zbarsky [:bz] 2013-03-20 19:08:14 PDT
Comment on attachment 727198 [details] [diff] [review]
Patch

r=me
Comment 7 :Ms2ger 2013-03-21 03:00:07 PDT
Strictly speaking, you should also update the uuids for all subclasses of nsIDOMNode; fortunately, there's a script: <http://people.mozilla.org/~sfink/uploads/update-uuids>.
Comment 8 :Aryeh Gregor (away until August 15) 2013-03-21 05:50:55 PDT
Should I update all the other IIDs as Ms2ger says?  I don't think anyone's ever asked me to do that before (maybe I broke some extensions!).
Comment 9 Boris Zbarsky [:bz] 2013-03-21 06:32:18 PDT
> Should I update all the other IIDs as Ms2ger says?

Yes.  I keep forgetting about that bit of annoyance.  :(
Comment 10 :Aryeh Gregor (away until August 15) 2013-03-21 08:02:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7db1904193
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-03-21 14:08:46 PDT
https://hg.mozilla.org/mozilla-central/rev/cf7db1904193
Comment 12 Joshua Cranmer [:jcranmer] 2013-03-21 18:19:07 PDT
(In reply to Jonas Sicking (:sicking) from comment #5)
> Unless there is compat risk, or there are strong stated use cases, I think
> we should nuke it from orbit.

It's used in <https://developer.mozilla.org/en-US/docs/JXON>, and you've just broken comm-central's JXON code as a result.
Comment 13 Boris Zbarsky [:bz] 2013-03-21 18:39:31 PDT
Can that be fixed using comment 2?
Comment 14 Joshua Cranmer [:jcranmer] 2013-03-21 19:02:43 PDT
Probably; I haven't tested it.
Comment 15 Mark Banner (:standard8) 2013-03-22 03:08:17 PDT
I ported the change suggested in comment 2 and it passed the broken unit tests:

https://hg.mozilla.org/comm-central/rev/cebc16631aa6
Comment 17 Kohei Yoshino [:kohei] 2013-03-29 09:54:14 PDT
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
Comment 18 Erik Arvidsson 2013-03-29 12:27:04 PDT
WebKit moved Node.prototype.hasAttributes and Node.prototype.attributes to Element.prototype. Removing hasAttributes completely seems like too large of a risk.
Comment 19 Mike Kaply [:mkaply] 2013-03-29 14:45:08 PDT
(In reply to Erik Arvidsson from comment #18)
> WebKit moved Node.prototype.hasAttributes and Node.prototype.attributes to
> Element.prototype. Removing hasAttributes completely seems like too large of
> a risk.

As in don't remove it from Node?

If you don't, you lose the ability to easily check for it before using it (which is the easy fix for the JXON code)
Comment 20 Jonas Sicking (:sicking) 2013-04-01 00:44:31 PDT
The fact that webkit still has this certainly puts the risk involved here into a different light.

I don't have strong opinions though. Ideal would be to go through the safe route of warning+measuring (using telemetry) and then removing only if usage is small enough. But that's a lot of work so I don't have a strong opinion.

But please be careful before issuing statements like the one in comment 3.
Comment 21 Kohei Yoshino [:kohei] 2013-04-02 07:28:51 PDT
Adding a brand-new site-compat keyword.
Comment 22 :Aryeh Gregor (away until August 15) 2013-04-02 07:30:20 PDT
Sorry for comment 3 -- I don't know what test I was using, but probably I was using a non-element node like "document" and didn't think to check an element.  I don't see the value in trying to remove this if everyone does in fact support it; I'd be amazed if it doesn't cause at least a few pages to break.
Comment 23 :Aryeh Gregor (away until August 15) 2013-04-02 07:45:28 PDT
Spec bug asking for it to be re-added: https://bugs.webkit.org/show_bug.cgi?id=113580
Comment 24 :Aryeh Gregor (away until August 15) 2013-04-02 07:45:55 PDT
I meant: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21522
Comment 25 Boris Zbarsky [:bz] 2013-04-02 07:48:20 PDT
Thank you, Aryeh.

I've backed this out on inbound in bug 856752.
Comment 26 Günter 2013-04-03 09:44:21 PDT
(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> 
> It's used in <https://developer.mozilla.org/en-US/docs/JXON>, and you've
> just broken comm-central's JXON code as a result.

That's excactly what happed to my Reminderfox implementation for CalDAV support following that JXON page!
It was working up to TB 21 but failed with TB22.0a2!

Note You need to log in before you can comment on or make changes to this bug.