A thread-safe Media Query Parser

A thread-safe Media Query Parser

Implement a thread-safe Media Query parser based on eseidel's tokenizer (https://codereview.chromium.org/123053002/).

This CL is composed of 2 parts:
* Rendering the MediaQuery related classes thread-safe, by eliminating AtomicStrings and replacing them with hash comparisons.
* Replacing BisonCSSParser for a new parser for MQ parsing.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169289

[eseidel, 2014-02-18 23:24:01]

https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
Source/core/css/MediaQueryExp.cpp:163: return mediaFeatureHash ==
MediaFeatureNames::monochromeMediaFeature.impl()->existingHash()
Why the fake hash-table? :)

https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
Source/core/css/MediaQueryExp.cpp:187: return m_mediaFeatureHash ==
MediaFeatureNames::widthMediaFeature.impl()->existingHash()
At some point abarth talked about making a StaticSet which could handle things
like this.

I think we should consider "using MediaFeatureNames;" for this file given how
often we type this.  I would do that first in a separate CL of course.

https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
File Source/core/css/parser/MediaQueryParser.h (right):

https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
Source/core/css/parser/MediaQueryParser.h:41: Vector<CSSToken> m_tokens;
It turns out we already sort have a CSSToken in our current code, it's just
called CSSParserValueList and CSSParserValue.  I think the lifetime of these
objects is slightly complicated due to the silly float/sink stuff for bison, so
we may end up creating a new CSSToken type anyway.

https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
File Source/core/css/parser/MediaQueryParserTest.cpp (right):

https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
Source/core/css/parser/MediaQueryParserTest.cpp:20:
testCases.append(std::make_pair("screen", ""));
I might use a static helper method for this sort of thing.

addTest(testCases, "screen and (color)", "");

Or even struct initialization:

struct TestCase {
   const char* input;
   const char* output;
}

TestCase cases[] = {
  {"screen and color", ""},
};

But I try real hard not to type any more characters than needed, since reading
them again is hard. :)

[Yoav Weiss, 2014-02-19 08:26:19]

On 2014/02/18 23:24:01, eseidel wrote:
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
> Source/core/css/MediaQueryExp.cpp:163: return mediaFeatureHash ==
> MediaFeatureNames::monochromeMediaFeature.impl()->existingHash()
> Why the fake hash-table? :)

The goal of the hash comparison was to avoid AtomicString without losing the
performance benefits of AtomicString comparison.

> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryEx...
> Source/core/css/MediaQueryExp.cpp:187: return m_mediaFeatureHash ==
> MediaFeatureNames::widthMediaFeature.impl()->existingHash()
> At some point abarth talked about making a StaticSet which could handle things
> like this.

I agree that a bloom filter based StaticSet can help weed out the comparisons
that turn out false here (which is probably most of them in many cases).
Can this optimization wait for a later CL?

> 
> I think we should consider "using MediaFeatureNames;" for this file given how
> often we type this.  I would do that first in a separate CL of course.
> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
> File Source/core/css/parser/MediaQueryParser.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
> Source/core/css/parser/MediaQueryParser.h:41: Vector<CSSToken> m_tokens;
> It turns out we already sort have a CSSToken in our current code, it's just
> called CSSParserValueList and CSSParserValue.  I think the lifetime of these
> objects is slightly complicated due to the silly float/sink stuff for bison,
so
> we may end up creating a new CSSToken type anyway.
> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
> File Source/core/css/parser/MediaQueryParserTest.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/1/Source/core/css/parser/Media...
> Source/core/css/parser/MediaQueryParserTest.cpp:20:
> testCases.append(std::make_pair("screen", ""));
> I might use a static helper method for this sort of thing.
> 
> addTest(testCases, "screen and (color)", "");
> 
> Or even struct initialization:
> 
> struct TestCase {
>    const char* input;
>    const char* output;
> }
> 
> TestCase cases[] = {
>   {"screen and color", ""},
> };

I like that! :)

> 
> But I try real hard not to type any more characters than needed, since reading
> them again is hard. :)

All in all, I now think I can split this CL into 2:
* One that gets rid of AtomicString in Media Query related classes.
* One that adds the parser and tokenizer, which may take longer to pass tests
and be 100% correct.

[Yoav Weiss, 2014-02-25 22:50:34]

I've uploaded a new patch that is refactored, passes tests, and should be more
efficient. All in all, I think it's ready for review.

[Yoav Weiss, 2014-02-26 10:00:35]

On 2014/02/25 22:50:34, Yoav Weiss wrote:
> I've uploaded a new patch that is refactored, passes tests, and should be more
> efficient. All in all, I think it's ready for review.

On second thought, I'm not happy with the hash based mediaType & mediaFeature
comparisons.
I think it can move to use CSSKeywordValues based IDs.

If that makes sense, please let me know, and avoid reviewing these parts of the
code.
Also let me know if there's value in splitting the CL into two separate ones.

[eseidel, 2014-02-26 10:18:41]

https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:48: return mediaFeatureHash ==
MediaFeatureNames::orientationMediaFeature.impl()->existingHash()
Consider writing an hashEquals() helper which takes a String and a hash and
makes these lines shorter. :)

hashEquals(orientationMediaFeature, featureHash)

[Yoav Weiss, 2014-02-26 23:16:27]

On 2014/02/26 10:18:41, eseidel wrote:
>
https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryExp.cpp:48: return mediaFeatureHash ==
> MediaFeatureNames::orientationMediaFeature.impl()->existingHash()
> Consider writing an hashEquals() helper which takes a String and a hash and
> makes these lines shorter. :)
> 
> hashEquals(orientationMediaFeature, featureHash)

Done.

Also removed the CSSValueID related functions out of BisonCSSParser and into
their own file.

I've abandoned my plans to move from static strings and hash comparisons to CSS
IDs for the media types and features after realizing that it would cause unknown
types and features to be lost (rather than current static strings solution,
where they would just be represented with regular strings, which is probably OK,
since they're probably rare)

[abarth-chromium, 2014-03-01 07:28:24]

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
File Source/core/css/CSSValueIDHelper.cpp (right):

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
Source/core/css/CSSValueIDHelper.cpp:2: //
ditto

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
File Source/core/css/CSSValueIDHelper.h (right):

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
Source/core/css/CSSValueIDHelper.h:2: //
I think this line isn't supposed to be here.

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaFe...
File Source/core/css/MediaFeatureNames.h (right):

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaFe...
Source/core/css/MediaFeatureNames.h:76: #define
CSS_MEDIAQUERY_NAMES_DECLARE(name, str) extern const String name##MediaFeature;
Can you change this to be MediaFeatureNames.in and use make_names.py?  That's a
better pattern and will make thread-safe strings for you.

Take a look at how FontFamilyNames.in integrates with the build system to see
how that works.

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
File Source/core/css/MediaQuery.cpp (right):

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
Source/core/css/MediaQuery.cpp:86: m_mediaType =
attemptStaticStringCreation(lowerMediaType, Force16Bit);
Then you won't need this sort of thing.

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
File Source/core/css/MediaQueryExp.cpp (right):

https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
Source/core/css/MediaQueryExp.cpp:57: || hashEquals(mediaFeatureHash,
scanMediaFeature);
Where do these strings come from?  We can make them StaticStrings too using
make_names.py and then you won't need to do all this work.

[Yoav Weiss, 2014-03-03 16:15:42]

On 2014/03/01 07:28:24, abarth wrote:
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
> File Source/core/css/CSSValueIDHelper.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
> Source/core/css/CSSValueIDHelper.cpp:2: //
> ditto
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
> File Source/core/css/CSSValueIDHelper.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValu...
> Source/core/css/CSSValueIDHelper.h:2: //
> I think this line isn't supposed to be here.
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaFe...
> File Source/core/css/MediaFeatureNames.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaFe...
> Source/core/css/MediaFeatureNames.h:76: #define
> CSS_MEDIAQUERY_NAMES_DECLARE(name, str) extern const String
name##MediaFeature;
> Can you change this to be MediaFeatureNames.in and use make_names.py?  That's
a
> better pattern and will make thread-safe strings for you.
> 
> Take a look at how FontFamilyNames.in integrates with the build system to see
> how that works.
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
> File Source/core/css/MediaQuery.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
> Source/core/css/MediaQuery.cpp:86: m_mediaType =
> attemptStaticStringCreation(lowerMediaType, Force16Bit);
> Then you won't need this sort of thing.
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
> File Source/core/css/MediaQueryExp.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/MediaQu...
> Source/core/css/MediaQueryExp.cpp:57: || hashEquals(mediaFeatureHash,
> scanMediaFeature);
> Where do these strings come from?  We can make them StaticStrings too using
> make_names.py and then you won't need to do all this work.

Done. I've created MediaFeatureNames.in and MediaTypeNames.in that create all
these strings statically.
I've also modified attemptStaticStringCreation to accept both 8bit and 16bit
strings.

[abarth-chromium, 2014-03-03 19:06:18]

Can we break this change into smaller pieces?  For example, we can land the "in"
file conversion first and the thread-safety parts second?  There's too much
stuff going on in this CL.

https://codereview.chromium.org/171383002/diff/350001/Source/core/core_genera...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/171383002/diff/350001/Source/core/core_genera...
Source/core/core_generated.gyp:204: '--gperf', '<(gperf_exe)',
Why do you need gperf?  That doesn't make much sense.

https://codereview.chromium.org/171383002/diff/350001/Source/core/css/MediaFe...
File Source/core/css/MediaFeatures.h (right):

https://codereview.chromium.org/171383002/diff/350001/Source/core/css/MediaFe...
Source/core/css/MediaFeatures.h:24: #define
CSS_MEDIAQUERY_NAMES_FOR_EACH_MEDIAFEATURE(macro) \
Why is this needed?  It's lame that we have to repeat all these names in this
file.  Should we generate this file from the "in" file as well?

[Yoav Weiss, 2014-03-03 23:07:16]

On 2014/03/03 19:06:18, abarth wrote:
> Can we break this change into smaller pieces?  For example, we can land the
"in"
> file conversion first and the thread-safety parts second?  There's too much
> stuff going on in this CL.

I'll split the .in files part to their own CL.

> 
>
https://codereview.chromium.org/171383002/diff/350001/Source/core/core_genera...
> File Source/core/core_generated.gyp (right):
> 
>
https://codereview.chromium.org/171383002/diff/350001/Source/core/core_genera...
> Source/core/core_generated.gyp:204: '--gperf', '<(gperf_exe)',
> Why do you need gperf?  That doesn't make much sense.

I probably don't. I'll clean that up.

> 
>
https://codereview.chromium.org/171383002/diff/350001/Source/core/css/MediaFe...
> File Source/core/css/MediaFeatures.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/350001/Source/core/css/MediaFe...
> Source/core/css/MediaFeatures.h:24: #define
> CSS_MEDIAQUERY_NAMES_FOR_EACH_MEDIAFEATURE(macro) \
> Why is this needed?  

MediaQueryEvaluator needs this macro to convert from incoming media feature name
strings to the appropriate eval function. 

> It's lame that we have to repeat all these names in this
> file.  Should we generate this file from the "in" file as well?

I can probably add it as an output to MediaFeatureNames.h, or create a second
output file from MediaFeatureNames.in, generating this macro.

[Yoav Weiss, 2014-03-07 05:23:44]

I've split 2 CLs from this one. Both have landed:
* Static string generation - https://codereview.chromium.org/185533012/
* MQ classes thread safety - https://codereview.chromium.org/178803006/

The remaining code is the MQ parser itself.

[abarth-chromium, 2014-03-07 05:58:03]

Is this based on Eric's CL?  It would probably make sense for Eric to take a
look at this CL.  I can look in more detail afterwards if that makes sense.

https://codereview.chromium.org/171383002/diff/370001/Source/core/css/parser/...
File Source/core/css/parser/CSSToken.h (right):

https://codereview.chromium.org/171383002/diff/370001/Source/core/css/parser/...
Source/core/css/parser/CSSToken.h:29: */
pls use a new-style copyright header

[Yoav Weiss, 2014-03-07 06:04:41]

On 2014/03/07 05:58:03, abarth wrote:
> Is this based on Eric's CL?  It would probably make sense for Eric to take a
> look at this CL.  I can look in more detail afterwards if that makes sense.
> 
>
https://codereview.chromium.org/171383002/diff/370001/Source/core/css/parser/...
> File Source/core/css/parser/CSSToken.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/370001/Source/core/css/parser/...
> Source/core/css/parser/CSSToken.h:29: */
> pls use a new-style copyright header

The tokenizer, input stream and CSSTokens are based on Eric's CL, so I agree
that he should probably review it as well.

I used the same copyright header as Eric had in his CL, but I can switch to the
new style.

[Yoav Weiss, 2014-03-08 10:18:23]

As discussed with Eric on IRC, I've renamed the CSS* classes to be MediaQuery*
classes, to signify that this is not a generic solution, and removed unused
tokens.
If there's any other unused code that I've missed, please let me know.

[eseidel, 2014-03-08 22:08:07]

I need to read this a couple more times, it's complicated. :)  I'll make sure
it's reviewed by EOD Monday.

https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryInputStream.h (right):

https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.h:12: // The CSS Syntax spec is
written as a (fixed size) look-ahead
Please remove this.

https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:20: // FIXME: Replace the
MediaQueryTokenizer with a generic CSSTokenizer, once there is one,
Fantatisc, thank you for highlighting this.

[Yoav Weiss, 2014-03-08 22:29:38]

On 2014/03/08 22:08:07, eseidel wrote:
> I need to read this a couple more times, it's complicated. :)  I'll make sure
> it's reviewed by EOD Monday.

Awesome! Thanks

> 
>
https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryInputStream.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryInputStream.h:12: // The CSS Syntax spec is
> written as a (fixed size) look-ahead
> Please remove this.

Done.

> 
>
https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryParser.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/430001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryParser.cpp:20: // FIXME: Replace the
> MediaQueryTokenizer with a generic CSSTokenizer, once there is one,
> Fantatisc, thank you for highlighting this.

[kenneth.r.christiansen, 2014-03-08 22:37:46]

A few mini comments while skimming through the patch

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.h (right):

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:85: void
skipTillParen(MediaQueryTokenType, TokenIterator&);
parenthesis?

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryToken.cpp (right):

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryToken.cpp:46: static UnitTable* unitTable = new
UnitTable(createUnitTable());
Why not use the common macro for this?

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.cpp (right):

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.cpp:81: return
MediaQueryToken(DelimToken, cc);
DelimiterToken why not write it out, it is quite short

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.h (right):

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:36: void
consumeUntilNotWhitespace();
non ?

You terminology mixes until and till

https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:81: CodePoints* getCodePoints()
why not just codePoints()?

[Yoav Weiss, 2014-03-10 07:47:33]

On 2014/03/08 22:37:46, kenneth.r.christiansen wrote:
> A few mini comments while skimming through the patch
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryParser.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryParser.h:85: void
> skipTillParen(MediaQueryTokenType, TokenIterator&);
> parenthesis?
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryToken.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryToken.cpp:46: static UnitTable* unitTable =
new
> UnitTable(createUnitTable());
> Why not use the common macro for this?
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryTokenizer.cpp (right):
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryTokenizer.cpp:81: return
> MediaQueryToken(DelimToken, cc);
> DelimiterToken why not write it out, it is quite short
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> File Source/core/css/parser/MediaQueryTokenizer.h (right):
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryTokenizer.h:36: void
> consumeUntilNotWhitespace();
> non ?
> 
> You terminology mixes until and till
> 
>
https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryTokenizer.h:81: CodePoints* getCodePoints()
> why not just codePoints()?

Modified according to the comments above.

[eseidel, 2014-03-11 20:14:18]

Lots of little comments.  Sorry to be so slow.  Will be faster next time.

What you're attempting to do is difficult.  This may take a few rounds, but I'm
happy to review much quicker now that I've paged this in.  Please don't hesitate
to ping me regularly.  I intend to review your patches within a day of you
posting them now.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryInputStream.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.cpp:20: if (m_offset >=
m_string.length())
Same here.  Looks like thsi could be implemented in terms of peek?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.cpp:27: if ((m_offset +
lookaheadOffset) >= m_string.length())
This is a bit odd.  EOF should only be length + 1, anything else should ASSERT,
IMO.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.cpp:34: ASSERT(m_offset + offset <=
m_string.length() + 1);
Maybe we need a lengthIncludingEOF() helper, or just a length() helper which
returns m_string.length() + 1?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:22:
MediaQueryParser::MediaQueryParser(String queryString)
I think String is normally passed const String&?  It's basically just
RefPtr<StringImpl> :)

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:37: m_state =
&MediaQueryParser::readFeature;
Intersting that you're using function pointers as state.  I might try making
this more readable by using some helper private consts like:

const StateFunction ReadFeature = &readFeature;

Then you can write:

m_state = ReadFeature;

or

m_state = kReadFeature;

or 

m_state = ReadFeatureState;

and it might read easier, since you type these a lot.  The cost is obviously
some boiler-plate in the header or top of the .cpp file, but I suspect it will
be easier on the eyes than the current code. :)

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:39: if (m_state ==
&MediaQueryParser::readRestrictor && token->value().lower() == "not") {
We have case-insensitive comparison functions.

equalIgnoringCase(token->value(), "not");

You can even make your own helper which takes a token if you type this a lot:

equalIgnoringCase(token, "not");

Or even:
token->hasValue("not")

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:40:
m_mediaQueryData.setRestrictor(MediaQuery::Not);
If these are often tied, you might add a helper method:

setStateAndRestrict(ReadMediaTypeState, MediaQuery::Not);

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:155: ((this)->*(m_state))(type,
token);
I might have moved this loop body into a helper function:

handleToken(token) or processToken(token);

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:170: , m_expressions(adoptPtr(new
ExpressionHeapVector()))
ExpressionHeapVector::create() is preferred if possible.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:175: void
MediaQueryData::resetCurrentQuery()
Does this store more than the "current query"?  Should the "current query" be a
separate object which we just do:

m_currentQuery = FooBar();

to clear?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:187:
m_querySet->addMediaQuery(adoptPtr(new MediaQuery(m_restrictor, m_mediaType,
m_expressions.release())));
::create() is better when possible, for hiding the adoptPtr stuff.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:188: resetCurrentQuery();
Should this instead be:

m_querySet->addMediaQuery(takeCurrentQuery());?

"take" is a common name for "copy out" in this sense.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:193: OwnPtr<MediaQueryExp>
expression = MediaQueryExp::create(m_mediaFeature, &(m_valueList));
I don't think you need hte inner ()

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:194: bool retValue =
(expression.get());
Or here. !! works too.

bool hadExpression = !!expression;

Except... this can't ever be null?  Unless create can return null!?  Which would
not be normal for create() to do!

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:200: void
MediaQueryData::addParserValue(MediaQueryTokenType type, MediaQueryToken* token)
If Token can't ever be null in these functions it should be a reference.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:210: CSSParserFunction* function =
new CSSParserFunction;
Please use OwnPtr.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.h (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:38: inline
PassRefPtrWillBeRawPtr<MediaQuerySet> querySet()
Why is this a PassRefPtr?  We hold m_querySet, no one else is taking ownership
of it...

Also, const?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:43: inline unsigned querySetSize()
If the above returns a const& or a *, then do we need a size accessor, or should
callers just do data.querySet().size()?

Also, const?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:48: inline bool
currentMediaQueryChanged()
const?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:55:
m_querySet->addMediaQuery(adoptPtr(new MediaQuery(MediaQuery::Not, "all",
nullptr)));
I would split out MediaQuery::createNotAll() and then do you need this add
method?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryToken.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryToken.cpp:18: table.set(String("em"),
CSSPrimitiveValue::CSS_EMS);
Should this funtion be on CSSPrimtiveValue?  If we add a type to
CSSPrimitiveValue should it automatically work here?

CSSPritiveValue::allTypes() or some such?  We do something like this for static
strings, etc.

This is static data, we shouldn't need to allocate it on the heap.

Unless we do these lookups a lot, we can suffer the O(N) lookup cost and just
use a flat table isntead of a hash.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.h (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:12: #define CODE_POINTS_NUM 128
What is this for?  Can it be a const instead of a macro?

This is just the size of a signed char, no?  sizeof(char)?  sizeof(LChar)?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:54: memset(codePoints, 0,
CODE_POINTS_NUM);
This could be static and stored in the binary instead of allocated on the heap,
no?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:55: codePoints['\n'] =
&MediaQueryTokenizer::whiteSpace;
Why are we using a jump-table instead of an if-cascade?

I guess it doens't really matter one way or the other.  I don't think this
should be in the header.

[Yoav Weiss, 2014-03-12 20:39:00]

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:37: m_state =
&MediaQueryParser::readFeature;
On 2014/03/11 20:14:19, eseidel wrote:
> Intersting that you're using function pointers as state.  I might try making
> this more readable by using some helper private consts like:
> 
> const StateFunction ReadFeature = &readFeature;
> 
> Then you can write:
> 
> m_state = ReadFeature;
> 
> or
> 
> m_state = kReadFeature;
> 
> or 
> 
> m_state = ReadFeatureState;
> 
> and it might read easier, since you type these a lot.  The cost is obviously
> some boiler-plate in the header or top of the .cpp file, but I suspect it will
> be easier on the eyes than the current code. :)

I like that :)

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:170: , m_expressions(adoptPtr(new
ExpressionHeapVector()))
On 2014/03/11 20:14:19, eseidel wrote:
> ExpressionHeapVector::create() is preferred if possible.

ExpressionHeapVector is a typedef for a Vector. Should I add "create" on Vector?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:175: void
MediaQueryData::resetCurrentQuery()
On 2014/03/11 20:14:19, eseidel wrote:
> Does this store more than the "current query"?  Should the "current query" be
a
> separate object which we just do:
> 
> m_currentQuery = FooBar();
> 
> to clear?

MediaQueryData also contains m_querySet that we don't want to clear here. I can
add a "current query" object here if needed. Or maybe take querySet out of
MediaQueryData, so that the rest can be easily cleared.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:194: bool retValue =
(expression.get());
On 2014/03/11 20:14:19, eseidel wrote:
> Or here. !! works too.
> 
> bool hadExpression = !!expression;
> 
> Except... this can't ever be null?  Unless create can return null!?  Which
would
> not be normal for create() to do!

MediaQueryExp::create returns nullptr when the expression is not a valid MQ
expression

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:210: CSSParserFunction* function =
new CSSParserFunction;
On 2014/03/11 20:14:19, eseidel wrote:
> Please use OwnPtr.

The owner of the CSSParserFunction needs to be the CSSParserValue. I'm not sure
what's the best way to do that, but I think that OwnPtr here would cause later
references to CSSParserValue to reference a destructed pointer.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.h (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.h:38: inline
PassRefPtrWillBeRawPtr<MediaQuerySet> querySet()
On 2014/03/11 20:14:19, eseidel wrote:
> Why is this a PassRefPtr?  We hold m_querySet, no one else is taking ownership
> of it...

MediaQueryParser::parseImpl is returning it to the caller, so AFAIU PassRefPtr
is appropriate. I may be wrong though.

> 
> Also, const?

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryToken.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryToken.cpp:18: table.set(String("em"),
CSSPrimitiveValue::CSS_EMS);
On 2014/03/11 20:14:19, eseidel wrote:
> Should this funtion be on CSSPrimtiveValue?  If we add a type to
> CSSPrimitiveValue should it automatically work here?
> 
> CSSPritiveValue::allTypes() or some such?  We do something like this for
static
> strings, etc.
> 
> This is static data, we shouldn't need to allocate it on the heap.

Maybe the best way to do that is through some CSSUnits.in and the build scripts
that'd create this table on Init? What do you think?

> 
> Unless we do these lookups a lot, we can suffer the O(N) lookup cost and just
> use a flat table isntead of a hash.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.h (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:54: memset(codePoints, 0,
CODE_POINTS_NUM);
On 2014/03/11 20:14:19, eseidel wrote:
> This could be static and stored in the binary instead of allocated on the
heap,
> no?

I'm not sure how to do that. I'd love some guidance here.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:55: codePoints['\n'] =
&MediaQueryTokenizer::whiteSpace;
On 2014/03/11 20:14:19, eseidel wrote:
> Why are we using a jump-table instead of an if-cascade?
> 
> I guess it doens't really matter one way or the other.  I don't think this
> should be in the header.

I implemented it as a table following a comment by abarth on the original CL
(that a lookup table will be faster).

[eseidel, 2014-03-13 00:49:35]

Will review the new patch shortly.

One goal here is to figure out what this should look like and separate that from
what it will look like on the first CL.  We can always file bugs and then add //
FIXMEs to this CL linking to those bugs.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryToken.cpp (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryToken.cpp:18: table.set(String("em"),
CSSPrimitiveValue::CSS_EMS);
> Maybe the best way to do that is through some CSSUnits.in and the build
scripts
> that'd create this table on Init? What do you think?

OK.  Just file a bug and table that for a later CL then. :)

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.h (right):

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:54: memset(codePoints, 0,
CODE_POINTS_NUM);
On 2014/03/12 20:39:00, Yoav Weiss wrote:
> On 2014/03/11 20:14:19, eseidel wrote:
> > This could be static and stored in the binary instead of allocated on the
> heap,
> > no?
> 
> I'm not sure how to do that. I'd love some guidance here.

You'd probably have to do it with static initializers:

CodePoint codePoints[128] {
  whiteSpace, // null
  whiteSpace, // start heading
  leftParen, // '('
  .. for all the ascii chars.
};

It would be tedious, and again may be best deferred until a later CL.  But the
current implementation will allocate a fresh copy of this array for each render
process, which is convenient, but needless.

https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.h:55: codePoints['\n'] =
&MediaQueryTokenizer::whiteSpace;
On 2014/03/12 20:39:00, Yoav Weiss wrote:
> On 2014/03/11 20:14:19, eseidel wrote:
> > Why are we using a jump-table instead of an if-cascade?
> > 
> > I guess it doens't really matter one way or the other.  I don't think this
> > should be in the header.
> 
> I implemented it as a table following a comment by abarth on the original CL
> (that a lookup table will be faster).

I'm told that modern compilers are able to convert switches as well as
if-cascades into lookup tables when needed, but I could also be putting waaay
too much faith in the compiler. :)

[Yoav Weiss, 2014-03-13 10:50:40]

> 
>
https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryParser.cpp:175: void
> MediaQueryData::resetCurrentQuery()
> On 2014/03/11 20:14:19, eseidel wrote:
> > Does this store more than the "current query"?  Should the "current query"
be
> a
> > separate object which we just do:
> > 
> > m_currentQuery = FooBar();
> > 
> > to clear?
> 

Actually, this requires an explicit assignment constructor and gets overly
complicated in g++-4.6. I suspect it's related to the lack of rvalue references,
and the fact that ExpressionHeapVector contains OwnPtrs for the expressions it
contains.
I'll try to change this to a clear function that's as simple as possible.

[eseidel, 2014-03-13 17:22:06]

I had these drafts left over from last night.  Looking at your newer patch now.

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/MediaQu...
File Source/core/css/MediaQuery.cpp (right):

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/MediaQu...
Source/core/css/MediaQuery.cpp:80: return adoptPtrWillBeNoop(new
MediaQuery(MediaQuery::Not, "all", nullptr));
The primary function of ::create() functions is to wrap the adoptPtr so that
it's not forgotten (and so that it doesn't have to be typed at each caller). 
Thanks for adding this.

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryInputStream.cpp (right):

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.cpp:48: if (isResultOK)
nit: I might have written this as a ternary at the end.
return isResultOK ? result : 0;

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.cpp:61: result =
charactersToDouble(m_string.characters8() + m_offset + start, end - start,
&isResultOK);
Sigh.  If we had a StringView class then the caller would just do this:
inputStream.stringView(start, end).toDouble(isOK);
One of these years we'll add something like that.

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryInputStream.h (right):

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryInputStream.h:32: unsigned long long
getUInt(unsigned, unsigned);
I think these need named arguments so we can tell what they are. :)

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:17: return parser.parseImpl();
nit: This could even be one line: return
MediaQueryParser(queryString).parseImpl();

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:46: if (m_state == ReadRestrictor &&
equalIgnoringCase(token->value(), "not")) {
Does this compile?  token.value() no?

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:55: m_state = Done;
This reads much nicer, thanks.

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:167:
m_querySet->addMediaQuery(m_mediaQueryData.takeMediaQueryAndClear());
I think "take" implies "clear" for most readers.  It's in contrast to "get"
which just shows you something, where as "take" you take ownership of it, but
it's also ok to leave the "andClear" if it reads better to you. :)

https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:175: , m_expressions(adoptPtr(new
ExpressionHeapVector()))
Is adoptPtr even needed with OwnPtr?

[Yoav Weiss, 2014-03-13 17:49:35]

Thanks! I'll fix the nits and upload them.

On 2014/03/13 17:22:06, eseidel wrote:

> 
>
https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryParser.cpp:46: if (m_state == ReadRestrictor
&&
> equalIgnoringCase(token->value(), "not")) {
> Does this compile?  token.value() no?

token is a TokenIterator so *token is a MediaQueryToken. So it really should be
token->value().
The reason I'm passing the iterator around is that some of the funcs need to
decrement it.

 
>
https://codereview.chromium.org/171383002/diff/510001/Source/core/css/parser/...
> Source/core/css/parser/MediaQueryParser.cpp:175: , m_expressions(adoptPtr(new
> ExpressionHeapVector()))
> Is adoptPtr even needed with OwnPtr?

It fails to compile for me without adoptPtr...

[eseidel, 2014-03-13 17:58:43]

Honestly, this lgtm.  I think we have some outstanding nits from earlier passes,
but I think this is very close to ready for landing.

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:25: ,
ReadRestrictor(&MediaQueryParser::readRestrictor)
I'm surprised these can't just be static?  Either statics in this file, or on
the class?  Obviously we won't have many MediaQueryParsers, but surprised they
need to carry these as instance variables.

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:58: if (m_state == ReadRestrictor &&
equalIgnoringCase(token->value(), "not")) {
I see, so -> dereferences the iterator's current value?

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:187: , m_expressions(adoptPtr(new
ExpressionHeapVector))
I'm surprised the adoptPtr is needed?

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:204: MediaQuery* mediaQuery = new
MediaQuery(m_restrictor, m_mediaType, m_expressions.release());
Normally adoptPtr is on the same line as new.  check-webkit-style used to
enforce that.

Then this would read:
OwnPtr<MediaQuery> query = adoptPtr(new MediaQuery...);
clear();
return query.release();

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:211: OwnPtr<MediaQueryExp>
expression = MediaQueryExp::create(m_mediaFeature, &m_valueList);
We need to rename the create() method if it can return null.  That's not
expected in the general case of create().  That can be a later patch.  But
create() should always return a value.  You can make a different
createIfPossible or something...

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:228: CSSParserFunction* function =
new CSSParserFunction;
How is memory management dealt with here?

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryTokenizer.cpp (right):

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.cpp:232: MediaQueryToken
MediaQueryTokenizer::consumeNumber()
This is a really long function and it might make sense to split it into helpers,
even if they're local statics.

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryTokenizer.cpp:400: UChar firstChar =
m_input->currentInputChar();
Is m_input ever null?  Can we make it a reference?

[Yoav Weiss, 2014-03-13 22:19:15]

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
File Source/core/css/parser/MediaQueryParser.cpp (right):

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:58: if (m_state == ReadRestrictor &&
equalIgnoringCase(token->value(), "not")) {
On 2014/03/13 17:58:45, eseidel wrote:
> I see, so -> dereferences the iterator's current value?

Yeah

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:187: , m_expressions(adoptPtr(new
ExpressionHeapVector))
On 2014/03/13 17:58:45, eseidel wrote:
> I'm surprised the adoptPtr is needed?

When doing `, m_expressions(new ExpressionHeapVector())`
I'm getting:  error: no matching constructor for initialization of
'OwnPtr<ExpressionHeapVector>'

It might be related to the lack of assignment operator for ExpressionHeapVector
I saw earlier, related to the fact that the vector contains OwnPtrs. I'm not
sure.

https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/...
Source/core/css/parser/MediaQueryParser.cpp:228: CSSParserFunction* function =
new CSSParserFunction;
On 2014/03/13 17:58:45, eseidel wrote:
> How is memory management dealt with here?

function (CSSParserFunction) is added to value (CSSParserValue), which is added
to m_valueList (CSSParserValueList).
Then when MediaQueryParser is destructed, m_valueList is destructed, and its
destructor loops through the values, and deletes the contained functions.

[Yoav Weiss, 2014-03-13 23:13:50]

I've fixed all the nits I could. I've also moved the unitTable to
CSSPrimitiveValue (as suggested earlier).

[eseidel, 2014-03-15 00:09:23]

lgtm

I really appreciate your persistence here.

https://codereview.chromium.org/171383002/diff/610001/Source/core/css/CSSPrim...
File Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/171383002/diff/610001/Source/core/css/CSSPrim...
Source/core/css/CSSPrimitiveValue.cpp:143: CSSPrimitiveValue::UnitTable&
CSSPrimitiveValue::getUnitTable()
No "get", and it looks like you may just want a CSSPrimitiveValue::fromName()
function instead.  But that can also be a later refactoring.

[commit-bot: I haz the power, 2014-03-15 00:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/171383002/610001

[commit-bot: I haz the power, 2014-03-15 00:11:57]

Change committed as 169289

[Noel Gordon, 2014-07-01 07:07:21]

This was reverted https://codereview.chromium.org/196653020 

I noted this patch was reverted, and when it was, our image decoding benchmarks
came back to normal on Nexus 5/7 devices.

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-nexus...

This has regressed again recently, and according to the benchmark, the
regression range is

https://chromium.googlesource.com/chromium/blink/+log/639644e03484d626291dbef...

I notice a lot of Media Query additions here, so maybe @yoav, possible to
investigate and fix?

(sorry for this message btw: we can't automatically bisect these on the perf
bots at the moment).

[Noel Gordon, 2014-07-01 07:07:23]

This was reverted https://codereview.chromium.org/196653020 

I noted this patch was reverted, and when it was, our image decoding benchmarks
came back to normal on Nexus 5/7 devices.

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-nexus...

This has regressed again recently, and according to the benchmark, the
regression range is

https://chromium.googlesource.com/chromium/blink/+log/639644e03484d626291dbef...

I notice a lot of Media Query additions here, so maybe @yoav, possible to
investigate and fix?

(sorry for this message btw: we can't automatically bisect these on the perf
bots at the moment).

[Yoav Weiss, 2014-07-01 07:48:54]

On 2014/07/01 07:07:23, Noel Gordon (Google) wrote:
> This was reverted https://codereview.chromium.org/196653020 
> 
> I noted this patch was reverted, and when it was, our image decoding
benchmarks
> came back to normal on Nexus 5/7 devices.
> 
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-nexus...
> 
> This has regressed again recently, and according to the benchmark, the
> regression range is
> 
>
https://chromium.googlesource.com/chromium/blink/+log/639644e03484d626291dbef...
> 
> I notice a lot of Media Query additions here, so maybe @yoav, possible to
> investigate and fix?
> 
> (sorry for this message btw: we can't automatically bisect these on the perf
> bots at the moment).

Hi Noel,

This patch was reverted not because of perf issues, but because the Oilpan bots
failed building after it was introduced.
I was not aware of any performance regressions related to it, nor resolved by
reverting it (a revert that was very brief, IIRC).

I also don't see the relation between MQs and image decoding benchmarks. Are
these benchmarks performing lots of MQ parsing?

Regarding the CLs in the range, I only see two that touch on MQ, none of them is
directly related to MQ parsing:
https://codereview.chromium.org/322043008 - Is there to improve performance, by
reducing MQ evaluation on style change.
https://codereview.chromium.org/308003007 - Seems only related to dev tools
(triggering MQ evaluation when device emulation is on), so unless the benchmarks
rely on that, this is probably not you guilty CL.

So, assuming it's related to MQs, https://codereview.chromium.org/322043008 is
the only suspect, even though I'd find it surprising. It looks like it'd be
fairly easy to revert that on a branch and run perf tests on it though.

Also - Rune which created that CL, is already CCed here. Rune - can you see why
that CL would cause image decoding benchmark regressions?

[rune, 2014-07-01 08:15:04]

On 2014/07/01 07:48:54, Yoav Weiss wrote:

> So, assuming it's related to MQs, https://codereview.chromium.org/322043008 is
> the only suspect, even though I'd find it surprising. It looks like it'd be
> fairly easy to revert that on a branch and run perf tests on it though.
> 
> Also - Rune which created that CL, is already CCed here. Rune - can you see
why
> that CL would cause image decoding benchmark regressions?

That would surprise me too. Sounds unlikely.

[Noel Gordon, 2014-08-01 08:02:18]

On 2014/07/01 07:48:54, Yoav Weiss wrote:
> Hi Noel,

Yoav, my apologies, completely missed this.  +cc so I might actually see your
reply in future :)

> This patch was reverted not because of perf issues, but because the Oilpan
bots
> failed building after it was introduced.
> I was not aware of any performance regressions related to it, nor resolved by
> reverting it (a revert that was very brief, IIRC).

Right.

> I also don't see the relation between MQs and image decoding benchmarks. Are
> these benchmarks performing lots of MQ parsing?

The decoding test appears to create & remove a large image on the page, over and
over,
about 30 to 60 times / sec.

Not sure what the MQ parsing needs to do, or if rapidly adding / removing (what
I
assume is an HTMLImageElement) would give it some exercise?


> Regarding the CLs in the range, I only see two that touch on MQ, none of them
is
> directly related to MQ parsing:
> ...
> So, assuming it's related to MQs, https://codereview.chromium.org/322043008 is
> the only suspect, even though I'd find it surprising. It looks like it'd be
> fairly easy to revert that on a branch and run perf tests on it though.

I found the perf data for image decoding broken. Turns out no perf test data had
been recorded for some time, the test was no longer working

  https://code.google.com/p/chromium/issues/detail?id=381548

Auto bisecting regressions was broken as a result.  Seems the perf data is
coming
now, but auto bisecting is still having issues 

  https://code.google.com/p/chromium/issues/detail?id=352750

> Also - Rune which created that CL, is already CCed here. Rune - can you see
why
> that CL would cause image decoding benchmark regressions?