Last Comment Bug 687400 - Kill isSameNode
: Kill isSameNode
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on: 723415 723645
Blocks: 1256299
  Show dependency treegraph
 
Reported: 2011-09-18 20:52 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2016-03-14 06:49 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (15.97 KB, patch)
2011-09-18 20:52 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bugs: review+
Details | Diff | Splinter Review
Add a warning (5.95 KB, patch)
2011-09-27 02:30 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bugs: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-18 20:52:40 PDT
Created attachment 560844 [details] [diff] [review]
Patch to fix

This function simply implements the '==' operator. For us it's just useless code we're carrying around, and for JS authors it's directly harmful as it's slower than simply using '=='.

Patch attached which removes it.
Comment 1 Olli Pettay [:smaug] (TPAC) 2011-09-18 23:29:03 PDT
Should we first warn about removal? Google code search shows that isSameNode is used
occasionally. (Although at least in some cases the code is absolutely bizarre; something like
if (node.isSameNode ? node.isSameNode(otherNode) : node == otherNode) ... )
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-19 00:53:30 PDT
I was thinking about that too. It'd be easy to warn if we're fine with removing the custom quickstubs that we currently have, and then put the removal in next release.

Let me know if you prefer that?
Comment 3 Mozilla RelEng Bot 2011-09-19 01:40:52 PDT
Try run for cda7df7c1a15 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cda7df7c1a15
Results (out of 27 total builds):
    success: 25
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sicking@mozilla.com-cda7df7c1a15
Comment 4 Olli Pettay [:smaug] (TPAC) 2011-09-21 00:30:23 PDT
(In reply to Jonas Sicking (:sicking) from comment #2)
> I was thinking about that too. It'd be easy to warn if we're fine with
> removing the custom quickstubs that we currently have, and then put the
> removal in next release.
What has quickstubs to do with the warning?
Comment 5 Olli Pettay [:smaug] (TPAC) 2011-09-21 01:37:51 PDT
Comment on attachment 560844 [details] [diff] [review]
Patch to fix

But we really must warn about the removal and have the warning at least during 
one release.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-27 02:30:11 PDT
Created attachment 562689 [details] [diff] [review]
Add a warning

Olli, feel free to land this if it passes review.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-27 05:40:50 PDT
https://hg.mozilla.org/mozilla-central/rev/ef9284803c47
Comment 8 John A. Bilicki III 2011-10-22 00:45:37 PDT
Is this patch a request to remove the method outright or to rewire what it does internally? If it's an outright removal request what is === compatibility in older browsers contrasting isSameNode? I am finding this invaluable in helping me iron out a Gecko range/selection bug.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-22 02:10:37 PDT
You can always use === to test if two nodes are the same node.

So |a.isSameNode(b)| will produce the same result as |a === b| in all versions of gecko. Well.. except that some older versions of gecko doesn't support the former. So the latter is strictly an improvement. And it executes faster too.
Comment 10 John A. Bilicki III 2011-10-22 02:24:01 PDT
Jonas, my question is if this bug is a request to remove the method outright or to rewire what it does internally?

If you've requested an outright removal of the method then can you please point out which version of the W3C DOM specification where it has been deprecated?
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-22 02:36:15 PDT
It's a request to outright remove it as an experiment to see if it can be done without "breaking the web". The spec editor is waiting for feedback from that experiment before removing it. Please search the public-webapps archives for "isSameNode".
Comment 12 John A. Bilicki III 2011-10-22 02:39:16 PDT
Then you have determined that the === operator is EXPLICITLY intended to work with comparing nodes? If so please reference where in which specification and version it is explicitly stated.
Comment 14 John A. Bilicki III 2011-10-23 10:15:11 PDT
j.j., please clarify which point and how it's wording applies DIRECTLY to context of  matching nodes in the manner isSameNode works.

Jonas, Firefox does not exist for you to personally experiment with to see if you can break the web. If you are concerned about performance on other people's websites then your bug report should have been limited to suggesting a warning be made in the console in strict mode at best.
Comment 15 j.j. 2011-10-23 11:58:31 PDT
[shrug]
7. Return true if x and y refer to the same object. Otherwise, return false.
Not enough?
Comment 16 John A. Bilicki III 2011-10-23 12:05:49 PDT
Incorrect reference, that is under 11.9.3 (==), NOT 11.9.4 (===).
Comment 17 j.j. 2011-10-23 12:08:28 PDT
Sorry, 11.9.6 step 7
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-10-23 12:41:24 PDT
John, if you need an EXPLICIT note in the current W3C DOM specification, please let me know and I'll make it happen.
Comment 19 j.j. 2011-10-23 12:49:08 PDT
> note in the current W3C DOM
Isn't that "language-neutral"?
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-28 19:54:03 PDT
Checked in to inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c201d75331
Comment 21 Marco Bonardo [::mak] 2011-10-29 01:52:06 PDT
https://hg.mozilla.org/mozilla-central/rev/82c201d75331

taking a brief look at this bug's history looks like all patches landed.
Comment 22 John A. Bilicki III 2011-11-01 16:34:55 PDT
The removal of this method is now breaking scripts on my site.

Thanks for not supporting standards and breaking the web!
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-01 21:37:30 PDT
Actually, this was done to better support cross browser behaviour since not all browsers support this function. It was also done to help JS authors as == is faster than isSameNode.

Also,isSameNode is likely going to to be removed from the specs, so there's only advantages to stop using it.
Comment 24 :Ms2ger (⌚ UTC+1/+2) 2011-11-02 03:08:29 PDT
DOM4 has been updated accordingly.
Comment 25 Jorge Villalobos [:jorgev] 2011-11-17 14:38:50 PST
Most add-on developers (and reviewers) test their add-ons looking for error messages, not warnings. Deprecation messages that we know we'll follow through should be errors IMO, to make them easier to spot. 

I see several add-ons on AMO that still use isSameNode, so I think we should extend the deprecation period with an error instead.
Comment 27 Marat Tanalin | tanalin.com 2011-12-07 16:38:24 PST
Though I personally usually don't use isSameNode() method (but I _did_ use on server side), I believe that its removal is a wrong step.

DOM levels should be incremental, and if something is (to be) removed from new DOM4, this does not mean that it should be removed from existing implementations. Stable version of a major browser is noway a place for such experiments.

You are free to mark it as obsolete feature, but you should not remove it.
Comment 28 John Nagle 2011-12-07 16:58:46 PST
I just got the message
"TypeError: elt.isSameNode is not a function" in Firefox 8.
Did someone back-port this "fix"?

(Also, I suspect there may be a bug in comparing nodes with "==" when working through proxy objects in add-ons.  But I haven't confirmed this yet.)
Comment 29 John Nagle 2011-12-07 17:00:59 PST
Disregard previous message re FF 8. My bad.
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2011-12-08 09:39:14 PST
(In reply to Marat Tanalin | tanalin.com from comment #27)
> Though I personally usually don't use isSameNode() method (but I _did_ use
> on server side), I believe that its removal is a wrong step.
> 
> DOM levels should be incremental, and if something is (to be) removed from
> new DOM4, this does not mean that it should be removed from existing
> implementations. Stable version of a major browser is noway a place for such
> experiments.
> 
> You are free to mark it as obsolete feature, but you should not remove it.

As one of the editors of DOM4, I can say that it is the intention that features that were removed from the specification are also removed from implementations, in order to simplify the platform as a whole. If features are to remain in implementations, they will remain in the specification as well.
Comment 31 Arkadiusz Michalski (Spirit) 2016-03-14 06:35:07 PDT
It's back to the DOM spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27424
https://github.com/whatwg/dom/commit/97f0a958bc1dcb98265d116bafdc80ae1b059823
so now will back to Gecko?
Comment 32 Olli Pettay [:smaug] (TPAC) 2016-03-14 06:49:03 PDT
Please file new bugs for new issues ;)

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