Bug 39220 - CSS3 Media Queries are not serialized according to CSSOM
Summary: CSS3 Media Queries are not serialized according to CSSOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Luiz Agostini
URL:
Keywords:
Depends on: 39701
Blocks: 37205
  Show dependency treegraph
 
Reported: 2010-05-17 08:49 PDT by Kenneth Rohde Christiansen
Modified: 2010-06-11 07:26 PDT (History)
7 users (show)

See Also:


Attachments
Layout test showing the error (1.30 KB, text/html)
2010-05-17 08:49 PDT, Kenneth Rohde Christiansen
no flags Details
patch 1 (18.45 KB, patch)
2010-05-20 16:34 PDT, Luiz Agostini
eric: review-
Details | Formatted Diff | Diff
patch 2 (18.77 KB, patch)
2010-05-21 15:23 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 3 (18.76 KB, patch)
2010-05-21 15:43 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 4 (17.34 KB, patch)
2010-05-28 12:22 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 5 (17.29 KB, patch)
2010-05-28 13:52 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-05-17 08:49:58 PDT
Created attachment 56246 [details]
Layout test showing the error

Spec: http://dev.w3.org/csswg/cssom/#serializing-media-queries
Comment 1 Kenneth Rohde Christiansen 2010-05-17 08:50:47 PDT
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)
Comment 2 Antti Koivisto 2010-05-20 03:39:49 PDT
Doubt this is Qt specific, removing the keyword.
Comment 3 Kenneth Rohde Christiansen 2010-05-20 05:29:59 PDT
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 :-)
Comment 4 Luiz Agostini 2010-05-20 16:34:28 PDT
Created attachment 56645 [details]
patch 1
Comment 5 Eric Seidel (no email) 2010-05-21 12:22:11 PDT
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?
Comment 6 Eric Seidel (no email) 2010-05-21 12:27:24 PDT
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.
Comment 7 Luiz Agostini 2010-05-21 13:17:47 PDT
(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.
Comment 8 Luiz Agostini 2010-05-21 15:23:48 PDT
Created attachment 56752 [details]
patch 2
Comment 9 Luiz Agostini 2010-05-21 15:35:25 PDT
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. :)
Comment 10 Luiz Agostini 2010-05-21 15:43:32 PDT
Created attachment 56757 [details]
patch 3

spelling correction
Comment 11 Luiz Agostini 2010-05-21 22:14:11 PDT
> > 
> > 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.
Comment 12 Eric Seidel (no email) 2010-05-24 13:53:22 PDT
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?
Comment 13 Luiz Agostini 2010-05-28 12:22:55 PDT
Created attachment 57361 [details]
patch 4
Comment 14 Luiz Agostini 2010-05-28 12:27:05 PDT
(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.
Comment 15 WebKit Review Bot 2010-05-28 13:39:55 PDT
Attachment 57361 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2587035
Comment 16 Luiz Agostini 2010-05-28 13:52:11 PDT
Created attachment 57373 [details]
patch 5
Comment 17 Luiz Agostini 2010-06-09 07:57:14 PDT
ping review
Comment 18 Antti Koivisto 2010-06-10 07:46:11 PDT
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
Comment 19 WebKit Commit Bot 2010-06-10 08:21:00 PDT
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
Comment 20 WebKit Commit Bot 2010-06-11 07:25:55 PDT
Comment on attachment 57373 [details]
patch 5

Clearing flags on attachment: 57373

Committed r61016: <http://trac.webkit.org/changeset/61016>
Comment 21 WebKit Commit Bot 2010-06-11 07:26:05 PDT
All reviewed patches have been landed.  Closing bug.