Bug 4127 – WebCore doesn't support Media Queries (CSS3 module)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kimmo Kinnunen]

WebCore doesn't support css3 media queries. They're extension of CSS media types, and allow small 
queries to be made about display device capabilities. 

http://www.w3.org/TR/css3-mediaqueries/

A media query can be inserted where a media type can appear in stylesheet or media -attribute of <style> 
tag.

Media Queries are not that interesting though, as there is zero real world content (Google finds one 
page..). Opera has some media queries support.

[Kimmo Kinnunen]

Created attachment 3080 [details]
Initial implementation of CSS Media Queries

Patch to implement CSS3 Media Queries. Affects WebCore/khtml/{css,html}

Note that the patch was created with ordinary diff, as I couldn't get
'cvs-create-patch' to include new files into the patch.

[Kimmo Kinnunen]

Created attachment 3081 [details]
~20 adhoc layout tests to test CSS3 Media Queries

Layout tests to test the media query implementation.
First three test also HTML4 media descriptions parsing.
Last two will fail because khtml doesn't do style recalc if stylesheet's media
attribute is changed via JS.

Note that this patch is create with ordinary diff, as I couldn't get
'cvs-create-patch' to include new files.

[Kimmo Kinnunen]

Created attachment 3082 [details]
Initial implementation of CSS Media Queries (contains ForwardingHeaders and xcodeproj also)

Initial implementation of CSS Media Queriesedia Queries. The first one was
missing ForwardingHeaders and xcodeproj files. Sorry for the inconvinience.

[Maciej Stachowiak]

Dave said he'd like to discuss the need for this change, since it's a significant hunk of new code and does 
not appear to have any current real-world uses.

[Eric Seidel (no email)]

Comment on attachment 3082 [details]
Initial implementation of CSS Media Queries (contains ForwardingHeaders and xcodeproj also)

Beth officially owns CSS (according to the assignment guidelines) and thus have
assigned this patch to her.  Hyatt should also take a look.  Regardless, this
patch has been waiting for nearly TWO MONTHS and needs to be reviewed.

[Darin Adler]

Comment on attachment 3082 [details]
Initial implementation of CSS Media Queries (contains ForwardingHeaders and xcodeproj also)

Just talked to Dave, and he's going to be the one reviewing this.

[Dave Hyatt]

Comment on attachment 3082 [details]
Initial implementation of CSS Media Queries (contains ForwardingHeaders and xcodeproj also)

Looks good to me.  We need some tests, and also need to check for leaks. 
Finally someone from Apple will have to run the perf tests.

[Darin Adler]

I asked Tim to land a couple patches that have been here for a long time, and he agreed to do it.

[Timothy Hatcher]

This patch no long applies cleanly. We have recently removed QPaintDeviceMetrics from the tree which this patch relies heavily. I am attempting to modify it for work.

[Timothy Hatcher]

Created attachment 6200 [details]
Revised patch impelmentiaon (applies clean, needs fixes to build)

I have been working on getting this finally landed. However, since this suck an old patch I had to manually apply it to TOT and account for recent directory changes/renames. I am getting some build errors in the css grammar file and tokenizer but I don't know bison nor flex.

/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:528: invalid $ value
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:528: $4 of `media_list' has no declared type
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:530: invalid $ value
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:530: $4 of `media_list' has no declared type
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:541: invalid $ value
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:541: $3 of `media_list' has no declared type
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:541: invalid $ value
/Volumes/Data/Safari-TOT/OpenSource/WebCore/css/css_grammar.y:541: $6 of `media_list' has no declared type

css/tokenizer.flex: In member function 'int WebCore::CSSParser::lex()':
css/tokenizer.flex:38: error: 'MEDIA_NOT' was not declared in this scope
css/tokenizer.flex:39: error: 'MEDIA_ONLY' was not declared in this scope
css/tokenizer.flex:40: error: 'MEDIA_AND' was not declared in this scope
css/tokenizer.flex:48: error: 'BEGIN' was not declared in this scope
css/tokenizer.flex:50: error: 'BEGIN' was not declared in this scope
css/tokenizer.flex:57: error: 'BEGIN' was not declared in this scope
css/tokenizer.flex:57: error: 'KHTML_MEDIAQUERY_SYM' was not declared in this scope
css/tokenizer.flex:89: error: 'BEGIN' was not declared in this scope

If someone or Kimmo would like to take this patch and fix it up I will be more than happy to run the performance tests and land it ASAP.

[Timothy Hatcher]

Correction: "However, since this such an old patch..."

[Kimmo Kinnunen]

Created attachment 6220 [details]
Ad-hoc layout tests in the new directory layout

[Kimmo Kinnunen]

Created attachment 6221 [details]
Revised media queries patch

Freudian slip on your comments?-) 

Anyway, here's the patch revised. It adds some functionality to Screen.h, so new review should probably be done if this is wanted to be checked in.
Also the old one leaked, this ought not to..

[Darin Adler]

I think we can probably just have screenIsMonochrome return false for Mac OS.

On the other hand, the code here is probably fine. I think in practice the only color spaces used for gray-scale are NSCalibratedWhiteColorSpace and NSDeviceWhiteColorSpace.

If we want to try to do it right, then we should presumably get the NSColorSpaceModel and check for NSGrayColorSpaceModel. I don't see how to get from a color space name to an NSColorSpace object. Those objects are new for 10.4, but our minimum system requirement is 10.4.

One way that might work is to make an NSColor, then call -[NSColor colorUsingColorSpaceName:] on that NSColor, then call -[NSColor colorSpace] on the result of that, and then call -[NSColorSpace colorSpaceModel] on the result of that.

[Timothy Hatcher]

Comment on attachment 6221 [details]
Revised media queries patch

I think this solution for monochrome is fine. Performance tests are fine.

[Timothy Hatcher]

Landed in r12549.

[Maciej Stachowiak]

I rolled out this change and reopened the bug. The revised version was causing random crashes on the layout tests, see bug 7068.

[Darin Adler]

I noticed when looking over the patch that it has some examples of using an explicit delete on objects that are subclasses of TreeShared. Such objects should use ref/deref or RefPtr, and not explicit delete.

[Kimmo Kinnunen]

Created attachment 8019 [details]
Revised test cases for CSS Media Queries

[Kimmo Kinnunen]

Created attachment 8020 [details]
Revised CSS Media Queries patch

Yet another version of revised patch implementing CSS Media Queries, inspired by the blog post. Doesn't include ChangeLog entry.

The previous patch leaked and additionally crashed in Debug builds. I've tested this with Debug and Production builds, but it'd be really great if somebody more experienced with the tools would test the patch, especially for leaks. Of course the semantics could be tested aswell...

[Darin Adler]

Comment on attachment 8020 [details]
Revised CSS Media Queries patch

Nice work. I think it would be great to have this in the engine. Here are some things that need to be fixed before another round of review:

There are many tabs in this patch; please use spaces instead of tabs for formatting.

In the parser, we can't simply use new and delete. It's always possible for the parse to fail entirely. For that reason anything allocated during parsing must be owned by the parser and freed by the parser if it's never used. That's the reason for all the CSSParser functions like createMediaList and createFloatingFunction. Here are problematic cases:

+        $$ = new MediaQueryExp($3, $5);

The old strategy of trying to do the deletion in error cases in the parser didn't work. There are details of this in bug 7331 and the check-in I did on 2006-02-20.

Please use our new formatting in new code:

+         CSSParser *p = static_cast<CSSParser *>(parser);

That should be CSSParser* p = static_cast<CSSParser*>(parser).

+		$$ = getMediaFeatureID( str.lower().latin1(), str.length() );

And that should be getMediaFeatureID(str.lower().latin1(), str.length()) with no spaces. Also, please don't use DeprecatedString in new code unless you can't find any alternative.

Please use FIXME: instead of XXX: in comments that represent things that you think need to be investigated, and lets try to get rid of as many of those as possible before landing.

+        MediaQueryEvaluator allEval(true), screenEval("screen", true), printEval("print", true);

Please don't define multiple objects with a single statement.

+    if (string.isEmpty() || string.isNull()) {
+		return true;
+    }

We don't use braces around single statements like this, and there's no need to check for isNull() once you've checked for isEmpty() since all null strngs are also empty.

+#define BEGIN yy_start = 1 + 2 *

What's the reason for that line?

+    delete (m_value);

Please don't use parentheses for delete statements.

+        RenderStyle *defaultStyleForRoot(Element* e);

Should be:

        RenderStyle* defaultStyleForRoot(Element*);

What is the status of these?

+        $$->appendMediaQuery($1); //FIXME: domString($1).lower() );
+            $$->appendMediaQuery($4); //FIXME: domString($4) );

I'd like to see use of Vector<RefPtr<MediaQuery>> instead of DeprecatedPtrList<MediaQuery> for the list of queries. Also, I think that m_queries is a fine name, no need for "m_lst".

We can remove the "_NOT_ part of the DOM!" comment.

Perhaps we can use atomic strings instead of integers for the media features. I'd prefer not to add yet another special gperf file since we have a more-modern solution to such problems.

I didn't review the actual evaluator carefully enough, but I noticed a few things. First, we don't need braces around each case in a case statement. We only use braces when there's a variable declaration. Also, no need for "else" statements after return statements. A line like this:

+                return (isNumberValue &&  (int)numberValue == 0 );

should instead be

    return isNumberValue && numberValue == 0;

after removing extra parentheses, space, and unnecessary type cast.

The new structure is supposed to be a class per file, and the file named after the class. So the file should be named MediaQuery, not CSSMediaQuery. I'd suggest not even using a class for MediaQueryExpList; just Vector<MediaQueryExp> should do.

We also need to figure out how to test this. The patch should include both change log and test cases.

[Joost de Valk (AlthA)]

I'd really, really like to see this working :) Don't let this become an orphan patch, please :)

[Kimmo Kinnunen]

Created attachment 8508 [details]
Revised CSS Media Queries patch including testcases

The patch is revised taking into account Darin's comments
 - removed tabs and corrected whitespaces in ptrs and refs
 - use float-sink in CSSParser
 - reduced FIXMEs, removed XXXs
 - no multiple objects in single statement
 - removed redundant parentheses
 - one class per file, removed MediaQueryExpList class
 - using atomicstring+hashset function table instead of gperf
 - attached test cases to the patch (was in separate file previously)
 - added changelogs
 - removed usage of Deprecated* with the exception of DeprecatedStringList

Other things modified
 - more clear distinction between media descriptors and media types
 - small changes to SVG part
 - renamed testcases
 - make setting of media.mediaTest throw if syntax error
 - added few test cases

Like previously, it'd be great if somebody else than me could also try out the patch..

[Dave Hyatt]

Comment on attachment 8508 [details]
Revised CSS Media Queries patch including testcases

Great!  I have a few comments/suggestions:

(1) Rename adjustFont to updateFont.
(2) Rename matchUAAndPrintRules to matchUARules.
(3) The defaultStyleForRoot method feels really gross to me (the way it has to duplicate the style resolution process).  Is there no way to just compute this style during the normal process?

[Dave Hyatt]

e.g., maybe have a mode during styleForElement that will just exclude the author and user rules.

[Kimmo Kinnunen]

Created attachment 8549 [details]
Revised patch

Renamed functions and merged defaultStyleForRoot (back) to styleForElement

[Dave Hyatt]

Comment on attachment 8549 [details]
Revised patch

r=me.

To the person who commits this, here's some tests to run before landing:

(1) Performance test PLT and i-bench
(2) Run all the layout tests
(3) Test the Web Inspector and make sure the style pane still works, since that is the only real test of the style rule collection code path.

[Beth Dakin]

I performance tested this a bunch, Maciej fixed a performance regression the patch caused, and i landed it all as r14779.