|
|
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... |