Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 2893243002: Add missing data members in StyleRareNonInheritedData copy constructor. (Closed)

Created:
1 year, 7 months ago by shend
Modified:
1 year, 6 months ago
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews, Jeffrey Yasskin
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add missing data members in StyleRareNonInheritedData copy constructor. Two data members are missing from the StyleRareNonInheritedData copy constructor. This can be problematic because StyleRareNonInheritedData is stored with copy-on-write semantics: when two ComputedStyles share the same StyleRareNonInheritedData, modifying one of them will create a copy without copying those two data members. This patch adds callback_selectors and paint_images into the copy constructor. BUG=628043 Review-Url: https://codereview.chromium.org/2893243002 Cr-Commit-Position: refs/heads/master@{#476577} Committed: https://chromium.googlesource.com/chromium/src/+/399dd8b1e55cfa495e75fcf03b3e65ef3188c82b

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 2

Patch Set 3 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -0 lines) Patch
M third_party/WebKit/Source/core/style/StyleRareNonInheritedData.cpp View 1 2 1 chunk +4 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 41 (24 generated)
shend
Hi Alan, PTAL. I've now found three cases where we didn't copy a data member ...
1 year, 7 months ago (2017-05-22 23:25:55 UTC) #9
alancutter (OOO until 2018)
On 2017/05/22 at 23:25:55, shend wrote: > Hi Alan, PTAL. I've now found three cases ...
1 year, 6 months ago (2017-05-24 04:02:07 UTC) #13
shend
On 2017/05/24 at 04:02:07, alancutter wrote: > On 2017/05/22 at 23:25:55, shend wrote: > > ...
1 year, 6 months ago (2017-05-28 23:24:10 UTC) #14
shend
Redirecting to meade@, WDYT regarding Alan's comments?
1 year, 6 months ago (2017-05-30 22:19:39 UTC) #16
meade_UTC10
On 2017/05/30 22:19:39, shend wrote: > Redirecting to meade@, WDYT regarding Alan's comments? I think ...
1 year, 6 months ago (2017-05-31 05:59:19 UTC) #17
meade_UTC10
oops forgot to send draft with previous message https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right): https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h#newcode77 third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:77: using ...
1 year, 6 months ago (2017-05-31 05:59:45 UTC) #18
shend
> Here's the CL that added callback_selectors_, although I can't see any reasoning for why ...
1 year, 6 months ago (2017-05-31 06:35:25 UTC) #19
shend
On 2017/05/31 at 06:35:25, shend wrote: > > Here's the CL that added callback_selectors_, although ...
1 year, 6 months ago (2017-05-31 06:42:00 UTC) #20
shend
https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right): https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h#newcode77 third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:77: using PaintImages = Vector<Persistent<StyleImage>>; On 2017/05/31 at 05:59:45, meade_UTC10 ...
1 year, 6 months ago (2017-05-31 06:58:25 UTC) #23
shend
Hi Ian, in a patch [1] you wrote to add StyleRareNonInheritedData::paint_images_, I noticed that paint_images_ ...
1 year, 6 months ago (2017-06-01 07:19:46 UTC) #27
ikilpatrick
lgtm
1 year, 6 months ago (2017-06-01 14:10:08 UTC) #28
ikilpatrick
lgtm, this was probably just an oversight from myself :) thanks.
1 year, 6 months ago (2017-06-01 14:11:02 UTC) #29
meade_UTC10
I agree it's probably an oversight. jyasskin please comment if we're wrong and callback_selectors is ...
1 year, 6 months ago (2017-06-02 03:51:26 UTC) #32
meade_UTC10
lgtm
1 year, 6 months ago (2017-06-02 03:51:36 UTC) #33
shend
On 2017/06/02 at 03:51:36, meade wrote: > lgtm Committing now, will revert if omission turns ...
1 year, 6 months ago (2017-06-02 05:11:40 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2893243002/40001
1 year, 6 months ago (2017-06-02 05:11:54 UTC) #38
commit-bot: I haz the power
1 year, 6 months ago (2017-06-02 05:16:01 UTC) #41
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/399dd8b1e55cfa495e75fcf03b3e...

Powered by Google App Engine
This is Rietveld 408576698