[webkit-reviews] review denied: [Bug 39220] CSS3 Media Queries are not serialized according to CSSOM : [Attachment 56645] patch 1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Luiz Agostini
<luiz.agostini at openbossa.org>'s request for review:
Bug 39220: CSS3 Media Queries are not serialized according to CSSOM
https://bugs.webkit.org/show_bug.cgi?id=39220

Attachment 56645: patch 1
https://bugs.webkit.org/attachment.cgi?id=56645&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/css/MediaQuery.cpp:53
 +	return strcmp(a.utf8().data(), b.utf8().data()) < 0;
 This does 2 copies to make one compare.

WebCore/css/MediaQuery.cpp:58
 +	// http://dev.w3.org/csswg/cssom/#serialize-a-media-query
I might have put this before the function.

WebCore/css/MediaQuery.cpp:43
 +	    return "only ";
Seems strange that these include the space.

WebCore/css/MediaQuery.cpp:96
 +	    delete newExp; // delete duplicated
Huh?  This seems really confusing that we would delete the item passed to us in
this fuction.

WebCore/css/MediaQuery.cpp:109
 +	delete exprs;
Huh?  Also confusing.  Why are we taking ownership of this object?  If we are
we shoudl be using a PassOwnPtr to clarify that.

WebCore/css/MediaQuery.cpp:114
 +	deleteAllValues(m_expressions);
You removed setting m_expressions in teh constructor.  That seems bad.

WebCore/css/MediaQuery.cpp:120
 +	return serialize() == other.serialize();
This seems very expensive.

WebCore/css/MediaQuery.h:57
 +	iterator begin() { return m_expressions.begin(); }
Exposing iterators here seems strange to me.

r- for the above concerns.