Paul Irish Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.3 (KHTML, like Gecko) Chrome/22.0.1223.1 Safari/537.3

Steps to reproduce:

http://dev.w3.org/csswg/css3-conditional/#window-api defines an imperative API for checking support through the @supports conditional CSS functionality just implemented in Bug 649740

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

Seems reasonable to me.  Cameron, do you want to do this?  Should be reasonably simple, I think.

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

We might want to get a slightly wider consensus on the name; I changed the name to supportsCSS and haven't yet discussed that name with anyone.  (I made that change because, in the context of the window object, I felt that having "CSS" in the name was important to provide additional context, and otherwise I wanted it to be short -- which means I didn't provide anything in the name to indicate that it has two arguments, a property-value pair.)

	Cameron McCormack (:heycam) Assignee
	Comment 3 • 12 years ago

I can do it.  I'm also not sure about the name.  "supportsCSS" feels to me like it could take a string of some top-level CSS like "@media all { h1 { color: green } }".  Is "supportsProperty" any better?  (That might sound like it takes only a property name.)

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

Tab's original was supportsProperty, but that felt pretty odd to me on the window object (which is not at all CSS specific), particularly given the similarity in name to ECMA's defineProperty.  And supportCSSProperty feels rather long to me.

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 12 years ago

This seems to work.  Not sure if you want to delay review until the name is more stable.

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

Comment on attachment 648595 [details] [diff] [review]
patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +3984,5 @@
> +
> +  nsCSSParser parser;
> +  parser.SetErrorReporting(false);
> +
> +  nsRefPtr<nsSimpleURI> uri(new nsSimpleURI);

I have no idea if this is correct... Why not just use the URLs from the document?

	Cameron McCormack (:heycam) Assignee
	Comment 7 • 12 years ago

It shouldn't matter, and it was easier for me create a new object than work out how to get the document's URI. :)

	Paul Irish Reporter
	Comment 8 • 12 years ago

supportsStyle() was my initial favorite. More specific than the "CSS" language and also rather terse.

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

(In reply to :Ms2ger (Back and backlogged) from comment #6)
> > +  nsRefPtr<nsSimpleURI> uri(new nsSimpleURI);
> 
> I have no idea if this is correct... Why not just use the URLs from the
> document?

It is absolutely not correct.  Which URI class is correct to use depends on the scheme; nsSimpleURI is the correct class for about: URLs but not for http: URLs.  The only correct way to create a URI object is as described in https://wiki.mozilla.org/Gecko:Overview#URI_objects .

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

mDoc->GetDocumentURI() should be fine.

	Cameron McCormack (:heycam) Assignee
	Comment 11 • 12 years ago

Do I need to check that mDoc is not null?  (When can it be null?)

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

It might be able to go null in cases when a page holds on to a window object for an iframe that has been removed from the document....

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

http://lists.w3.org/Archives/Public/www-style/2012Aug/0749.html has the group decisions from San Diego about this; spec should get revised soon, but not yet.

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 12 years ago

I wasn't totally sure what to do if a premature end of the string was reached within a condition or property value.  The behaviour that fell out was the same as EOF handling in style sheets.  For example these return true:

  CSS.supports("(color: green) /*")
  CSS.supports("(color: green")
  CSS.supports("(font-family: 'Helvetica")
  CSS.supports("color", "green /*")
  CSS.supports("font-family", "'Helvetica")

and these false:

  CSS.supports("(font-family: 'Helvetica\n")
  CSS.supports("font-family", "'Helvetica\n")

Another I was unsure about was:

  CSS.supports("Color", "green")

That is currently returning true, but the spec says it should return true only if it is "a literal match for the name of a CSS property that the UA supports".

	Cameron McCormack (:heycam) Assignee
	Comment 17 • 12 years ago

bz: I'm using static Web IDL operations, and they generate static member functions that take an extra nsISupports* for the global object.  I don't need those in my supports() functions; is it worth being able to turn this argument off with something in Bindings.conf if it's not needed?

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

Comment on attachment 684915 [details] [diff] [review]
patch (v1.2)

>+if (!SpecialPowers.getBoolPref("layout.css.supports-rule.enabled")) {
>+  todo(false, "skipping test because pref is disabled");
>+} else {
Is it impossible to enable the pref for tests?
This check is virtually making the test useless.

	Cameron McCormack (:heycam) Assignee
	Comment 19 • 12 years ago

I don't think it's useless, since the test will still run on the mozilla-central and aurora builders, but you are right that it would be better to force the pref on like I do for the @supports reftests.

	Cameron McCormack (:heycam) Assignee
	Comment 20 • 12 years ago

Force the @supports pref on in the test.

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

Yeah, it might be worth having an annotation in the IDL to indicate that the function's behavior does not depend on the global and hence doesn't need it...  File a bug on that?

	Cameron McCormack (:heycam) Assignee
	Comment 22 • 12 years ago

Filed bug 815404 for that.

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

(In reply to Cameron McCormack (:heycam) from comment #19)
> I don't think it's useless, since the test will still run on the
> mozilla-central and aurora builders, but you are right that it would be
> better to force the pref on like I do for the @supports reftests.
Meant to say "useless to catch the regression" because the test will never fail as long as the pref is disabled.
Anyway, thanks to updating the patch promptly.

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

Comment on attachment 685018 [details] [diff] [review]
patch (v1.3)


CSS.webidl:

 - should have "conditionText" rather than "declaration"

>+ * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
>+ * liability, trademark and document use rules apply.

I don't think this is right.

layout/style/Makefile.in:

Put it only in EXPORTS_mozilla/dom and not in EXPORTS

nsCSSParser:

Could you name the new methods Evaluate rather than Parse?

>+  bool parsedOK = ParseProperty(propID);
>+  if (parsedOK && GetToken(true)) {
>+    parsedOK = false;
>+  }

Could you just do:

  bool parsedOK = ParseProperty(propID) && !GetToken(true);

(and likewise in the next method)




I wonder if:
 * it would be useful to pass a sheet URI for error reporting
 * are there any differences in what's valid depending on whether
   there's a base URI or not?
 * are there any differences in what's valid depending on the principal?

If so, maybe it's worth having the CSS object remember what document it
came from (probably two separate URIs, I suspect)?

I'd like Boris's opinions on those -- and probably also a brief look at the WebIDL end as well.

I probably should have redirected this review to bzbarsky at the
beginning.



The test would probably have been better written as a testharness.js
test, but at this point I wouldn't rewrite it.

r=dbaron with that contingent on bz's feedback

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

(In reply to Cameron McCormack (:heycam) from comment #17)
> bz: I'm using static Web IDL operations, and they generate static member
> functions that take an extra nsISupports* for the global object.  I don't
> need those in my supports() functions; is it worth being able to turn this
> argument off with something in Bindings.conf if it's not needed?

I think the "I wonder if:" list in comment 24 rather contradicts this.

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

The WebIDL builds look fine modulo the naming nit.

For the rest, in order:

1)  I think we should probably suppress error reporting during supports() calls.  Are we
    not doing that?

2)  The only thing that we treat as invalid or not depending on base URI are relative-URI
    @import rules.

3)  If you parse anything that might have attempts to call SetValueToURL you need to set 
    a principal.  If you don't, we'll just treat it as invalid CSS.  At the moment that
    seems to include anything involving -moz-image-rect, font src stuff, and anything
    involving VARIANT_URL.  Note that we don't _do_ anything with the principal per se,
    but URLValue requires a non-null principal...

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

(In reply to Boris Zbarsky (:bz) from comment #26)
> 3)  If you parse anything that might have attempts to call SetValueToURL you
> need to set 
>     a principal.  If you don't, we'll just treat it as invalid CSS.  At the
> moment that
>     seems to include anything involving -moz-image-rect, font src stuff, and
> anything
>     involving VARIANT_URL.  Note that we don't _do_ anything with the
> principal per se,
>     but URLValue requires a non-null principal...

So this seems relevant to CSS.supports(), since people may well pass values that have url() in them.

If we're doing the work to pass a principal through, it seems we may as well pass the base and sheet (error reporting) URLs through as well.

	Cameron McCormack (:heycam) Assignee
	Comment 28 • 12 years ago

(In reply to David Baron [:dbaron] from comment #24)
> >+ * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> >+ * liability, trademark and document use rules apply.
> 
> I don't think this is right.

Why is that not right?  There are other .webidl files in that directory that are based on IDL snippets from W3C specs which have a similar copyright notice and that don't, so I'm not really sure what the rule is here.

	Cameron McCormack (:heycam) Assignee
	Comment 29 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #26)
> The WebIDL builds look fine modulo the naming nit.
> 
> For the rest, in order:
> 
> 1)  I think we should probably suppress error reporting during supports()
> calls.  Are we
>     not doing that?

We're not explicitly, but I'm not OUTPUT_ERRORing them at the end.  I can use the nsAutoSuppressErrors to handle this explicitly (and avoid the overhead of building up the error strings).

(In reply to David Baron [:dbaron] from comment #27)
> If we're doing the work to pass a principal through, it seems we may as well
> pass the base and sheet (error reporting) URLs through as well.

I'm not sure it make sense to pass a sheet URL.  In

  <!DOCTYPE html>
  <script>CSS.supports("blah")</script>

what would it be?

	Cameron McCormack (:heycam) Assignee
	Comment 30 • 12 years ago

Does it make sense to use the document's URI and principal like this?

Still not sure it is worth passing anything except the principal, given we won't report any CSS errors.

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

Comment on attachment 695279 [details] [diff] [review]
patch (v1.4)

You should probably error out if "win" comes out null in GetParsingInfo, or something.  Though maybe using a null doc/base URI is ok.

You can use win->GetDoc() to get an nsIDocument* as retval without the XPCOM goop.  If it's null, again throw or us null URIs.

Apart from that, this looks fine.  The only thing I'd maybe change is using a struct return value that stores raw nsIURI* or something so we don't have to do the extra refcounting.

	Cameron McCormack (:heycam) Assignee
	Comment 32 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #31)
> You should probably error out if "win" comes out null in GetParsingInfo, or
> something.  Though maybe using a null doc/base URI is ok.

Can win ever be null?  Only if aGlobal can be null I guess?

	Cameron McCormack (:heycam) Assignee
	Comment 33 • 12 years ago

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

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

> Can win ever be null?  Only if aGlobal can be null I guess?

I'm not sure what exactly can happen with sandboxes and JS components and whatnot there...  in general, aGlobal can in theory be null or can be just a non-window (e.g. a sandbox PrincipalHolder), but I'm not sure whether that can happen in practice.

	Cameron McCormack (:heycam) Assignee
	Comment 35 • 12 years ago

Added a null check:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0b040b16d8

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

https://hg.mozilla.org/mozilla-central/rev/a981b50c1d43
https://hg.mozilla.org/mozilla-central/rev/0b0b040b16d8

	Jean-Yves Perrier [:teoli]
	Comment 37 • 11 years ago

Documented:
https://developer.mozilla.org/en-US/docs/Web/API/CSS.supports
https://developer.mozilla.org/en-US/docs/Web/API/CSS
and 
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/22 (on by default)
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/20 (off by default)