Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893243002:
Add missing data members in StyleRareNonInheritedData copy constructor. (Closed)
DescriptionAdd 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 #
Dependent Patchsets: Messages
Total messages: 41 (24 generated)
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== copy-all BUG= ========== to ========== 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 them in. BUG=628043 ==========
shend@chromium.org changed reviewers: + alancutter@chromium.org
Hi Alan, PTAL. I've now found three cases where we didn't copy a data member over and none of them caused issues in the wild. Do you know why this is the case?
Description was changed from ========== 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 them in. BUG=628043 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/22 at 23:25:55, shend wrote: > Hi Alan, PTAL. I've now found three cases where we didn't copy a data member over and none of them caused issues in the wild. Do you know why this is the case? Both of these fields I believe are atypical and require investigation as to whether it makes sense to do this.
On 2017/05/24 at 04:02:07, alancutter wrote: > On 2017/05/22 at 23:25:55, shend wrote: > > Hi Alan, PTAL. I've now found three cases where we didn't copy a data member over and none of them caused issues in the wild. Do you know why this is the case? > > Both of these fields I believe are atypical and require investigation as to whether it makes sense to do this. Hmm yeah these two members are pretty mysterious. How would you feel if I land this to see if anything breaks? If there's no bugs or perf regressions caused by this, then it shouldn't be a problem.
shend@chromium.org changed reviewers: + meade@chromium.org - alancutter@chromium.org
Redirecting to meade@, WDYT regarding Alan's comments?
On 2017/05/30 22:19:39, shend wrote: > Redirecting to meade@, WDYT regarding Alan's comments? I think it's worth trying to understand what callback_selectors_ and paint_images_ are used before before just trying things... for example, the callback selectors are updated here, but why? Is it a workaround for it not being copied the way you've done it in your CL?: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... Here's the CL that added callback_selectors_, although I can't see any reasoning for why it is left off the copy constructor
oops forgot to send draft with previous message https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right): https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:77: using PaintImages = Vector<Persistent<StyleImage>>; Do you expect PaintImages to be used by anything outside of StyleRareNonInheritedData? If not, you probably shouldn't create an alias. https://google.github.io/styleguide/cppguide.html#Aliases
> Here's the CL that added callback_selectors_, although I can't see any reasoning for why it is left off the copy constructor Do you have a link to the CL?
On 2017/05/31 at 06:35:25, shend wrote: > > Here's the CL that added callback_selectors_, although I can't see any reasoning for why it is left off the copy constructor > > Do you have a link to the CL? Nevermind found both CLs: CallbackSelectors: https://chromium.googlesource.com/chromium/src/+/62d39efdf1705778694f0611f7f4... PaintImages: https://chromium.googlesource.com/chromium/src/+/0ab7ac1ec67da157bc4ef5cbcb1b... For PaintImages, could I ask ikilpatrick@? If it was not a mistake, then it might also give insight into CallbackSelectors.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right): https://codereview.chromium.org/2893243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:77: using PaintImages = Vector<Persistent<StyleImage>>; On 2017/05/31 at 05:59:45, meade_UTC10 wrote: > Do you expect PaintImages to be used by anything outside of StyleRareNonInheritedData? If not, you probably shouldn't create an alias. > > https://google.github.io/styleguide/cppguide.html#Aliases Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shend@chromium.org changed reviewers: + ikilpatrick@chromium.org
Hi Ian, in a patch [1] you wrote to add StyleRareNonInheritedData::paint_images_, I noticed that paint_images_ was not included in the copy constructor. I was wondering if this was intentional and if so, what's the reason? [1] https://chromium.googlesource.com/chromium/src/+/0ab7ac1ec67da157bc4ef5cbcb1b...
lgtm
lgtm, this was probably just an oversight from myself :) thanks.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I agree it's probably an oversight. jyasskin please comment if we're wrong and callback_selectors is deliberately appearing in StyleRareNonInheritedData's copy constructor (and if so please tell us why).
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/02 at 03:51:36, meade wrote: > lgtm Committing now, will revert if omission turns out to be deliberate.
The CQ bit was checked by shend@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496380304006210,
"parent_rev": "bc8303e30e9d84c918e984e9231edb314ff0fb07", "commit_rev":
"399dd8b1e55cfa495e75fcf03b3e65ef3188c82b"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/399dd8b1e55cfa495e75fcf03b3e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/399dd8b1e55cfa495e75fcf03b3e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
