Oilpan: Prepare to move IdTargetObserver and IdTargetObserverRegistry to Oilpan heap.

Oilpan: Prepare to move IdTargetObserver and IdTargetObserverRegistry to Oilpan heap.

BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173747

[tkent, 2014-05-09 06:38:21]

Please review this.

[keishi, 2014-05-09 06:57:56]

lgtm

[haraken, 2014-05-09 07:01:34]

LGTM

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
File Source/core/dom/IdTargetObserverRegistry.h (right):

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserverRegistry.h:56: typedef
WillBeHeapHashSet<RawPtrWillBeMember<IdTargetObserver> > ObserverSet;

Just to confirm: This is a Member, not a WeakMember, right? As far as I read the
code, the IdTargetObserverRegistry is owned by a TreeScope and all observers are
guaranteed to be in the TreeScope. So the IdTargetObserverRegistry and the
observers should die together, so it looks OK to use a Member here.

[zerny-chromium, 2014-05-09 08:01:49]

lgtm

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
File Source/core/dom/IdTargetObserverRegistry.h (right):

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserverRegistry.h:40: class IdTargetObserverRegistry :
public NoBaseWillBeGarbageCollectedFinalized<IdTargetObserverRegistry> {
FINAL

[wibling-chromium, 2014-05-09 08:23:47]

lgtm

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
File Source/core/dom/IdTargetObserver.h (right):

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserver.h:47:
RawPtrWillBeMember<IdTargetObserverRegistry> m_registry;
NIT: Have you considered adding a IdTargetObserverRegistry& registry() accessor
to stay as close as possible to the current code. Mads described this pattern in
a recent email sent to blink-dev titled, C++ reference and Oilpan transition
types.

[tkent, 2014-05-09 09:37:20]

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
File Source/core/dom/IdTargetObserver.h (right):

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserver.h:47:
RawPtrWillBeMember<IdTargetObserverRegistry> m_registry;
On 2014/05/09 08:23:47, wibling-chromium wrote:
> NIT: Have you considered adding a IdTargetObserverRegistry& registry()
accessor
> to stay as close as possible to the current code. Mads described this pattern
in
> a recent email sent to blink-dev titled, C++ reference and Oilpan transition
> types.

Added |IdTargetObserverRegistry& registry()| in the new patch.

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
File Source/core/dom/IdTargetObserverRegistry.h (right):

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserverRegistry.h:40: class IdTargetObserverRegistry :
public NoBaseWillBeGarbageCollectedFinalized<IdTargetObserverRegistry> {
On 2014/05/09 08:01:49, zerny-chromium wrote:
> FINAL

Done.

https://codereview.chromium.org/278803002/diff/1/Source/core/dom/IdTargetObse...
Source/core/dom/IdTargetObserverRegistry.h:56: typedef
WillBeHeapHashSet<RawPtrWillBeMember<IdTargetObserver> > ObserverSet;
On 2014/05/09 07:01:35, haraken wrote:
> 
> Just to confirm: This is a Member, not a WeakMember, right? As far as I read
the
> code, the IdTargetObserverRegistry is owned by a TreeScope and all observers
are
> guaranteed to be in the TreeScope. So the IdTargetObserverRegistry and the
> observers should die together, so it looks OK to use a Member here.

Right.  This cycle (IdTargetObserverRegistry -> ObserverSet -> IdTargetObserver
-> IdTargetObserver) should have no problem because everything is in a single
document, and an IdTargetObserver should be unregistered when it's removed from
a document tree.

[tkent, 2014-05-09 09:37:24]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-09 09:39:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/278803002/20001

[commit-bot: I haz the power, 2014-05-09 11:49:10]

Change committed as 173747