Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Description • 12 years ago

This is a painfully useless feature.  Could we try always returning true?  With any luck, that will put us always in the "good" code path.  It's possible that it will break enough sites that we have to make exceptions, but it seems like a way better strategy than trying to spec all the feature strings that compat requires.  Perhaps more importantly, it should serve as a clear indicator to authors that the function is useless and shouldn't be used!

Ms2ger is apparently fine with this, so if Anne is too, we'd love to change the spec.  We just need UAs willing to take a small risk and see if any sites break.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 12 years ago

Do we have any data how often we return false currently?

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 2 • 12 years ago

Should I make nsSVGFeatures::HasFeature always return true too?  It seems to be used by a test file.

Why does nsDOMAttribute::IsSupported warn but nothing else does?  I don't see a warning in the console anyway when I test, although maybe I'm doing it wrong.

Try, since I'm guessing this will cause at least one mochitest failure: https://tbpl.mozilla.org/?tree=Try&rev=5eb11043eb4e

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 12 years ago

Comment on attachment 671232 [details] [diff] [review]
Patch

Review of attachment 671232 [details] [diff] [review]:
-----------------------------------------------------------------

Worth a try, I think.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

ccing Jonas and David for their thoughts.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 12 years ago

I'm fine with doing this if the spec is changed to say that this is the new specced behavior. I agree that these functions are useless, but it's a little iffy to go our own way against the spec anyway.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 6 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #5)
> I'm fine with doing this if the spec is changed to say that this is the new
> specced behavior. I agree that these functions are useless, but it's a
> little iffy to go our own way against the spec anyway.

Anne, Ms2ger, and I are all happy to change the spec if someone (e.g., Gecko) is willing to try it out.  I'll do that (and land official tests) before I land any change to Gecko, so we'll match the spec.


On a related note: the DOM spec says that isSupported shouldn't exist.  Anne thinks this is better in the long term because it's one less useless function, and that it's tenable because practically no pages use isSupported (unlike hasFeature).  Are we interested in doing this?  My feeling is that maintaining a one-line function forever is negligible cost, and there's no point in breaking even a tiny number of pages to gain such a tiny simplification.

	Masatoshi Kimura [:emk]
	Comment 7 • 12 years ago

(In reply to :Aryeh Gregor from comment #6)
> On a related note: the DOM spec says that isSupported shouldn't exist.  Anne
> thinks this is better in the long term because it's one less useless
> function, and that it's tenable because practically no pages use isSupported
> (unlike hasFeature).  Are we interested in doing this?  My feeling is that
> maintaining a one-line function forever is negligible cost, and there's no
> point in breaking even a tiny number of pages to gain such a tiny
> simplification.
We've already broken a few pages by removing functions such as xmlVersion. If we will never remove it, it should be added to the spec.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 12 years ago

Let's worry about whether to remove it or not in a separate bug, and for now just make the functions return true.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 9 • 12 years ago

I just removed the failing dom-level2-core tests outright.  They're horrible and we should get rid of as many as possible.  I also got rid of Ms2ger's test because it will be changed or replaced upstream when the spec change lands, which will be before this patch lands, so there's no point in trying to register all the new failures.  We'll get the new upstream tests when we next sync.

No changes here other than to tests, relative to the last patch version.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 10 • 12 years ago

(This also bitrots bug 773780.  :(  I think that has to land soon if we don't want to block all Node-related work on it!)

	Robert Longson [:longsonr]
	Comment 11 • 12 years ago

There are some w3c svg testsuite tests for hasFeature e.g.

http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObject/struct-dom-02-b.html
http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObject/struct-dom-03-b.html
http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObject/struct-dom-04-b.html

Perhaps you should suggest to the svg w3c group that these be removed/deprecated?

Also feature strings are used in svg by http://www.w3.org/TR/SVG/struct.html#ConditionalProcessing and http://www.w3.org/TR/SVG/feature.html 

If hasFeature is going away then would need updating for SVG2 wouldn't it?

In the meantime won't it be odd that hasFeature returns true but when you create an SVG element with a requiredFeatures attribute it might or might not display? 

I'm not suggesting that feature strings are good/useful and a legitimate solution may be to rip them them out of SVG and/or say that requiredFeatures never disables any SVG element whether gecko has implemented that part of the specification or not.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 12 • 12 years ago

I'm not familiar with SVG's use of hasFeature.  From what you're saying, and glancing at the spec, it looks like SVG uses feature strings for declarative feature-testing, and uses hasFeature() to match.  This might well be worth keeping around.  If so, the DOM spec needs to special-case SVG.  Perhaps it could say that any feature strings beginning with "http://www.w3.org/TR/SVG" or "org.w3c.svg" returns true or false depending on what the SVG spec says, and all others return true.  Does that make sense to everyone?

	Robert Longson [:longsonr]
	Comment 13 • 12 years ago

We return true/false for SVG feature strings dependent on what things we've implemented. For instance we haven't implemented SVG fonts so we return false for http://www.w3.org/TR/SVG11/feature#Font

Comment 12 makes sense to me.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 12 years ago

> I think that has to land soon if we don't want to block all Node-related work on it!

Well, I think it could have landed today if someone hadn't bitrotted it in spite of an explicit request to the contrary.

But yes, it needs to land soon, period.  And it's way higher priority than any other Node change I've seen coming through, honestly.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 15 • 12 years ago

The code being removed looks pretty sketchy -- were these two APIs really supposed to have the same implementation?

In any case, I don't think this API is useful given that nothing makes us keep the API results in sync with the code.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 16 • 12 years ago

(In reply to David Baron [:dbaron] from comment #15)
> The code being removed looks pretty sketchy -- were these two APIs really
> supposed to have the same implementation?

I think so, yes.  The old standard seems fairly vague about whether there's supposed to be any difference:

http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMFeatures

I guess the theory might be that some features could be supported for one node but not another, but I don't see anything from a glance at the spec that suggests anything specific, so making them always the same appears legit.

> In any case, I don't think this API is useful given that nothing makes us
> keep the API results in sync with the code.

Comment 11 says that SVG uses this feature mechanism declaratively, so that you can define fallback in a general way for browsers that don't support a certain feature.  It's similar to the various feature-testing methods often proposed for CSS, except that in CSS it's usually not hard to design properties to have good fallback given CSS error handling rules, but I guess that's not the case in SVG.  Given that the declarative SVG feature looks useful, it makes sense to have a programmatic way to access the same info, for SVG.  hasFeature() already works for this purpose, and is used in the wild, so it makes sense to me to keep it for this.

On the other hand, hasFeature() is not useful at all for DOM features, because it's almost always more intuitive and reliable and fine-grained to just check whether the relevant properties exist, or call them and catch any exceptions, etc.

So at this point I'd suggest returning true for all non-SVG features, and keeping the existing functionality for SVG, as outlined in comment 12.  Does that sound good to everyone?

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 12 years ago

I guess that sounds ok to me.

> I think that has to land soon if we don't want to block all Node-related work on it!

Peter is aiming to do that tomorrow.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

Comment on attachment 671357 [details] [diff] [review]
Patch with updated tests, for less orange!

r- pending updated patch.

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 19 • 12 years ago

Anne has agreed to the SVG exception.  I'll update the spec and add some upstream tests now.

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

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 12 years ago

Comment on attachment 671811 [details] [diff] [review]
Patch v3, doesn't disable SVG feature-testing

r=me

	Aryeh Gregor (:ayg) (no longer with Mozilla) Assignee
	Comment 21 • 12 years ago

Try is green, spec and test changes are here:

https://github.com/whatwg/dom/commit/cdf75764431b41dd2e3e6ca671b39ca3c8d55262
http://dvcs.w3.org/hg/webapps/rev/a9725c7b0fd1

And there's no conflict with bug 773780.

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

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f73f203effde

	Florian Scholz (Open Web Docs)
	Comment 23 • 11 years ago

Documented:
https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers#DOM
https://developer.mozilla.org/en-US/docs/DOM/Node.isSupported#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/DOM/document.implementation#Gecko-specific_notes