Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225233005:
Oilpan: Remove raw pointer to DescendantInvalidationSet from StyleInvalidator::RecursionData (Closed)
|
Created:
5 years, 5 months ago by keishi Modified:
5 years, 5 months ago CC:
blink-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Remove raw pointer to DescendantInvalidationSet from StyleInvalidator::RecursionData
BUG=509911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199288
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199294
Patch Set 1 #Patch Set 2 : #
Total comments: 1
Patch Set 3 : added DIALLOW_ALLOCATION #
Total comments: 8
Patch Set 4 : #Messages
Total messages: 18 (4 generated)
keishi@chromium.org changed reviewers: + haraken@chromium.org, oilpan-reviews@chromium.org, yutak@chromium.org
PTAL
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... File Source/core/css/invalidation/StyleInvalidator.h (right): https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:96: RecursionData* m_data; What about this raw pointer?
On 2015/07/17 11:00:43, sof wrote: > https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... > File Source/core/css/invalidation/StyleInvalidator.h (right): > > https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... > Source/core/css/invalidation/StyleInvalidator.h:96: RecursionData* m_data; > What about this raw pointer? I've added DISALLOW_ALLOCATION to RecursionCheckpoint. Given that RecursionCheckpoint::m_data is only assigned in the constructor, I think this should be safe. But should I change RecursionData to GarbageCollected?
https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... File Source/core/css/invalidation/StyleInvalidator.h (right): https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:33: DISALLOW_ALLOCATION(); STACK_ALLOCATED() ? https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:42: DEFINE_INLINE_TRACE() Don't need this, as the backing store will be marked&traced, initiated by conservative scanning of the stack. https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:72: DISALLOW_ALLOCATION(); STACK_ALLOCATED() ? https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:97: RecursionData* m_data; Add a comment saying that this is stack reference, and need not separate tracing.
On 2015/07/22 05:24:27, keishi wrote: > On 2015/07/17 11:00:43, sof wrote: > > > https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... > > File Source/core/css/invalidation/StyleInvalidator.h (right): > > > > > https://codereview.chromium.org/1225233005/diff/20001/Source/core/css/invalid... > > Source/core/css/invalidation/StyleInvalidator.h:96: RecursionData* m_data; > > What about this raw pointer? > > I've added DISALLOW_ALLOCATION to RecursionCheckpoint. Given that > RecursionCheckpoint::m_data is only assigned in the constructor, I think this > should be safe. But should I change RecursionData to GarbageCollected? Yes, should be safe as-is, as it is a pointer to something up (down?) the stack. Adding a comment to document that, ought to be enough imho.
https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... File Source/core/css/invalidation/StyleInvalidator.h (right): https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:33: DISALLOW_ALLOCATION(); On 2015/07/22 06:08:42, sof wrote: > STACK_ALLOCATED() ? Done. https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:42: DEFINE_INLINE_TRACE() On 2015/07/22 06:08:42, sof wrote: > Don't need this, as the backing store will be marked&traced, initiated by > conservative scanning of the stack. Done. https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:72: DISALLOW_ALLOCATION(); On 2015/07/22 06:08:42, sof wrote: > STACK_ALLOCATED() ? Done. https://codereview.chromium.org/1225233005/diff/40001/Source/core/css/invalid... Source/core/css/invalidation/StyleInvalidator.h:97: RecursionData* m_data; On 2015/07/22 06:08:42, sof wrote: > Add a comment saying that this is stack reference, and need not separate > tracing. Done.
lgtm
LGTM
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225233005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199288
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1246353002/ by keishi@chromium.org. The reason for reverting is: wrong bug number.
The CQ bit was checked by keishi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225233005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199294
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1330763002/ by keishi@chromium.org. The reason for reverting is: blink_perf.css regression https://code.google.com/p/chromium/issues/detail?id=513378#makechanges. |
