Last Comment Bug 801425 - Make hasFeature() and isSupported() always return true
: Make hasFeature() and isSupported() always return true
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
https://www.w3.org/Bugs/Public/show_b...
Depends on: 848081 773780 801562
Blocks: 495081
  Show dependency treegraph
 
Reported: 2012-10-14 09:53 PDT by :Aryeh Gregor (away until August 15)
Modified: 2013-03-23 13:04 PDT (History)
12 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.63 KB, patch)
2012-10-14 10:10 PDT, :Aryeh Gregor (away until August 15)
Ms2ger: feedback+
Details | Diff | Review
Patch with updated tests, for less orange! (45.95 KB, patch)
2012-10-15 02:47 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review-
Details | Diff | Review
Patch v3, doesn't disable SVG feature-testing (33.42 KB, patch)
2012-10-16 06:10 PDT, :Aryeh Gregor (away until August 15)
bzbarsky: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-10-14 09:53:10 PDT
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.
Comment 1 Olli Pettay [:smaug] 2012-10-14 10:10:10 PDT
Do we have any data how often we return false currently?
Comment 2 :Aryeh Gregor (away until August 15) 2012-10-14 10:10:36 PDT
Created attachment 671232 [details] [diff] [review]
Patch

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
Comment 3 :Ms2ger 2012-10-14 10:13:22 PDT
Comment on attachment 671232 [details] [diff] [review]
Patch

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

Worth a try, I think.
Comment 4 Boris Zbarsky [:bz] 2012-10-14 16:37:54 PDT
ccing Jonas and David for their thoughts.
Comment 5 Jonas Sicking (:sicking) 2012-10-14 22:25:24 PDT
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.
Comment 6 :Aryeh Gregor (away until August 15) 2012-10-15 00:10:26 PDT
(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.
Comment 7 Masatoshi Kimura [:emk] 2012-10-15 00:44:04 PDT
(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.
Comment 8 Jonas Sicking (:sicking) 2012-10-15 01:38:55 PDT
Let's worry about whether to remove it or not in a separate bug, and for now just make the functions return true.
Comment 9 :Aryeh Gregor (away until August 15) 2012-10-15 02:47:02 PDT
Created attachment 671357 [details] [diff] [review]
Patch with updated tests, for less orange!

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.
Comment 10 :Aryeh Gregor (away until August 15) 2012-10-15 02:50:07 PDT
(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!)
Comment 11 Robert Longson 2012-10-15 03:17:24 PDT
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.
Comment 12 :Aryeh Gregor (away until August 15) 2012-10-15 03:54:56 PDT
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?
Comment 13 Robert Longson 2012-10-15 04:16:17 PDT
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.
Comment 14 Boris Zbarsky [:bz] 2012-10-15 05:37:19 PDT
> 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.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-10-15 09:10:57 PDT
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.
Comment 16 :Aryeh Gregor (away until August 15) 2012-10-15 09:30:56 PDT
(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?
Comment 17 Boris Zbarsky [:bz] 2012-10-15 11:35:45 PDT
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.
Comment 18 Boris Zbarsky [:bz] 2012-10-15 11:38:14 PDT
Comment on attachment 671357 [details] [diff] [review]
Patch with updated tests, for less orange!

r- pending updated patch.
Comment 19 :Aryeh Gregor (away until August 15) 2012-10-16 06:10:42 PDT
Created attachment 671811 [details] [diff] [review]
Patch v3, doesn't disable SVG feature-testing

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
Comment 20 Boris Zbarsky [:bz] 2012-10-16 10:47:35 PDT
Comment on attachment 671811 [details] [diff] [review]
Patch v3, doesn't disable SVG feature-testing

r=me
Comment 21 :Aryeh Gregor (away until August 15) 2012-10-28 05:27:22 PDT
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
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-10-28 14:02:39 PDT
https://hg.mozilla.org/mozilla-central/rev/f73f203effde

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