[webkit-reviews] review denied: [Bug 4127] WebCore doesn't support Media Queries (CSS3 module) : [Attachment 8020] Revised CSS Media Queries patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 4127: WebCore doesn't support Media Queries (CSS3 module)
http://bugzilla.opendarwin.org/show_bug.cgi?id=4127

Attachment 8020: Revised CSS Media Queries patch
http://bugzilla.opendarwin.org/attachment.cgi?id=8020&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.