Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(244)

Issue 171383002: A thread-safe Media Query Parser (Closed)

Created:
6 years, 10 months ago by Yoav Weiss
Modified:
6 years, 4 months ago
CC:
blink-reviews, kenneth.christiansen, ed+blinkwatch_opera.com, dglazkov+blink, apavlov+blink_chromium.org, adamk+blink_chromium.org, darktears, rune+blink, Noel Gordon
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Refactored and passes tests #

Total comments: 1

Patch Set 3 : Rebased and cleaned up some more #

Patch Set 4 : Rebase, split CSSID from Bison, cleaner MQExp #

Patch Set 5 : Rebase to fix build errors on some platforms #

Patch Set 6 : Fix build errors on Android #

Patch Set 7 : Another attempt to fix Android build issues #

Total comments: 5

Patch Set 8 : Generate strings with make_names #

Total comments: 2

Patch Set 9 : Rebased and fixed float parsing #

Total comments: 1

Patch Set 10 : Crash fix. New license. #

Patch Set 11 : Renamed CSS to MediaQuery. Removed unused tokens. #

Patch Set 12 : Added a fixme saying that a generic solution can replace this one #

Total comments: 2

Patch Set 13 : Removed comment #

Total comments: 5

Patch Set 14 : Fixed review comments #

Patch Set 15 : Another comment fixed #

Total comments: 36

Patch Set 16 : #

Patch Set 17 : Fix a debug build error #

Total comments: 9

Patch Set 18 : Fixed gcc compile issues and debug asserts #

Total comments: 11

Patch Set 19 : Fixed review nits #

Patch Set 20 : Rebase #

Patch Set 21 : Fix Android build #

Patch Set 22 : Moar rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1183 lines, -5 lines) Patch
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +5 lines, -0 lines 0 comments Download
M Source/core/css/CSSPrimitiveValue.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/css/CSSPrimitiveValue.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +34 lines, -0 lines 1 comment Download
M Source/core/css/MediaList.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/css/MediaQuery.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/MediaQuery.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/css/MediaQueryExp.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/MediaQuerySetTest.cpp View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/css/parser/CSSParserIdioms.h View 1 1 chunk +1 line, -1 line 0 comments Download
A Source/core/css/parser/MediaQueryInputStream.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +57 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryInputStream.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +66 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryParser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +98 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +243 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryToken.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +69 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryToken.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +62 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryTokenizer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +74 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryTokenizer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +431 lines, -0 lines 0 comments Download
A Source/core/css/parser/MediaQueryTokenizerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +27 lines, -0 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
eseidel
https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryExp.cpp#newcode163 Source/core/css/MediaQueryExp.cpp:163: return mediaFeatureHash == MediaFeatureNames::monochromeMediaFeature.impl()->existingHash() Why the fake hash-table? :) ...
6 years, 10 months ago (2014-02-18 23:24:01 UTC) #1
Yoav Weiss
On 2014/02/18 23:24:01, eseidel wrote: > https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryExp.cpp > File Source/core/css/MediaQueryExp.cpp (right): > > https://codereview.chromium.org/171383002/diff/1/Source/core/css/MediaQueryExp.cpp#newcode163 > ...
6 years, 10 months ago (2014-02-19 08:26:19 UTC) #2
Yoav Weiss
I've uploaded a new patch that is refactored, passes tests, and should be more efficient. ...
6 years, 10 months ago (2014-02-25 22:50:34 UTC) #3
Yoav Weiss
On 2014/02/25 22:50:34, Yoav Weiss wrote: > I've uploaded a new patch that is refactored, ...
6 years, 10 months ago (2014-02-26 10:00:35 UTC) #4
eseidel
https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQueryExp.cpp#newcode48 Source/core/css/MediaQueryExp.cpp:48: return mediaFeatureHash == MediaFeatureNames::orientationMediaFeature.impl()->existingHash() Consider writing an hashEquals() helper ...
6 years, 10 months ago (2014-02-26 10:18:41 UTC) #5
Yoav Weiss
On 2014/02/26 10:18:41, eseidel wrote: > https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQueryExp.cpp > File Source/core/css/MediaQueryExp.cpp (right): > > https://codereview.chromium.org/171383002/diff/230001/Source/core/css/MediaQueryExp.cpp#newcode48 > ...
6 years, 10 months ago (2014-02-26 23:16:27 UTC) #6
abarth-chromium
https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.cpp File Source/core/css/CSSValueIDHelper.cpp (right): https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.cpp#newcode2 Source/core/css/CSSValueIDHelper.cpp:2: // ditto https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.h File Source/core/css/CSSValueIDHelper.h (right): https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.h#newcode2 Source/core/css/CSSValueIDHelper.h:2: // ...
6 years, 9 months ago (2014-03-01 07:28:24 UTC) #7
Yoav Weiss
On 2014/03/01 07:28:24, abarth wrote: > https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.cpp > File Source/core/css/CSSValueIDHelper.cpp (right): > > https://codereview.chromium.org/171383002/diff/330001/Source/core/css/CSSValueIDHelper.cpp#newcode2 > ...
6 years, 9 months ago (2014-03-03 16:15:42 UTC) #8
abarth-chromium
Can we break this change into smaller pieces? For example, we can land the "in" ...
6 years, 9 months ago (2014-03-03 19:06:18 UTC) #9
Yoav Weiss
On 2014/03/03 19:06:18, abarth wrote: > Can we break this change into smaller pieces? For ...
6 years, 9 months ago (2014-03-03 23:07:16 UTC) #10
Yoav Weiss
I've split 2 CLs from this one. Both have landed: * Static string generation - ...
6 years, 9 months ago (2014-03-07 05:23:44 UTC) #11
abarth-chromium
Is this based on Eric's CL? It would probably make sense for Eric to take ...
6 years, 9 months ago (2014-03-07 05:58:03 UTC) #12
Yoav Weiss
On 2014/03/07 05:58:03, abarth wrote: > Is this based on Eric's CL? It would probably ...
6 years, 9 months ago (2014-03-07 06:04:41 UTC) #13
Yoav Weiss
As discussed with Eric on IRC, I've renamed the CSS* classes to be MediaQuery* classes, ...
6 years, 9 months ago (2014-03-08 10:18:23 UTC) #14
eseidel
I need to read this a couple more times, it's complicated. :) I'll make sure ...
6 years, 9 months ago (2014-03-08 22:08:07 UTC) #15
Yoav Weiss
On 2014/03/08 22:08:07, eseidel wrote: > I need to read this a couple more times, ...
6 years, 9 months ago (2014-03-08 22:29:38 UTC) #16
kenneth.r.christiansen
A few mini comments while skimming through the patch https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/MediaQueryParser.h File Source/core/css/parser/MediaQueryParser.h (right): https://codereview.chromium.org/171383002/diff/440001/Source/core/css/parser/MediaQueryParser.h#newcode85 Source/core/css/parser/MediaQueryParser.h:85: ...
6 years, 9 months ago (2014-03-08 22:37:46 UTC) #17
Yoav Weiss
On 2014/03/08 22:37:46, kenneth.r.christiansen wrote: > A few mini comments while skimming through the patch ...
6 years, 9 months ago (2014-03-10 07:47:33 UTC) #18
eseidel
Lots of little comments. Sorry to be so slow. Will be faster next time. What ...
6 years, 9 months ago (2014-03-11 20:14:18 UTC) #19
Yoav Weiss
https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/MediaQueryParser.cpp File Source/core/css/parser/MediaQueryParser.cpp (right): https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/MediaQueryParser.cpp#newcode37 Source/core/css/parser/MediaQueryParser.cpp:37: m_state = &MediaQueryParser::readFeature; On 2014/03/11 20:14:19, eseidel wrote: > ...
6 years, 9 months ago (2014-03-12 20:39:00 UTC) #20
eseidel
Will review the new patch shortly. One goal here is to figure out what this ...
6 years, 9 months ago (2014-03-13 00:49:35 UTC) #21
Yoav Weiss
> > https://codereview.chromium.org/171383002/diff/480001/Source/core/css/parser/MediaQueryParser.cpp#newcode175 > Source/core/css/parser/MediaQueryParser.cpp:175: void > MediaQueryData::resetCurrentQuery() > On 2014/03/11 20:14:19, eseidel wrote: > ...
6 years, 9 months ago (2014-03-13 10:50:40 UTC) #22
eseidel
I had these drafts left over from last night. Looking at your newer patch now. ...
6 years, 9 months ago (2014-03-13 17:22:06 UTC) #23
Yoav Weiss
Thanks! I'll fix the nits and upload them. On 2014/03/13 17:22:06, eseidel wrote: > > ...
6 years, 9 months ago (2014-03-13 17:49:35 UTC) #24
eseidel
Honestly, this lgtm. I think we have some outstanding nits from earlier passes, but I ...
6 years, 9 months ago (2014-03-13 17:58:43 UTC) #25
Yoav Weiss
https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/MediaQueryParser.cpp File Source/core/css/parser/MediaQueryParser.cpp (right): https://codereview.chromium.org/171383002/diff/530001/Source/core/css/parser/MediaQueryParser.cpp#newcode58 Source/core/css/parser/MediaQueryParser.cpp:58: if (m_state == ReadRestrictor && equalIgnoringCase(token->value(), "not")) { On ...
6 years, 9 months ago (2014-03-13 22:19:15 UTC) #26
Yoav Weiss
I've fixed all the nits I could. I've also moved the unitTable to CSSPrimitiveValue (as ...
6 years, 9 months ago (2014-03-13 23:13:50 UTC) #27
eseidel
lgtm I really appreciate your persistence here. https://codereview.chromium.org/171383002/diff/610001/Source/core/css/CSSPrimitiveValue.cpp File Source/core/css/CSSPrimitiveValue.cpp (right): https://codereview.chromium.org/171383002/diff/610001/Source/core/css/CSSPrimitiveValue.cpp#newcode143 Source/core/css/CSSPrimitiveValue.cpp:143: CSSPrimitiveValue::UnitTable& CSSPrimitiveValue::getUnitTable() ...
6 years, 9 months ago (2014-03-15 00:09:23 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/171383002/610001
6 years, 9 months ago (2014-03-15 00:09:36 UTC) #29
commit-bot: I haz the power
Change committed as 169289
6 years, 9 months ago (2014-03-15 00:11:57 UTC) #30
Noel Gordon
This was reverted https://codereview.chromium.org/196653020 I noted this patch was reverted, and when it was, our ...
6 years, 5 months ago (2014-07-01 07:07:21 UTC) #31
Noel Gordon
This was reverted https://codereview.chromium.org/196653020 I noted this patch was reverted, and when it was, our ...
6 years, 5 months ago (2014-07-01 07:07:23 UTC) #32
Yoav Weiss
On 2014/07/01 07:07:23, Noel Gordon (Google) wrote: > This was reverted https://codereview.chromium.org/196653020 > > I ...
6 years, 5 months ago (2014-07-01 07:48:54 UTC) #33
rune
On 2014/07/01 07:48:54, Yoav Weiss wrote: > So, assuming it's related to MQs, https://codereview.chromium.org/322043008 is ...
6 years, 5 months ago (2014-07-01 08:15:04 UTC) #34
Noel Gordon
6 years, 4 months ago (2014-08-01 08:02:18 UTC) #35
Message was sent while issue was closed.
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?

Powered by Google App Engine
This is Rietveld 408576698