Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
873573005:
Oilpan: merge MIDIAccessInitializer's prefinalizers. (Closed)
|
Created:
5 years, 10 months ago by sof Modified:
5 years, 10 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: merge MIDIAccessInitializer's prefinalizers.
As the prefinalizer handling does not support having multiple prefinalizers
per object, combine the MIDIAccessInitializer + LifecycleObserver
prefinalizers methods into one, and arrange for it to be the prefinalizer
for MIDIAccessInitializer.
In the event this manual prefinalizer re-combination turns out to be a
common pattern and workaround, we will revisit the restriction of allowing
only one prefinalizer registration per object.
R=haraken,kouhei
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189103
Patch Set 1 #
Total comments: 6
Patch Set 2 : add cast to expected type + fixme #Messages
Total messages: 15 (4 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM, thanks for the fix. https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:27: // support multiple prefinalizers for an object. Shall we add a FIXME?
https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:27: // support multiple prefinalizers for an object. On 2015/01/28 09:53:10, haraken wrote: > > Shall we add a FIXME? Can do that, what did you have in mind? i.e., multiple prefinalizer support or avoid there being multiple prefinalizers for MIDIAccessInitializer in the first place?
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:28: ThreadState::current()->unregisterPreFinalizer(*this); How about ThreadState::current()->unregisterPreFinalizer(static_cast<LifecycleObserver&>(*this)); (void*)(LifecycleObserver*)this may be != (void*)this
https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:27: // support multiple prefinalizers for an object. On 2015/01/28 09:56:42, sof wrote: > On 2015/01/28 09:53:10, haraken wrote: > > > > Shall we add a FIXME? > > Can do that, what did you have in mind? i.e., multiple prefinalizer support or > avoid there being multiple prefinalizers for MIDIAccessInitializer in the first > place? Avoid multiple prefinalizers and add an ASSERT about it :)
On 2015/01/28 09:58:04, kouhei wrote: > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): > > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > Source/modules/webmidi/MIDIAccessInitializer.cpp:28: > ThreadState::current()->unregisterPreFinalizer(*this); > How about > ThreadState::current()->unregisterPreFinalizer(static_cast<LifecycleObserver&>(*this)); > > (void*)(LifecycleObserver*)this may be != (void*)this Makes good sense; curiously they are the same here, even though LifecycleObserver isn't a leftmost base.
On 2015/01/28 10:04:07, sof wrote: > On 2015/01/28 09:58:04, kouhei wrote: > > > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > > File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): > > > > > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > > Source/modules/webmidi/MIDIAccessInitializer.cpp:28: > > ThreadState::current()->unregisterPreFinalizer(*this); > > How about > > > ThreadState::current()->unregisterPreFinalizer(static_cast<LifecycleObserver&>(*this)); > > > > (void*)(LifecycleObserver*)this may be != (void*)this > > Makes good sense; curiously they are the same here, even though > LifecycleObserver isn't a leftmost base. Can we add an ASSERT to check that multiple prefinalizers are registered on one object?
On 2015/01/28 10:05:15, haraken wrote: > On 2015/01/28 10:04:07, sof wrote: > > On 2015/01/28 09:58:04, kouhei wrote: > > > > > > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > > > File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): > > > > > > > > > https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... > > > Source/modules/webmidi/MIDIAccessInitializer.cpp:28: > > > ThreadState::current()->unregisterPreFinalizer(*this); > > > How about > > > > > > ThreadState::current()->unregisterPreFinalizer(static_cast<LifecycleObserver&>(*this)); > > > > > > (void*)(LifecycleObserver*)this may be != (void*)this > > > > Makes good sense; curiously they are the same here, even though > > LifecycleObserver isn't a leftmost base. > > Can we add an ASSERT to check that multiple prefinalizers are registered on one > object? If by one/same object we mean object pointer/reference equality, it's already in place. Similarly on the unregistration side; it will assert on unregistering unbound/already-unregistered object references.
Thanks for the reviews. https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:27: // support multiple prefinalizers for an object. On 2015/01/28 10:01:36, haraken wrote: > On 2015/01/28 09:56:42, sof wrote: > > On 2015/01/28 09:53:10, haraken wrote: > > > > > > Shall we add a FIXME? > > > > Can do that, what did you have in mind? i.e., multiple prefinalizer support or > > avoid there being multiple prefinalizers for MIDIAccessInitializer in the > first > > place? > > Avoid multiple prefinalizers and add an ASSERT about it :) Done. https://codereview.chromium.org/873573005/diff/1/Source/modules/webmidi/MIDIA... Source/modules/webmidi/MIDIAccessInitializer.cpp:28: ThreadState::current()->unregisterPreFinalizer(*this); On 2015/01/28 09:58:04, kouhei wrote: > How about > ThreadState::current()->unregisterPreFinalizer(static_cast<LifecycleObserver&>(*this)); > > (void*)(LifecycleObserver*)this may be != (void*)this Done.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/873573005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189103 |
