Bug 39220 – CSS3 Media Queries are not serialized according to CSSOM

• Component: CSS
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

Created attachment 56246 [details]
Layout test showing the error

Spec: http://dev.w3.org/csswg/cssom/#serializing-media-queries

[Kenneth Rohde Christiansen]

The media query:

NOT braille, media and (orientation: 'landscape') AND (min-WIDTH:100px) and (max-width: 200px ), all and and (color) and (color).

is serialized to:

not braille, media and (orientation: landscape) and (min-width: 100px) and (max-width: 200px), all and (color) and (color)

but it should have been serialized to:

not braille, media and (orientation: landscape) and (max-width: 200px) and (min-width: 100px), (color)

[Antti Koivisto]

Doubt this is Qt specific, removing the keyword.

[Kenneth Rohde Christiansen]

I dont think the Qt keyword means that it is Qt specific (we normally prepend [Qt] to the title for that), but that it has direct relation to our Qt port, and thus will show up in our filters. Feel free to correct me :-)

[Luiz Agostini]

Created attachment 56645 [details]
patch 1

[Eric Seidel (no email)]

Comment on attachment 56645 [details]
patch 1

WebCore/css/MediaQuery.cpp:51
 +  static bool stringCompare(const String& a, const String& b)
Really?  This doesn't exist yet?

[Eric Seidel (no email)]

Comment on attachment 56645 [details]
patch 1

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.

[Luiz Agostini]

(In reply to comment #6)
> (From update of attachment 56645 [details])
> WebCore/css/MediaQuery.cpp:53
>  +      return strcmp(a.utf8().data(), b.utf8().data()) < 0;
>  This does 2 copies to make one compare.

indeed. I need a comparison method for String. I need to look for how to proceed.

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

ok

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

just a helper function. the space is just convenient. I will move the code to the main method.

> 
> 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.

We are taking the ownership.  I understand thate I must use PassOwnPtr in the parameter. Is it needed to use a container of OwnPtr to keep the objects? I mean: if I use a Vector of objects should it be a Vector<OwnPtr<> > or just Vector<>. Is any of them worse or wrong?

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

m_expressions was a pointer but was changed to be an object. no initialization is needed.

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

This is what is specified.

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

In previous implementation a Vector was used and a pointer to the internal vector was returned in method expressions() to be used outside the class.
I made several changes and I am now using a HashMap to keep the expressions. The best way to the class users to iterate over the expressions seems to me to be using an iterator over the values of the HashMap.

> 
> r- for the above concerns.

[Luiz Agostini]

Created attachment 56752 [details]
patch 2

[Luiz Agostini]

Is it needed to use a container of OwnPtr to keep the objects? I mean: if I use a Vector of objects should it be a Vector<OwnPtr<> > or just Vector<>. Is any of them worse or wrong?

Really bad question. I should look at the code before asking. :)

[Luiz Agostini]

Created attachment 56757 [details]
patch 3

spelling correction

[Luiz Agostini]

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

If we think that this operator is hot I could keep a cache of the computed String. It is probably not needed.

> 
> > 
> > WebCore/css/MediaQuery.h:57
> >  +      iterator begin() { return m_expressions.begin(); }
> > Exposing iterators here seems strange to me.
> 
> In previous implementation a Vector was used and a pointer to the internal vector was returned in method expressions() to be used outside the class.
> I made several changes and I am now using a HashMap to keep the expressions. The best way to the class users to iterate over the expressions seems to me to be using an iterator over the values of the HashMap.
> 

I should be using const_iterator anyway. I will wait for further comments before submitting this change.

[Eric Seidel (no email)]

Comment on attachment 56757 [details]
patch 3

LayoutTests/fast/media/media-query-serialization.html:21
 +      function shouldBe(desc, a, b)
Why invent your own shouldBe and log() instead of making this a script-test?

WebCore/css/MediaQuery.cpp:43
 +      return strcmp(a.utf8().data(), b.utf8().data()) < 0;
This is very expensive because it copies.  it would be better to walk the characters().  I would write a new asciiOnlyCompare() function which does just that in String.h if I were writing this.

WebCore/css/MediaQuery.cpp:58
 +      default:
Much better to list out all possible enum values than use default: so that the compiler can let you know when you add an enum value and need to update your switch statements.

WebCore/css/MediaQuery.cpp:94
 +      if (m_expressions.add(key, newExp.get()).second)
Why not call .release() instead of .get()?  I'm confused under which conditions we don't want to release?

WebCore/css/MediaQuery.cpp:95
 +          newExp.release();
Should MediaQueryExp be ref counted?

WebCore/css/MediaQuery.cpp:118
 +      return serialize() == other.serialize();
Probably should add a comment that this expensive compare is done to match the spec.

WebCore/css/MediaQuery.h:58
 +      iterator begin() { return m_expressions.begin(); }
I think it's cleaner to have a:
const Vector<MediaQueryExp*>& expressions() const; method, no?

Why would these iterators be mutable?

WebCore/css/MediaQueryExp.h:69
 +      String serialize();
Should this be const?

Should we be caching any of these serializations?

[Luiz Agostini]

Created attachment 57361 [details]
patch 4

[Luiz Agostini]

(In reply to comment #12)
> (From update of attachment 56757 [details])
> LayoutTests/fast/media/media-query-serialization.html:21
>  +      function shouldBe(desc, a, b)
> Why invent your own shouldBe and log() instead of making this a script-test?

done

> 
> WebCore/css/MediaQuery.cpp:43
>  +      return strcmp(a.utf8().data(), b.utf8().data()) < 0;
> This is very expensive because it copies.  it would be better to walk the characters().  I would write a new asciiOnlyCompare() function which does just that in String.h if I were writing this.

I have been working on it (bug 39701) and it is finally landed.

done

> 
> WebCore/css/MediaQuery.cpp:58
>  +      default:
> Much better to list out all possible enum values than use default: so that the compiler can let you know when you add an enum value and need to update your switch statements.

done

> 
> WebCore/css/MediaQuery.cpp:94
>  +      if (m_expressions.add(key, newExp.get()).second)
> Why not call .release() instead of .get()?  I'm confused under which conditions we don't want to release?

code is different in this latest patch.

> 
> WebCore/css/MediaQuery.cpp:95
>  +          newExp.release();
> Should MediaQueryExp be ref counted?

No need for that.

> 
> WebCore/css/MediaQuery.cpp:118
>  +      return serialize() == other.serialize();
> Probably should add a comment that this expensive compare is done to match the spec.

serialization is now cached so that just the first call is expensive.

> 
> WebCore/css/MediaQuery.h:58
>  +      iterator begin() { return m_expressions.begin(); }
> I think it's cleaner to have a:
> const Vector<MediaQueryExp*>& expressions() const; method, no?

ok. if you prefer.

> 
> Why would these iterators be mutable?

they should not. :)

> 
> WebCore/css/MediaQueryExp.h:69
>  +      String serialize();
> Should this be const?

done

> 
> Should we be caching any of these serializations?

all serializations are now cached.

[WebKit Review Bot]

Attachment 57361 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2587035

[Luiz Agostini]

Created attachment 57373 [details]
patch 5

[Luiz Agostini]

ping review

[Antti Koivisto]

Comment on attachment 57373 [details]
patch 5

r=me, minor comments:

m_serializationCache should be mutable that avoids the const_cast
m_valid bit might be clearer than m_ignored bit

[WebKit Commit Bot]

Comment on attachment 57373 [details]
patch 5

Rejecting patch 57373 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
ORT_DIR /Developer/Library/Xcode
    setenv XCODE_VERSION_ACTUAL 0313
    setenv XCODE_VERSION_MAJOR 0300
    setenv XCODE_VERSION_MINOR 0310
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Ld /Users/eseidel/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal i386
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/3190154

[WebKit Commit Bot]

Comment on attachment 57373 [details]
patch 5

Clearing flags on attachment: 57373

Committed r61016: <http://trac.webkit.org/changeset/61016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.