Closed Bug 156716 (mediaqueries) Opened 22 years ago Closed 16 years ago

CSS3 "media queries"

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: bugzilla_kl, Assigned: dbaron)

References

()

Details

(4 keywords, Whiteboard: [patch]parity-opera)

Attachments

(14 files, 20 obsolete files)

109.21 KB, patch
Details | Diff | Splinter Review
2.72 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.19 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
14.50 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
8.33 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.44 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
10.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
28.86 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
11.58 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
67.79 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
48.85 KB, patch
Details | Diff | Splinter Review
3.51 KB, patch
Details | Diff | Splinter Review
24.60 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
The CSS3 media queries (alows you to specify not only a media type, but also
some additional parameters) are a candidate recommendation since the 8.7.2002,
so implementing could start.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: css3
Severity: normal → enhancement
Turning into RFE.
Summary: CSS3 "media queries" → [RFE] CSS3 "media queries"
I suggest we start working on this after we've implemented the important bits of
CSS2 and fixed most of our CSS1 bugs...
Priority: -- → P5
Target Milestone: --- → Future
Summary: [RFE] CSS3 "media queries" → CSS3 "media queries"
Attached file test case (obsolete) —
Keywords: testcase
Whiteboard: parity-opera
This could break some pages that use the support of media quries to apply Opera
7 specific CSS rules ( because opera 7 is currently the only browser that
supports this )

See :
http://www.virtuelvis.com/archives/145.html

But if any pages does do this its the webcoders fault .. it's not supposed to be
used like that.

BTW :
http://www.literarymoose.info/=/synopsis/stretch.xhtml describes a useful and
interesting way to use media quries.
This is a prerequisite patch that reorganizes the way we parse and store media
lists.	I'm not sure if it's the right approach:  in particular, it causes us
to fail:
  http://www.hixie.ch/tests/evil/css/import/main/exoticmedia.html
  http://www.hixie.ch/tests/evil/css/import/extra/httpexoticmedia.html
  http://www.hixie.ch/tests/evil/css/import/extra/styleexoticmedia.html
But we still pass:
  http://www.hixie.ch/tests/evil/css/import/extra/importexoticmedia.html
  http://www.hixie.ch/tests/evil/css/import/extra/mediaexoticmedia.html
Then again, I think I could work around that without too much difficulty,
except for the comment bit.  But I'm not sure how well such a workaround would
extend to all of media queries.  I'd really rather not have two separate pieces
of parsing code for all of media queries.
Another approach would, I suppose, be to share the parsing code only for what's
between the commas.  Then the two parts (HTML and CSS) could have different
error handling rules.  Then again, even implementing media queries at all would
practically force us to break HTML's error handling rules (specified in
http://www.w3.org/TR/1999/REC-html401-19991224/types.html#h-6.13 ) for something
like:

  media="x, (y, screen, z)"

since we've introduced semantics for parentheses, which HTML didn't previously
have.  So maybe it's not that bad that this breaks the tests I cited in the
previous comment.
Actually, even that isn't strictly necessary.  I guess we could do all media
query parsing with the rule that comma has higher precedence than (), plus do
what I suggest in the first sentence of the previous comment.
Whiteboard: parity-opera → [patch]parity-opera
You will start failing some of those tests once you implement media queries,
because some of those tests are invalid.

That aside, however, your later comments are correct. There are two steps to
parsing media queries. First, there is a host-language-specific parser for
splitting the media list into separate media. For HTML, that is simply a "split"
on the "," character, with no parenthesis pairing or anything. For CSS, it is a
full CSS parser, splitting at unnested commas. Then, you parse the media
queries. The parser is the same for all host languages, with one dependent
detail: error handling. For CSS, if a syntax error occurs, the entire @media or
@import rule must be dropped altogether. For HTML, if a syntax error occurs,
just that component of the media list is ignored.

Note that unknown properties do not count as syntax errors. Unknown properties
merely make the declaration parse as false.

So

   "screen and (colors: blue, red)"

...would be parsed as:

   media0: "screen and (colors: blue, red)"

...for CSS, but:

   media0: "screen and (colors: blue"
   media1: " red)"

...for HTML. (Doesn't really matter in this case, since all three of those media
declarations are invalid and cause whatever it was to not be applied.)
Yeah, that was the conclusion I came to.

The patch was messier that I expected because it was helpful to clean up a good
bit of our string API usage:  in particular, I'm converting some non-frozen-API
nsAString usage to nsSubstring, which I think is the right thing to do (it's
faster, and it provides a more useful API, which I needed).
Attachment #178509 - Flags: superreview?(bzbarsky)
Attachment #178509 - Flags: review?(bzbarsky)
Note that this renames what's in nsIMediaList.h but doesn't rename the file.  I
don't think it's worth renaming the file since I expect more changes soon, and
I'll want to figure out what do to with files then.
Comment on attachment 178509 [details] [diff] [review]
preparatory cleanup of media parsing and storage

>Index: layout/style/nsCSSLoader.cpp
> CSSLoaderImpl::PrepareSheet(nsICSSStyleSheet* aSheet,

>+    // We have aMediaString only when linked from HTML, so pass PR_TRUE.

This is worth asserting (that is, assert that the loader's mDocument QIs to
nsIHTMLDocument, I would say).

>+  } else if (!aMediaList) {
>+    mediaList = new nsMediaList();

Why is this needed?  The style sheet code looks like it deals fine with a null
media list, no?  If we do decide to make sure that sheet objects always have a
non-null media list, we should null-check here and remove the various
null-checks in nsCSSStyleSheet.

>Index: layout/style/nsCSSParser.cpp
>+  nsresult DoParseMediaList(const nsSubstring& aBuffer,

Might be worth documenting here what this function does and what it expects (eg
a non-null media list passed in...).

>+  PRBool GatherMedia(nsresult& aErrorCode, nsMediaList* aMedia,
>+                     PRUnichar aStopSymbol);

Again, worth adding some docs here.

>+  // True for parsing media lists for HTML attributes, where we have to
>+  // ignore CSS comments.
>+  PRPackedBool mHTMLMode;

Maybe mHTMLMediaMode?

> PRBool CSSParserImpl::ParseImportRule(nsresult& aErrorCode, 
>+    if (!GatherMedia(aErrorCode, media, ';') ||
>+        !ExpectSymbol(aErrorCode, ';', PR_TRUE)) {
>+      REPORT_UNEXPECTED_TOKEN(PEImportUnexpected);
>+      // don't advance section, simply ignore invalid @import
>+      return PR_FALSE;
>+    }

Add a debug-only |else| asserting that |media| has a nonzero length?

> PRBool CSSParserImpl::ParseMediaRule(nsresult& aErrorCode,
>+  if (GatherMedia(aErrorCode, media, '{')) {

And here assert that |media| has a nonzero length?

>Index: layout/style/nsCSSRules.cpp
>+CSSImportRuleImpl::CSSImportRuleImpl(nsMediaList* aMedia)
>-  // XXXbz This is really silly.... the mMedia we create here will be
>-  // clobbered if we manage to load a sheet.  Which should really
>-  // never fail nowadays, in sane cases.

Actually, this is probably still XXX-worthy... we're no longer creating a new
object, but we're still doing random addrefing and releasing on the sheet's
media list...

> CSSImportRuleImpl::CSSImportRuleImpl(const CSSImportRuleImpl& 
>+  if (aCopy.mMedia)
>+    aCopy.mMedia->Clone(getter_AddRefs(mMedia));

And this is probably not needed if we succeed in cloning the copy's sheet,
since our SetSheet will grab the media list from the sheet...

> nsCSSMediaRule::GetCssText(nsAString& aCssText)

Why not just use the GetText method of mMedia here like the import rule does? 
I really don't know why it wasn't done that way to start with...  Same in
nsCSSMediaRule::List(), probably.

>         if (index > 0)

In any case, that would fail to compile, since |index| is no longer declared.

>Index: layout/style/nsCSSStyleSheet.cpp

>+nsMediaList::Item(PRUint32 aIndex, nsAString& aReturn)
>+  nsIAtom *medium = MediumAt(aIndex);

Hmm... I don't see MediumAt doing any bounds-checking, and this is a scriptable
interface.  Need to bounds-check somewhere -- either here or inside MediumAt. 
The latter is probably better, since we want to bounds-check after casting to
PRInt32 to catch cases when a PRUint32 would overflow and give us a negative
index.

>@@ -2091,29 +1951,28 @@ void nsCSSStyleSheet::List(FILE* out, PR

And also use GetText here?

>Index: layout/style/nsICSSParser.h
>+  NS_IMETHOD ParseMediaList(const nsSubstring& aBuffer,

Document this a bit?  For example, does it assume that a non-null media list
will be passed in?  (I know it does, from reading the code, but that should be
documented.)  Same for whether the passed-in list is cleared, etc.

r+sr=bzbarsky with those nits fixed.
Attachment #178509 - Flags: superreview?(bzbarsky)
Attachment #178509 - Flags: superreview+
Attachment #178509 - Flags: review?(bzbarsky)
Attachment #178509 - Flags: review+
(In reply to comment #11)
> (From update of attachment 178509 [details] [diff] [review] [edit])
> >Index: layout/style/nsCSSLoader.cpp
> > CSSLoaderImpl::PrepareSheet(nsICSSStyleSheet* aSheet,
> 
> >+    // We have aMediaString only when linked from HTML, so pass PR_TRUE.
> 
> This is worth asserting (that is, assert that the loader's mDocument QIs to
> nsIHTMLDocument, I would say).

Sorry, I meant linked from link elements, style elements, or PIs (as opposed to
from @import, which always parses its own media list).
Ah, ok.  Makes sense, then.
Attachment #178998 - Attachment description: preparatory cleanup of media parsing and storage → preparatory cleanup of media parsing and storage (checked in 2005-03-29 16:36 -0800)
Alias: media → mediaqueries
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [patch]parity-opera → [wanted-1.9][patch]parity-opera
QA Contact: ian → style-system
Just an observation: it looks like Opera and Safari (I believe Konqueror, as well) have had this for about a year now, meaning the race is now between Gecko and Trident to see who crosses the finish line last.

I'm not trying to be negative, just wanted everyone to be aware that the pressure has increased. Thanks for all your hard work!
Blocks: acid3
Flags: wanted1.9+
Whiteboard: [wanted-1.9][patch]parity-opera → [patch]parity-opera
This might be useful for Mozilla Mobile's efforts
Keywords: mobile
Yes, this is near the top of my list for work after Fx 3.0 ships.
Status: NEW → ASSIGNED
Attached patch work in progress (obsolete) — Splinter Review
Attachment #246256 - Attachment is obsolete: true
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
This is the previous patch with some remaining features added and a few bug fixes.
Attachment #314235 - Attachment is obsolete: true
(To be clear, these are still a work in progress; I'll be revising them further before requesting review.)
This probably needs to be optimized further, but it's the remaining piece to get us passing the Acid3 media queries test.

(That said, some of the code in patch 1 / attachment 316774 [details] [diff] [review] for some of the color features probably isn't quite shippable quality yet, but I enabled it to make sure I was passing that test; I might want to comment it out again for the initial landing.)
Promoting the flush on the parent seems like it should be ok.  From a safety viewpoint, Flush_Style can already run scripts, so we don't lose much.  From a performance viewpoint, we'll flush the parent once, and then the remaining flushes for further operations in the child will be no-ops.  So it should be ok, in my opinion.
Fixes a logic error that prevented features that allow min- and max- from being present without them.
Attachment #316774 - Attachment is obsolete: true
I think this matches the latest consensus in the various relevant standards bodies.  It also allows media queries through for HTML.
marking wanted1.9.1? to get this in the triage queue. 
Flags: wanted1.9.1?
Adding to 1.9.1 list.
Flags: wanted1.9.1? → wanted1.9.1+
So I'm going to attach patches for review; I think there are a few issues where the spec may change (marked in the patch and tests) and also a few other things that we should clean up.  But I think this is getting pretty close to the point where we should get it checked in; some other issues can be dealt with in followup bugs and may not even be absolutely necessary for the first release in which we ship the feature.
See comment 33.
Attachment #323026 - Attachment is obsolete: true
Attachment #325868 - Flags: superreview?(bzbarsky)
Attachment #325868 - Flags: review?(bzbarsky)
This changes our parsing of -moz-column-count, where we use ParsePositiveVariant on integers already even though it didn't handle them.

I also use this further in the next patch.
Attachment #325869 - Flags: superreview?(roc)
Attachment #325869 - Flags: review?(bzbarsky)
Attachment #323025 - Attachment is obsolete: true
Attachment #325870 - Flags: superreview?(bzbarsky)
Attachment #325870 - Flags: review?(bzbarsky)
Attachment #320097 - Attachment is obsolete: true
Attachment #325871 - Flags: superreview?(bzbarsky)
Attachment #325871 - Flags: review?(bzbarsky)
Attachment #316776 - Attachment is obsolete: true
Attachment #325872 - Flags: superreview?(bzbarsky)
Attachment #325872 - Flags: review?(bzbarsky)
Attachment #320098 - Attachment is obsolete: true
Attachment #325873 - Flags: superreview?(bzbarsky)
Attachment #325873 - Flags: review?(bzbarsky)
Attachment #320099 - Attachment is obsolete: true
Attachment #320855 - Attachment is obsolete: true
Attachment #325876 - Flags: superreview?(bzbarsky)
Attachment #325876 - Flags: review?(bzbarsky)
Without this, I fail a bunch of my tests when I have my laptop plugged into a larger monitor but still mirroring to the smaller laptop display.  (If I turn off the mirroring to the laptop display, I get the large size even without this patch.)

It really seems like this shouldn't be needed, but it is...
Attachment #325877 - Flags: superreview?(roc)
Attachment #325877 - Flags: review?(roc)
Attached patch patch 10: testsSplinter Review
I'm sure there's a lot more that I could add, but this is a lot already...
Attachment #323027 - Attachment is obsolete: true
Attachment #325878 - Flags: superreview?(bzbarsky)
Attachment #325878 - Flags: review?(bzbarsky)
It'll likely take me a week or two to get to this (just like everything else).
Attachment #325869 - Flags: superreview?(roc) → superreview+
Attachment #325877 - Flags: superreview?(roc)
Attachment #325877 - Flags: superreview+
Attachment #325877 - Flags: review?(roc)
Attachment #325877 - Flags: review+
Comment on attachment 325868 [details] [diff] [review]
patch 1: revert most of the special media list parsing for HTML

>+++ b/layout/style/nsCSSParser.cpp
>+  if (aHTMLMode) {
>     mHTMLMediaMode = PR_FALSE;

Why bother with the conditional?  Might it be faster to just do an unconditional set?

> // All parameters but the first are the same as for |ParseMediaList|,
> // but for HTML we get the buffer in chunks according to the HTML spec's
> // parsing rules instead of in one piece.

This is no longer true.  In fact, we can probably eliminate DoParseMediaList altogether and move that code into ParseMediaList, right?

r+sr=bzbarsky with the second comment addressed; either way on the conditional thing.
Attachment #325868 - Flags: superreview?(bzbarsky)
Attachment #325868 - Flags: superreview+
Attachment #325868 - Flags: review?(bzbarsky)
Attachment #325868 - Flags: review+
Comment on attachment 325869 [details] [diff] [review]
patch 2: make ParsePositiveVariant check integer values

r=bzbarsky
Attachment #325869 - Flags: review?(bzbarsky) → review+
Comment on attachment 325870 [details] [diff] [review]
patch 3: core of media queries (no dynamic change handling)

>+++ b/layout/style/nsCSSParser.cpp
>@@ -1053,16 +1111,20 @@ CSSParserImpl::DoParseMediaList(const ns
>   if (!GatherMedia(rv, aMediaList, PRUnichar(0)) && !mHTMLMediaMode) {
>+    // XXX Instead of clearing here, should we have been parsing into a
>+    // fresh media list in the first place?

The only caller is ParseMediaList (see comments on patch1), and that does clear the list, doesn't it?  So I'm not sure what this comment is really asking.

Also, do we really want to skip the Clear() and SetNonEmpty() calls when we're in HTML mode?  That seems wrong to me.

>+    aMediaList->Clear();
>+    aMediaList->SetNonEmpty(); // don't match anything

Would it make sense to have a single method that does both of those? aMediaList->SetParseError() or something?  Not a big deal either way.

> PRBool CSSParserImpl::GatherMedia(nsresult& aErrorCode,
>+  // "If the comma-separated list is the empty list it is assumed to
>+  // specify the media query ‘all’."  (css3-mediaqueries, section

Would you mind switching the quotes around 'all' to ASCII ones?

>+    nsMediaQuery *query = queryHolder;

This is just an optimization, right?

>+    nsresult rv = aMedia->AppendQuery(query);
>+    if (NS_FAILED(rv)) {
>+      aErrorCode = rv;
>+      return PR_FALSE;
>+    }
>+    queryHolder.forget(); // aMedia now owns query

How about we make AppendQuery take an nsAutoPtr<nsMediaQuery> and document that it will take ownership if it keeps a point to the query?  That way appending it to the nsTArray would automatically transfer ownership to the right place (see nsAutoPtr::nsAutoPtr(nsAutoPtr<T>) ) and this forget() wouldn't have to happen.  It seems like that setup would make it impossible to accidentally double-delete, right?

>+PRBool CSSParserImpl::ParseMediaQueryExpression(nsresult& aErrorCode, nsMediaQuery* aQuery)
>+    case nsMediaFeature::eIntRatio:
>+      {
>+        // XXX This allows whitespace around the '/'.  The spec isn't
>+        // clear on whether we should.

The editor's draft now makes it clear that we should.

>+++ b/layout/style/nsCSSStyleSheet.cpp
>+nsMediaExpression::Matches(nsPresContext *aPresContext,
>+    case nsMediaFeature::eIntRatio:

You should probably also assert that the array lengths are 2 here.  And maybe that the integers are strictly positive?

>+    case nsMediaFeature::eResolution:
>+          actualDPI = actualDPI * 2.54f;

Maybe put the constant in nsCoord.h, which has things that depend on that constant anyway?

>+nsMediaQuery::AppendToString(nsAString& aString) const
>+  if (mHadUnknownExpression) {
>+    // FIXME: I heard anne mention there was something specific we
>+    // should do here.

Is there a followup bug on this?

>+            nsCSSDeclaration::AppendCSSValueToString(eCSSProperty_width,
>+                                                     expr.mValue, aString);

This could use a comment about the fact that the eCSSProperty_width doesn't matter as long as it's some property that only takes length values.

>+            nsCSSDeclaration::AppendCSSValueToString(eCSSProperty_z_index,

Similar comment here, and for the eIntRatio cases.

>+          case nsMediaFeature::eIntRatio:

Assert that the array has length 2?

>+nsMediaQuery::Matches(nsPresContext* aPresContext) const
>+  if (mHadUnknownExpression)
>+    return mNegated; // FIXME: Editor's draft says PR_FALSE, but that
>+                     // disagrees with Acid3 and is likely to change.

Is this still an open issue?  Has this been raised with the editor?  The editor's draft still seems to say PR_FALSE.

In general, are we aiming to implement the error handling rules as specified in the June 12, 2008 editor's draft, or something else?

>+    nsresult rv = (expr.mFeature->mGetter)(aPresContext, actual);
>+    NS_ENSURE_SUCCESS(rv, PR_FALSE); // any better ideas?

The only way rv can be failure is if the allocation fails for aspect ratios, right?  Once we switch to no-failure allocation in moz2 it all won't matter anyway and we can make these getters return void (or even return an nsCSSValue).

>+++ b/layout/style/nsIMediaList.h
>+class nsMediaQuery {
>+  void SetType(nsIAtom* aMediaType)     { mMediaType = aMediaType; }

Worth asserting that aMediaType != nsnull?

>+++ b/layout/style/nsMediaFeatures.cpp
>+GetDeviceWidth(nsPresContext* aPresContext, nsCSSValue& aResult)
>+{
>+    // XXX: I'm not sure if this is really the right thing for print:
>+    // do we want to include unprintable areas / page margins?

I'm not sure how that differs from, e.g. reporting the screen width which includes toolbars, menubars, etc....  Just reporting the device seems like the right ting to me; people shouldn't really be using this anyway, imo (and imo it shouldn't even be in the spec).

>+GetColor(nsPresContext* aPresContext, nsCSSValue& aResult)
>+    // FIXME: On a monochrome device, return 0!

Is there a followup bug on this?

>+GetColorIndex(nsPresContext* aPresContext, nsCSSValue& aResult)
>+    // return zero.  Given that we there isn't any better information
>+    // exposed,

s/we there/there/

>+GetResolution(nsPresContext* aPresContext, nsCSSValue& aResult)
>+{
>+    // XXX: The spec isn't clear whether this is CSS pixels or device
>+    // pixels.  For now, do device pixels.

We should raise this with the editor too.  I'm not sure which makes more sense.

I assume this is the patch that was mostly going to change in response to changes in the editor's draft, right?  I'd be happy to either review an updated patch or a patch on top of this to address both the things above and the editor's draft updates.
Comment on attachment 325871 [details] [diff] [review]
patch 4: dynamic change handling, at the rule cascade level

>+++ b/layout/style/nsCSSRuleProcessor.cpp
>@@ -2311,33 +2317,31 @@ nsCSSRuleProcessor::GetRuleCascade(nsPre
>   // Having RuleCascadeData objects be per-medium works for now since
>   // nsCSSRuleProcessor objects are per-document.  (For a given set
>   // of stylesheets they can vary based on medium (@media) or document
>   // (@-moz-document).)  Things will get a little more complicated if
>   // we implement media queries, though.

That comment doesn't seem to match the new reality.

>+    if (cascade->mCacheKey.Matches(aPresContext))
>       return cascade;

So we could run into situations where we have a bunch of rule cascades that are no longer relevant (if we have a slew of media queries and the user does a bunch of resizing of the window, say).  Is that an issue?  Should we be trying to get rid of rule cascades after some period of time?  I think we should certainly clear rule cascades on memory pressure.  Might be worth a followup bug.

>+++ b/layout/style/nsCSSStyleSheet.cpp
> 
>+
> template <class Numeric>
> PRInt32 DoCompare(Numeric a, Numeric b)

Why add that blank line?

>+nsMediaQueryResultCacheKey::Matches(nsPresContext* aPresContext) const
>+    NS_ENSURE_SUCCESS(rv, PR_FALSE); // any better ideas?

Again, this should stop being an issue soon.

>+++ b/layout/style/nsIMediaList.h

>+ * This object may not be used after any any media rules in any of the
>+ * sheets it was given to have been modified.

Or after any of the style sheets' media texts have been changed, right?

> However, this is
>+ * generally not a problem since ClearRuleCascades is called on the
>+ * sheet whenever this happens, and these objects are stored inside the
>+ * rule cascades.

We don't ClearRuleCascades if someone does a SetText on a nsMediaList.  I guess DOM callers can only do SetMediaText, but even then consider this testcase:

data:text/html,<link media="screen" rel="stylesheet" href="data:text/css,body{ color: green}">aaa

and then doing:

javascript:alert(document.styleSheets[0].media.mediaText); document.styleSheets[0].media.mediaText = "print"; alert(document.styleSheets[0].media.mediaText)

The text stays green in my trunk build (though I'm not sure why).  That's pretty suspect.  Maybe we're clearing rule cascades but not triggering an actual restyle?

There's also the XXX comment in the nsCSSMediaRule copy constructor that worries me.

Finally, nsCSSImportRule::SetMedia calls SetText directly.  Does it not share the media list with the child sheet?

So basically, I think this needs some careful auditing.  Ideally, SetText would be private, and we would make sure that an nsMediaList always had a stylesheet to notify (and that it's always the right one) or that we don't set a stylesheet unless we're sure of it and disallow modification if we don't have a stylesheet to notify.

The potential here for accidentally ending up with references to deleted memory really scares me.  :(

Alternately, we could make nsMediaExpression reference-counted to make sure that the worst-case scenario is not a deleted object access, until we clean up the nsMediaList mess.

>+class nsMediaQueryResultCacheKey {
>+  void AddExpression(const nsMediaExpression* aExpression,
>+                     PRBool aExpressionMatches);

Worth documenting what this means: that aExpressionMatches records that we're caching that aExpression matched, and that if aExpression doesn't match later on then whatever is keyed by this nsMediaQueryResultCacheKey shouldn't be used.

r- until I'm a little more convinced on the nsMediaExpression cache thing.
Attachment #325871 - Flags: superreview?(bzbarsky)
Attachment #325871 - Flags: superreview-
Attachment #325871 - Flags: review?(bzbarsky)
Attachment #325871 - Flags: review-
Comment on attachment 325872 [details] [diff] [review]
patch 5: remove some unneeded null-checks in nsStyleSet

r+sr=bzbarsky
Attachment #325872 - Flags: superreview?(bzbarsky)
Attachment #325872 - Flags: superreview+
Attachment #325872 - Flags: review?(bzbarsky)
Attachment #325872 - Flags: review+
Comment on attachment 325873 [details] [diff] [review]
patch 6: dynamic change handling at the rule processor level

>+nsCSSRuleProcessor::RefreshRuleCascade(nsPresContext* aPresContext)
>+      // Ensure that the current one is always mRuleCascades.
>+      newCascade->mNext = mRuleCascades;
>+      mRuleCascades = newCascade;
>       cascade = newCascade.forget();

Wouldn't

  mRuleCascades = newCascade.forget();

and scoping |cascade| to the only place it's read (the while loop above) work just as well, and be clearer?

>+++ b/layout/style/nsStyleSet.cpp
>+nsStyleSet::MediumFeaturesChanged(nsPresContext* aPresContext)
>+  // XXX We don't notify mBindingManager.  Should we?

That's the only way that <xbl:resources> stylesheets can be handled, right?  Might be worth writing some rules in those that use media queries and then seeing whether they actually work.  If not (as I suspect), file a followup bug to get it working?

r+sr=bzbarsky with that
Attachment #325873 - Flags: superreview?(bzbarsky)
Attachment #325873 - Flags: superreview+
Attachment #325873 - Flags: review?(bzbarsky)
Attachment #325873 - Flags: review+
Comment on attachment 325875 [details] [diff] [review]
patch 7: dynamic change handling at the pres context level

>+++ b/layout/base/nsPresContext.cpp
>+      MediaFeatureValuesChanged();
>       RebuildAllStyleData(NS_STYLE_HINT_REFLOW);

Shouldn't that pass PR_TRUE to MediaFeatureValuesChanged?

>@@ -1155,16 +1157,17 @@ nsPresContext::SetFullZoom(float aZoom)
>+  MediaFeatureValuesChanged();
>   RebuildAllStyleData(NS_STYLE_HINT_REFLOW);

Same here.

Maybe the argument should just not have a default value to avoid this being a problem?

r+sr=bzbarsky as long as those don't rebuild style data twice
Attachment #325875 - Flags: superreview?(bzbarsky)
Attachment #325875 - Flags: superreview+
Attachment #325875 - Flags: review?(bzbarsky)
Attachment #325875 - Flags: review+
Comment on attachment 325876 [details] [diff] [review]
patch 8: when passing flushes to the parent, promote style flushes to layout flushes

r+sr=bzbarsky
Attachment #325876 - Flags: superreview?(bzbarsky)
Attachment #325876 - Flags: superreview+
Attachment #325876 - Flags: review?(bzbarsky)
Attachment #325876 - Flags: review+
Comment on attachment 325878 [details] [diff] [review]
patch 10: tests

+++ b/layout/style/test/test_acid3_test46.html
@@ -0,0 +1,100 @@
+    for (var i = doc.documentElement.childNodes.length-1; i >= 0; i -= 1)
+      doc.documentElement.removeChild(doc.documentElement.childNodes[i]);

Isn't this equivalent to |doc.documentElement.textContent = "";|?

I guess if this is what the acid3 test has, we can just keep it as-is.

+++ b/layout/style/test/test_media_queries.html
@@ -0,0 +1,290 @@
+  let width_val = 117;
+  let height_val = 76;

It's not clear where these numbers are coming from...  Are they just "not too round not default" numbers?  Might be worth documenting that, if so.

+  for each (let feature in [ "max-aspect-ratio", "device-aspect-ratio" ]) {

Do we want to add tests for whitespace handling around the '/'?

Could we add some tests for dynamic mutation of media lists in various ways: mutation of sheet.media vs (linking element).media, (@import rule).media vs (imported sheet).media, mutations via the various DOM accessors vs SetMediaText.

r+sr=bzbarsky on the tests here, with the above additions either pre or post-landing.
Attachment #325878 - Flags: superreview?(bzbarsky)
Attachment #325878 - Flags: superreview+
Attachment #325878 - Flags: review?(bzbarsky)
Attachment #325878 - Flags: review+
(In reply to comment #49)
> > // All parameters but the first are the same as for |ParseMediaList|,
> > // but for HTML we get the buffer in chunks according to the HTML spec's
> > // parsing rules instead of in one piece.
> 
> This is no longer true.  In fact, we can probably eliminate DoParseMediaList
> altogether and move that code into ParseMediaList, right?

Yeah.  So the reason it's the way it is is that I wrote this patch late in the process, but wanted it to go underneath the other patches because it simplified what I needed to do in the patches above them.  Eliminating that would have made a good bit of merge pain for the other patches on this bug.  I'll do that merging in an additional patch on top of the other patches.
(In reply to comment #51)
> >+    nsMediaQuery *query = queryHolder;
> 
> This is just an optimization, right?

Not really, since I want to keep the pointer to |query| around after transferring ownership from queryHolder to aMedia.  There will always be *something* that owns it, but there are multiple returns.

> >+    nsresult rv = aMedia->AppendQuery(query);
> >+    if (NS_FAILED(rv)) {
> >+      aErrorCode = rv;
> >+      return PR_FALSE;
> >+    }
> >+    queryHolder.forget(); // aMedia now owns query
> 
> How about we make AppendQuery take an nsAutoPtr<nsMediaQuery> and document that
> it will take ownership if it keeps a point to the query?  That way appending it
> to the nsTArray would automatically transfer ownership to the right place (see
> nsAutoPtr::nsAutoPtr(nsAutoPtr<T>) ) and this forget() wouldn't have to happen.
>  It seems like that setup would make it impossible to accidentally
> double-delete, right?

Done, and also moved the appending earlier so I can make queryHolder have much less scope (and thus generate less code for early returns, perhaps).

> >+++ b/layout/style/nsCSSStyleSheet.cpp
> >+nsMediaExpression::Matches(nsPresContext *aPresContext,
> >+    case nsMediaFeature::eIntRatio:
> 
> You should probably also assert that the array lengths are 2 here.  And maybe
> that the integers are strictly positive?

Agreed about the array lengths.  The code doesn't make any assumptions about being strictly positive, and I'd rather not assert that.  In fact, |actual| can have one side be zero (for 'aspect-ratio' in a 0-width or 0-height iframe).

> >+    case nsMediaFeature::eResolution:
> >+          actualDPI = actualDPI * 2.54f;
> 
> Maybe put the constant in nsCoord.h, which has things that depend on that
> constant anyway?

I'd rather not; thinking about conversions between 'dpi' and 'dpcm' is confusing enough without seeing whether the constant you're looking at is 2.54 or its reciprocal.

> >+nsMediaQuery::AppendToString(nsAString& aString) const
> >+  if (mHadUnknownExpression) {
> >+    // FIXME: I heard anne mention there was something specific we
> >+    // should do here.
> 
> Is there a followup bug on this?

I think it's obsolete given the change to:

# When all media queries in a media query list are ignored it is the same as
# if "not all" was specified.

That's part of addressing the substantive spec change that I'd mentioned to you.

> >+nsMediaQuery::Matches(nsPresContext* aPresContext) const
> >+  if (mHadUnknownExpression)
> >+    return mNegated; // FIXME: Editor's draft says PR_FALSE, but that
> >+                     // disagrees with Acid3 and is likely to change.
> 
> Is this still an open issue?  Has this been raised with the editor?  The
> editor's draft still seems to say PR_FALSE.
> 
> In general, are we aiming to implement the error handling rules as specified in
> the June 12, 2008 editor's draft, or something else?

This is the spec issue I'd mentioned to you; Acid3 has now been fixed:
http://ln.hixie.ch/?start=1215829569&count=1

> >+    nsresult rv = (expr.mFeature->mGetter)(aPresContext, actual);
> >+    NS_ENSURE_SUCCESS(rv, PR_FALSE); // any better ideas?
> 
> The only way rv can be failure is if the allocation fails for aspect ratios,
> right?  Once we switch to no-failure allocation in moz2 it all won't matter
> anyway and we can make these getters return void (or even return an
> nsCSSValue).

Yep.

> >+++ b/layout/style/nsMediaFeatures.cpp
> >+GetDeviceWidth(nsPresContext* aPresContext, nsCSSValue& aResult)
> >+{
> >+    // XXX: I'm not sure if this is really the right thing for print:
> >+    // do we want to include unprintable areas / page margins?
> 
> I'm not sure how that differs from, e.g. reporting the screen width which
> includes toolbars, menubars, etc....  Just reporting the device seems like the
> right ting to me; people shouldn't really be using this anyway, imo (and imo it
> shouldn't even be in the spec).

I'm also not sure which we currently do.  I should file a followup bug.

> >+GetColor(nsPresContext* aPresContext, nsCSSValue& aResult)
> >+    // FIXME: On a monochrome device, return 0!
> 
> Is there a followup bug on this?

I'll file one.

> >+GetResolution(nsPresContext* aPresContext, nsCSSValue& aResult)
> >+{
> >+    // XXX: The spec isn't clear whether this is CSS pixels or device
> >+    // pixels.  For now, do device pixels.
> 
> We should raise this with the editor too.  I'm not sure which makes more sense.

This has been clarified in the "Units" section of the spec (to be device pixels).

> I assume this is the patch that was mostly going to change in response to
> changes in the editor's draft, right?  I'd be happy to either review an updated
> patch or a patch on top of this to address both the things above and the
> editor's draft updates.
> 

Yeah, the spec change in question was the one with the Acid3 comment above (which also means we avoid the serialization issue mentioned since we should serialize as "not all").
(In reply to comment #59)
> > How about we make AppendQuery take an nsAutoPtr<nsMediaQuery> and document that
> > it will take ownership if it keeps a point to the query?  That way appending it
> > to the nsTArray would automatically transfer ownership to the right place (see
> > nsAutoPtr::nsAutoPtr(nsAutoPtr<T>) ) and this forget() wouldn't have to happen.
> >  It seems like that setup would make it impossible to accidentally
> > double-delete, right?
> 
> Done, and also moved the appending earlier so I can make queryHolder have much
> less scope (and thus generate less code for early returns, perhaps).

Er, actually, I think I'd tried that before and switch to what I had... since the compiler error message that results is rather familiar.  Basically, nsTArray's various appending functions will only work with const arguments passed to them.  So you can pass an |nsMediaQuery*const|, but not an |nsAutoPtr<nsMediaQuery>&|.  There are various workarounds, though I haven't thought of any that are particularly elegant; I think I'll stick with what I have (although keep the reorganization).
Actually, I put the logic into AppendQuery this time, which is a little more elegant.
(In reply to comment #51)
> Also, do we really want to skip the Clear() and SetNonEmpty() calls when we're
> in HTML mode?  That seems wrong to me.

It seems wrong to me as well.  But it seems a bunch of my tests for parseability were depending on that mistake, though.  I think the tests are fixable, though.
(In reply to comment #62)
> (In reply to comment #51)
> > Also, do we really want to skip the Clear() and SetNonEmpty() calls when we're
> > in HTML mode?  That seems wrong to me.
> 
> It seems wrong to me as well.  But it seems a bunch of my tests for
> parseability were depending on that mistake, though.  I think the tests are
> fixable, though.

Though the problem in question went away once I finished addressing the other error handling issue.
This deals with the second half of comment 54.
Attachment #331223 - Flags: superreview?(bzbarsky)
Attachment #331223 - Flags: review?(bzbarsky)
Attachment #325870 - Attachment is obsolete: true
Attachment #331224 - Flags: superreview?(bzbarsky)
Attachment #331224 - Flags: review?(bzbarsky)
Attachment #325870 - Flags: superreview?(bzbarsky)
Attachment #325870 - Flags: review?(bzbarsky)
I still need to address the main comments on attachment 325871 [details] [diff] [review].
Comment on attachment 331223 [details] [diff] [review]
patch 12: pass dynamic change notifications on to the binding manager too

r+sr=bzbarsky
Attachment #331223 - Flags: superreview?(bzbarsky)
Attachment #331223 - Flags: superreview+
Attachment #331223 - Flags: review?(bzbarsky)
Attachment #331223 - Flags: review+
Comment on attachment 331224 [details] [diff] [review]
patch 3: core of media queries (no dynamic change handling)

Looks good.  Note that I reviewed the interdiff, not this diff.
Attachment #331224 - Flags: superreview?(bzbarsky)
Attachment #331224 - Flags: superreview+
Attachment #331224 - Flags: review?(bzbarsky)
Attachment #331224 - Flags: review+
Attachment #325871 - Attachment is obsolete: true
Attachment #331362 - Flags: superreview?(bzbarsky)
Attachment #331362 - Flags: review?(bzbarsky)
Attachment #331362 - Attachment is patch: true
Attachment #331362 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 331362 [details] [diff] [review]
patch 4: dynamic change handling, at the rule cascade level

r+sr=bzbarsky (based on the interdiff)
Attachment #331362 - Flags: superreview?(bzbarsky)
Attachment #331362 - Flags: superreview+
Attachment #331362 - Flags: review?(bzbarsky)
Attachment #331362 - Flags: review+
Depends on: 448281
(In reply to comment #73)
> Still some followups to be filed before closing this bug out (and always more
> tests that could be written...).
> 
I was going to try to get some test dev folks pointed at this for writing more tests.  From looking over the specification and the tests that you already wrote, what other tests are needed?  Here are some of my thoughts:

* More exhaustive testing of media-type and media-feature pairings (testing valid and invalid pairings are detected as such)
* Range-Testing items with both min-* and max-* attributes to ensure ranges are interpreted and applied correctly
* Testing that missing () around expressions causes the expression to not be parsed.
* Test that relative units are matched properly
** What happens to a media query if it's relative unit is changed by script?  i.e. media query designates 3em unit, the root element's font size is set to 12pt, but script later changes that to 16pt and reruns the media query.

Is that last bullet something we need to worry about?  Or are media-queries unaffected by reflow?  I tend to think they'd be unaffected, but I'm still wrapping my head around this.

Are there other items we need to test that I'm not considering?

I think we need a bunch of tests for the print cases, some of which aren't working correctly yet (that's the big area where we need followup bugs).  That is, testing that all the features work correctly based on what we know about the printer.

We could also use more tests for dynamic change cases, e.g., making sure that things that cause any of the features to change actually get reevaluated dynamically.  These include changes of preferences (layout.css.dpi, font size preferences (where I think we're currently buggy), changes of full zoom or text zoom, changes of window size, dynamic changes (from the OS) of screen size or resolution, etc.  Some of these are easy to test; some are quite hard.

(In reply to comment #74)
> * Range-Testing items with both min-* and max-* attributes to ensure ranges are
> interpreted and applied correctly

The existing tests do a bit of this.

> * Test that relative units are matched properly
> ** What happens to a media query if it's relative unit is changed by script? 
> i.e. media query designates 3em unit, the root element's font size is set to
> 12pt, but script later changes that to 16pt and reruns the media query.

'em' units are relative to the initial value of 'font-size', so they don't change in response to what the root element's font size is set to.
Because bug 448132 fixed, acid 3 test 46 took too much time.
A new bug need to be filed.
Gabriele, I'm not sure what comment 76 means.  If a bug needs to be filed, please file, with enough details to tell what the bug is about?
I've gotten some feedback on my documentation that I could use some clarification on.  In particular, questions about how Gecko handles various situations:

Does Firefox implement all the media types?  If not, which aren't supported, and are they ignored or assumed to be something else?

When in full-screen mode, does Firefox use a different value for the current media type (for example, "tv" or "projection")?

Does Fennec use "handheld" or "screen" or do both match?

Is there a way to tell Firefox to use "handheld" when run on handheld tablets?

Does Gecko recompute the applicable media styles and adjust the CSS when the user resizes the window or pane?

Also, a specification issue has been pointed out: the min- and max- prefixes do not let you specify two exclusive ranges that abut one another.  There will either be a gap, or they will overlap.

I'd appreciate some insight on these.  Thanks!
> Does Firefox implement all the media types?  

No.  We implement screen and print (though some add-ons implement projection, iirc).

> When in full-screen mode, does Firefox use a different value 

No, modulo above-mentioned add-ons.

I don't know the answer to the Fennec question for sure, but I suspect "screen", unless they've changed some core Gecko code.

> Is there a way to tell Firefox to use "handheld" when run on handheld tablets?

Not right now.

> Does Gecko recompute the applicable media styles and adjust the CSS when the
> user resizes the window or pane?

Yes.  The tests for this bug test that...

> Also, a specification issue has been pointed out

Indeed.  That should probably be raised with the working group?
Depends on: 468645
So I should have marked this bug as fixed ages ago (comment 73), but marking it as fixed now.

There are a number of followup bugs in the "depends on" list, and also bug 448035 (as noted above).

Comment 75 still lists some additional things that could use more tests, although bug 466171 (and bug 466559) covered the biggest one.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I filed what I think is the main remaining point in comment 75 as bug 469881.
Flags: in-testsuite+
Depends on: 757554
You need to log in before you can comment on or make changes to this bug.