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

Issue 1176393005: Remove usage of CSSParserValue in MediaQueryParser (Closed)

Created:
5 years, 6 months ago by rwlbuis
Modified:
5 years, 6 months ago
Reviewers:
Timothy Loh, Yoav Weiss
CC:
blink-reviews, kenneth.christiansen, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Remove usage of CSSParserValue in MediaQueryParser Remove usage of CSSParserValue in MediaQueryParser by using CSSParserToken everywhere. Also prefer to deal with const ref rather than pointers. BUG=499780 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197219

Patch Set 1 #

Patch Set 2 : More fixes #

Patch Set 3 : Fix unit test problems #

Patch Set 4 : Simplify MediaQueryData::tryAddParserToken #

Total comments: 2

Patch Set 5 : Add comment #

Total comments: 4

Patch Set 6 : Address review comments #

Total comments: 1

Patch Set 7 : Patch for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -72 lines) Patch
M Source/core/css/MediaQueryExp.h View 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/css/MediaQueryExp.cpp View 1 2 3 4 5 10 chunks +48 lines, -43 lines 0 comments Download
M Source/core/css/parser/MediaQueryParser.h View 2 chunks +2 lines, -3 lines 0 comments Download
M Source/core/css/parser/MediaQueryParser.cpp View 2 3 4 chunks +12 lines, -24 lines 0 comments Download

Messages

Total messages: 26 (10 generated)
rwlbuis
PTAL. - the static_assert is optional, but I think handy since we don't want to ...
5 years, 6 months ago (2015-06-15 17:53:51 UTC) #2
Yoav Weiss
https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp#newcode228 Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() == NumberToken || token.type() == ...
5 years, 6 months ago (2015-06-15 21:46:27 UTC) #4
rwlbuis
https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp#newcode228 Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() == NumberToken || token.type() == ...
5 years, 6 months ago (2015-06-15 22:00:53 UTC) #5
Yoav Weiss
On 2015/06/15 22:00:53, rwlbuis wrote: > https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp > File Source/core/css/MediaQueryExp.cpp (right): > > https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp#newcode228 > ...
5 years, 6 months ago (2015-06-15 22:15:18 UTC) #6
rwlbuis
On 2015/06/15 22:15:18, Yoav Weiss wrote: > https://codereview.chromium.org/1176393005/diff/60001/Source/core/css/MediaQueryExp.cpp#newcode228 > > Source/core/css/MediaQueryExp.cpp:228: } else if (token.type() ...
5 years, 6 months ago (2015-06-15 22:40:37 UTC) #7
Timothy Loh
https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQueryExp.cpp#newcode230 Source/core/css/MediaQueryExp.cpp:230: if (token.type() == NumberToken || token.type() == PercentageToken || ...
5 years, 6 months ago (2015-06-16 00:05:36 UTC) #8
rwlbuis
PTAL https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/1176393005/diff/80001/Source/core/css/MediaQueryExp.cpp#newcode230 Source/core/css/MediaQueryExp.cpp:230: if (token.type() == NumberToken || token.type() == PercentageToken ...
5 years, 6 months ago (2015-06-16 02:34:15 UTC) #9
Timothy Loh
lgtm https://codereview.chromium.org/1176393005/diff/100001/Source/core/css/parser/CSSParserToken.cpp File Source/core/css/parser/CSSParserToken.cpp (right): https://codereview.chromium.org/1176393005/diff/100001/Source/core/css/parser/CSSParserToken.cpp#newcode23 Source/core/css/parser/CSSParserToken.cpp:23: static_assert(sizeof(CSSParserToken) == sizeof(SameSizeAsCSSParserToken), "CSSParserToken should stay small"); Actually, ...
5 years, 6 months ago (2015-06-16 06:20:36 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176393005/120001
5 years, 6 months ago (2015-06-16 14:09:44 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66809)
5 years, 6 months ago (2015-06-16 16:39:33 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176393005/120001
5 years, 6 months ago (2015-06-16 16:54:44 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66833)
5 years, 6 months ago (2015-06-16 19:16:02 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176393005/120001
5 years, 6 months ago (2015-06-16 19:21:14 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66864)
5 years, 6 months ago (2015-06-16 22:10:19 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1176393005/120001
5 years, 6 months ago (2015-06-16 22:18:08 UTC) #25
commit-bot: I haz the power
5 years, 6 months ago (2015-06-17 00:58:31 UTC) #26
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197219

Powered by Google App Engine
This is Rietveld 408576698