|
|
Created:
4 years, 11 months ago by hayato Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the oilpan build break after r367789.
See https://codereview.chromium.org/1530643003/#msg35
The CL did not use HeapVector nor HeapHashMap.
BUG=531990
Committed: https://crrev.com/b7d9547b2d4c99fd541e74295c6e40213bf34094
Cr-Commit-Position: refs/heads/master@{#367812}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use raw pointer #Messages
Total messages: 26 (12 generated)
Description was changed from ========== Fix the oilpan build break after https://crrev.com/b7c3414768afff35204a6c6b869c47d90d3953e0 See https://codereview.chromium.org/1530643003/#msg35 The CL does not use HeapVector nor HeapHashMap. ========== to ========== Fix the oilpan build break after https://crrev.com/b7c3414768afff35204a6c6b869c47d90d3953e0 See https://codereview.chromium.org/1530643003/#msg35 The CL does not use HeapVector nor HeapHashMap. BUG=531990 ==========
hayato@chromium.org changed reviewers: + kochi@chromium.org, tkent@chromium.org
PTAL
will defer to tkent-san for oilpan review.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
lgtm Could you change the title to the shorter Fix the oilpan build break after r367789. ? https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:33: RefPtrWillBeMember<HTMLSlotElement> defaultSlot = nullptr; If you really want to keep a RefPtr<> on the non-Oilpan side, then RefPtrWillBeRawPtr<> is the transition type to use. If RefPtr<> usage isn't merited, then leaving this as-was (HTMLSlotElement*) would be appropriate.
On 2016/01/06 08:39:34, kochi wrote: > will defer to tkent-san for oilpan review. Probably the description should say "The CL did not use HeapVector..." instead of "The CL does not use...".
tkent@chromium.org changed reviewers: - sigbjornf@opera.com
lgtm https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:31: using Name2Slot = WillBeHeapHashMap<AtomicString, RefPtrWillBeMember<HTMLSlotElement>>; The original code uses raw pointers, so you may use RawPtrWillBeMember<> instead of RefPtrWillBeMember<>. Of course RefPtrWillBeMember works well.
Description was changed from ========== Fix the oilpan build break after https://crrev.com/b7c3414768afff35204a6c6b869c47d90d3953e0 See https://codereview.chromium.org/1530643003/#msg35 The CL does not use HeapVector nor HeapHashMap. BUG=531990 ========== to ========== Fix the oilpan build break after r367789. See https://codereview.chromium.org/1530643003/#msg35 The CL did not use HeapVector nor HeapHashMap. BUG=531990 ==========
The CQ bit was checked by hayato@chromium.org to run a CQ dry run
Use raw pointer
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562013002/20001
https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp (right): https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:31: using Name2Slot = WillBeHeapHashMap<AtomicString, RefPtrWillBeMember<HTMLSlotElement>>; On 2016/01/06 08:41:21, tkent (slow) wrote: > The original code uses raw pointers, so you may use RawPtrWillBeMember<> instead > of RefPtrWillBeMember<>. > Of course RefPtrWillBeMember works well. Ditto. https://codereview.chromium.org/1562013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/shadow/SlotAssignment.cpp:33: RefPtrWillBeMember<HTMLSlotElement> defaultSlot = nullptr; On 2016/01/06 08:40:30, sof wrote: > If you really want to keep a RefPtr<> on the non-Oilpan side, then > RefPtrWillBeRawPtr<> is the transition type to use. > > If RefPtr<> usage isn't merited, then leaving this as-was (HTMLSlotElement*) > would be appropriate. Done. I reverted this so that it uses a raw pointer. We do not need to guard a slot here.
I also updated the description. Let me land the fix.
The CQ bit was unchecked by hayato@chromium.org
The CQ bit was checked by hayato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, sigbjornf@opera.com Link to the patchset: https://codereview.chromium.org/1562013002/#ps20001 (title: "Use raw pointer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1562013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1562013002/20001
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
(almost there now, but please consider using NOTRY= for fixes that addresses tree breaks.)
Message was sent while issue was closed.
Description was changed from ========== Fix the oilpan build break after r367789. See https://codereview.chromium.org/1530643003/#msg35 The CL did not use HeapVector nor HeapHashMap. BUG=531990 ========== to ========== Fix the oilpan build break after r367789. See https://codereview.chromium.org/1530643003/#msg35 The CL did not use HeapVector nor HeapHashMap. BUG=531990 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix the oilpan build break after r367789. See https://codereview.chromium.org/1530643003/#msg35 The CL did not use HeapVector nor HeapHashMap. BUG=531990 ========== to ========== Fix the oilpan build break after r367789. See https://codereview.chromium.org/1530643003/#msg35 The CL did not use HeapVector nor HeapHashMap. BUG=531990 Committed: https://crrev.com/b7d9547b2d4c99fd541e74295c6e40213bf34094 Cr-Commit-Position: refs/heads/master@{#367812} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b7d9547b2d4c99fd541e74295c6e40213bf34094 Cr-Commit-Position: refs/heads/master@{#367812}
Message was sent while issue was closed.
On 2016/01/06 09:33:00, sof wrote: > (almost there now, but please consider using NOTRY= for fixes that addresses > tree breaks.) Yeah, I leaned it. Thank you. I did not think that Oilpan build break is a tree breaker. |