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

Issue 178803006: Turn MQ classes into thread safe (Closed)

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

Description

Turn MQ classes into thread safe Turns the MQ evaluation classes (MediaQuery, MediaQueryExp and MediaQueryEvaluator) to thread safe by switching them from using AtomicStrings to static strings, initialized by Init. This CL is a split of https://codereview.chromium.org/171383002/ BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168682

Patch Set 1 #

Patch Set 2 : Rebased after MediaFeature generation landed #

Total comments: 7

Patch Set 3 : Addressed review comments #

Patch Set 4 : Rebase #

Patch Set 5 : Another rebase #

Total comments: 2

Patch Set 6 : Moved string comparison optimization to StringImpl #

Total comments: 1

Patch Set 7 : Removed StringImpl comparison atomic check #

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -137 lines) Patch
M Source/core/css/MediaList.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/MediaQuery.h View 1 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/css/MediaQuery.cpp View 1 2 3 chunks +5 lines, -3 lines 0 comments Download
M Source/core/css/MediaQueryEvaluator.h View 1 2 chunks +4 lines, -4 lines 0 comments Download
M Source/core/css/MediaQueryEvaluator.cpp View 1 2 5 chunks +6 lines, -5 lines 0 comments Download
M Source/core/css/MediaQueryExp.h View 1 2 3 chunks +4 lines, -5 lines 0 comments Download
M Source/core/css/MediaQueryExp.cpp View 1 2 3 4 5 4 chunks +106 lines, -101 lines 0 comments Download
M Source/core/html/parser/HTMLParserIdioms.h View 1 chunk +14 lines, -14 lines 0 comments Download
M Source/core/html/parser/HTMLParserIdioms.cpp View 1 2 3 2 chunks +25 lines, -1 line 0 comments Download

Messages

Total messages: 13 (0 generated)
Yoav Weiss
I've split another part of https://codereview.chromium.org/171383002/ It relies on https://codereview.chromium.org/185533012/ which was already reviewed, so ...
6 years, 9 months ago (2014-03-05 07:36:47 UTC) #1
Yoav Weiss
On 2014/03/05 07:36:47, Yoav Weiss wrote: > I've split another part of https://codereview.chromium.org/171383002/ > > ...
6 years, 9 months ago (2014-03-05 13:42:42 UTC) #2
abarth-chromium
This CL generally looks fine. I'm not sure the special handling of hashes is necessary... ...
6 years, 9 months ago (2014-03-06 08:03:57 UTC) #3
Yoav Weiss
On 2014/03/06 08:03:57, abarth wrote: > This CL generally looks fine. I'm not sure the ...
6 years, 9 months ago (2014-03-06 09:50:08 UTC) #4
abarth-chromium
https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp#newcode49 Source/core/css/MediaQueryExp.cpp:49: } I'm still not convinced this optimization is important. ...
6 years, 9 months ago (2014-03-06 16:59:58 UTC) #5
abarth-chromium
https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp File Source/core/css/MediaQueryExp.cpp (right): https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp#newcode49 Source/core/css/MediaQueryExp.cpp:49: } On 2014/03/06 16:59:58, abarth wrote: > I'm still ...
6 years, 9 months ago (2014-03-06 17:05:44 UTC) #6
Yoav Weiss
On 2014/03/06 17:05:44, abarth wrote: > https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp > File Source/core/css/MediaQueryExp.cpp (right): > > https://codereview.chromium.org/178803006/diff/80001/Source/core/css/MediaQueryExp.cpp#newcode49 > ...
6 years, 9 months ago (2014-03-06 17:31:19 UTC) #7
abarth-chromium
https://codereview.chromium.org/178803006/diff/90001/Source/wtf/text/StringImpl.cpp File Source/wtf/text/StringImpl.cpp (right): https://codereview.chromium.org/178803006/diff/90001/Source/wtf/text/StringImpl.cpp#newcode1938 Source/wtf/text/StringImpl.cpp:1938: return false; Can we land this change separately? This ...
6 years, 9 months ago (2014-03-06 18:52:05 UTC) #8
Yoav Weiss
On 2014/03/06 18:52:05, abarth wrote: > https://codereview.chromium.org/178803006/diff/90001/Source/wtf/text/StringImpl.cpp > File Source/wtf/text/StringImpl.cpp (right): > > https://codereview.chromium.org/178803006/diff/90001/Source/wtf/text/StringImpl.cpp#newcode1938 > ...
6 years, 9 months ago (2014-03-06 18:59:04 UTC) #9
abarth-chromium
LGTM Thanks!
6 years, 9 months ago (2014-03-06 22:00:17 UTC) #10
Yoav Weiss
The CQ bit was checked by yoav@yoav.ws
6 years, 9 months ago (2014-03-06 22:05:58 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/178803006/110001
6 years, 9 months ago (2014-03-06 22:07:28 UTC) #12
commit-bot: I haz the power
6 years, 9 months ago (2014-03-06 22:26:12 UTC) #13
Message was sent while issue was closed.
Change committed as 168682

Powered by Google App Engine
This is Rietveld 408576698