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

Issue 148523016: Move most of the [Pass]RefPtr's of CSSPrimitiveValue to our transition types. (Closed)

Created:
6 years, 10 months ago by wibling-chromium
Modified:
6 years, 10 months ago
CC:
blink-reviews, shans, fs, yurys+blink_chromium.org, apavlov+blink_chromium.org, adamk+blink_chromium.org, Inactive, loislo+blink_chromium.org, Steve Block, dino_apple.com, rwlbuis, krit, alancutter (OOO until 2018), Timothy Loh, abarth-chromium, dstockwell, dglazkov+blink, pdr., rune+blink, Eric Willigers, rjwright, Mads Ager (chromium), gyuyoung.kim_webkit.org, darktears, Mike Lawther (Google), ed+blinkwatch_opera.com, f(malita), groby+blinkspell_chromium.org, Stephen Chennney, Mikhail, sof
Visibility:
Public.

Description

Move most of the [Pass]RefPtr's of CSSPrimitiveValue to our transition types. I still need to move the CSSPrimitiveValues referred from the classes . Pair . Counter . CSSBasicShape and subclasses . CSSCalcExpressionNode and subclasses . RectBase and subclasses Instead of using Persistents I plan on moving the objects to the oilpan heap. I also added a TraceTrait for RefPtr to compile trace method bodies in non-oilpan mode when tracing collections. Finally I changed RawPtr's release() to zero out the m_ptr to match RefPtr's version. R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, jochen@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org BUG=341815 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167022 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167089

Patch Set 1 #

Total comments: 13

Patch Set 2 : Review feedback #

Patch Set 3 : Sync to latest change #

Total comments: 1

Patch Set 4 : Fix android build #

Patch Set 5 : Sync to latest change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+408 lines, -306 lines) Patch
M Source/core/animation/AnimatableLength.h View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/animation/AnimatableLength.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/BasicShapeFunctions.cpp View 3 chunks +3 lines, -3 lines 0 comments Download
M Source/core/css/CSSBorderImageSliceValue.h View 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/css/CSSBorderImageSliceValue.cpp View 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 10 chunks +29 lines, -29 lines 0 comments Download
M Source/core/css/CSSCrossfadeValue.h View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/css/CSSCrossfadeValue.cpp View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSFontValue.h View 1 chunk +5 lines, -5 lines 0 comments Download
M Source/core/css/CSSFontValue.cpp View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/css/CSSGradientValue.h View 1 7 chunks +34 lines, -25 lines 0 comments Download
M Source/core/css/CSSGradientValue.cpp View 4 chunks +18 lines, -0 lines 0 comments Download
M Source/core/css/CSSImageValue.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/CSSParserValues.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/CSSPrimitiveValue.h View 1 2 2 chunks +37 lines, -14 lines 0 comments Download
M Source/core/css/CSSPrimitiveValue.cpp View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/css/CSSReflectValue.h View 1 2 3 chunks +5 lines, -5 lines 0 comments Download
M Source/core/css/CSSReflectValue.cpp View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSShadowValue.h View 2 chunks +18 lines, -18 lines 0 comments Download
M Source/core/css/CSSShadowValue.cpp View 2 chunks +12 lines, -6 lines 0 comments Download
M Source/core/css/CSSValuePool.h View 2 chunks +18 lines, -18 lines 0 comments Download
M Source/core/css/CSSValuePool.cpp View 5 chunks +30 lines, -19 lines 0 comments Download
M Source/core/css/RGBColor.h View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/css/RGBColor.cpp View 1 chunk +8 lines, -8 lines 0 comments Download
M Source/core/css/Rect.h View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/css/SVGCSSComputedStyleDeclaration.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/css/parser/BisonCSSParser.h View 1 2 8 chunks +14 lines, -14 lines 0 comments Download
M Source/core/css/parser/BisonCSSParser-in.cpp View 1 2 75 chunks +112 lines, -111 lines 0 comments Download
M Source/core/editing/ApplyStyleCommand.cpp View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/editing/EditingStyle.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/editing/EditorCommand.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/html/HTMLHRElement.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/svg/SVGLength.h View 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/svg/SVGLength.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/heap/Handle.h View 1 1 chunk +11 lines, -0 lines 0 comments Download
M Source/heap/Heap.h View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M Source/wtf/RawPtr.h View 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 20 (0 generated)
wibling-chromium
6 years, 10 months ago (2014-02-12 13:17:01 UTC) #1
haraken
LGTM. https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h File Source/core/css/CSSGradientValue.h (right): https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h#newcode53 Source/core/css/CSSGradientValue.h:53: // objects its members are visited via the ...
6 years, 10 months ago (2014-02-12 14:05:44 UTC) #2
haraken
jochen: would you rubberstamp the wtf/ change?
6 years, 10 months ago (2014-02-12 14:06:55 UTC) #3
zerny-chromium
lgtm https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h File Source/core/css/CSSGradientValue.h (right): https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h#newcode54 Source/core/css/CSSGradientValue.h:54: struct CSSGradientColorStop { Add DISALLOW_ALLOCATION
6 years, 10 months ago (2014-02-12 14:10:43 UTC) #4
wibling-chromium
Thanks for the reviews! Replies inline below. https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h File Source/core/css/CSSGradientValue.h (right): https://codereview.chromium.org/148523016/diff/1/Source/core/css/CSSGradientValue.h#newcode53 Source/core/css/CSSGradientValue.h:53: // objects ...
6 years, 10 months ago (2014-02-12 14:42:18 UTC) #5
wibling-chromium
The CQ bit was checked by wibling@chromium.org
6 years, 10 months ago (2014-02-12 14:47:07 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/148523016/130001
6 years, 10 months ago (2014-02-12 14:47:15 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-12 14:47:33 UTC) #8
commit-bot: I haz the power
Failed to apply patch for Source/core/css/parser/BisonCSSParser-in.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 10 months ago (2014-02-12 14:47:34 UTC) #9
jochen (gone - plz use gerrit)
wtf lgtm
6 years, 10 months ago (2014-02-12 15:18:46 UTC) #10
wibling-chromium
The CQ bit was checked by wibling@chromium.org
6 years, 10 months ago (2014-02-12 15:51:41 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/148523016/200002
6 years, 10 months ago (2014-02-12 15:51:59 UTC) #12
commit-bot: I haz the power
Change committed as 167022
6 years, 10 months ago (2014-02-12 17:07:16 UTC) #13
abarth-chromium
A revert of this CL has been created in https://codereview.chromium.org/137863007/ by abarth@chromium.org. The reason for ...
6 years, 10 months ago (2014-02-12 17:35:13 UTC) #14
sof
https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h File Source/heap/Handle.h (right): https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h#newcode392 Source/heap/Handle.h:392: // TraceTrait to allow compilation of trace method bodies ...
6 years, 10 months ago (2014-02-12 20:28:12 UTC) #15
wibling-chromium
On 2014/02/12 20:28:12, sof wrote: > https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h > File Source/heap/Handle.h (right): > > https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h#newcode392 > ...
6 years, 10 months ago (2014-02-13 10:50:02 UTC) #16
wibling-chromium
On 2014/02/12 20:28:12, sof wrote: > https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h > File Source/heap/Handle.h (right): > > https://codereview.chromium.org/148523016/diff/200002/Source/heap/Handle.h#newcode392 > ...
6 years, 10 months ago (2014-02-13 10:50:05 UTC) #17
wibling-chromium
The CQ bit was checked by wibling@chromium.org
6 years, 10 months ago (2014-02-13 12:15:00 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wibling@chromium.org/148523016/570001
6 years, 10 months ago (2014-02-13 12:15:12 UTC) #19
commit-bot: I haz the power
6 years, 10 months ago (2014-02-13 14:03:28 UTC) #20
Message was sent while issue was closed.
Change committed as 167089

Powered by Google App Engine
This is Rietveld 408576698