Closed Bug 687400 Opened 13 years ago Closed 13 years ago

Kill isSameNode

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: sicking, Assigned: sicking)

References

Details

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

Attachments

(2 files)

Attached patch Patch to fixSplinter Review
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.
Attachment #560844 - Flags: review?(Olli.Pettay)
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) ... )
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
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?
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
(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 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.
Attachment #560844 - Flags: review?(Olli.Pettay) → review+
Attached patch Add a warningSplinter Review
Olli, feel free to land this if it passes review.
Assignee: nobody → jonas
Attachment #562689 - Flags: review?(Olli.Pettay)
Attachment #562689 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [inbound]
Attachment #562689 - Flags: checkin+
Whiteboard: [inbound] → [leave open for remaining patch]
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.
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.
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?
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".
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.
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.
[shrug]
7. Return true if x and y refer to the same object. Otherwise, return false.
Not enough?
Incorrect reference, that is under 11.9.3 (==), NOT 11.9.4 (===).
Sorry, 11.9.6 step 7
John, if you need an EXPLICIT note in the current W3C DOM specification, please let me know and I'll make it happen.
> note in the current W3C DOM
Isn't that "language-neutral"?
https://hg.mozilla.org/mozilla-central/rev/82c201d75331

taking a brief look at this bug's history looks like all patches landed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [leave open for remaining patch]
Target Milestone: --- → mozilla10
The removal of this method is now breaking scripts on my site.

Thanks for not supporting standards and breaking the web!
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.
DOM4 has been updated accordingly.
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.
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.
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.)
Disregard previous message re FF 8. My bad.
(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.
Depends on: 723415
Depends on: 723645
Please file new bugs for new issues ;)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.