Oilpan: move PromiseTracker to the heap (fix build after r181646.)

Oilpan: move PromiseTracker to the heap (fix build after r181646.)

Follow up r181646 and make PromiseTracker work better with Oilpan.

TBR=oilpan-reviews
BUG=340522
NOTRY=true

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

[sof, 2014-09-09 15:37:39]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-09-09 15:37:39]

Please take a look.

Just trying to keep the build alive.. I decided to not just work around the
compile failure, but move this new abstraction to the heap proper.

https://codereview.chromium.org/558673003/diff/1/Source/core/inspector/Promis...
File Source/core/inspector/PromiseTracker.cpp (left):

https://codereview.chromium.org/558673003/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:135:
data->m_promise.setWeak(wrapper.leakPtr(),
&PromiseDataWrapper::didRemovePromise);
I doubt this is correct, but thought I'd put up this CL sooner, rather than
later -- how to assign a heap reference to a ScopedPersistent<> ?

[sof, 2014-09-09 18:21:45]

https://codereview.chromium.org/558673003/diff/1/Source/core/inspector/Promis...
File Source/core/inspector/PromiseTracker.cpp (left):

https://codereview.chromium.org/558673003/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:135:
data->m_promise.setWeak(wrapper.leakPtr(),
&PromiseDataWrapper::didRemovePromise);
On 2014/09/09 15:37:39, sof wrote:
> I doubt this is correct, but thought I'd put up this CL sooner, rather than
> later -- how to assign a heap reference to a ScopedPersistent<> ?

It seems consistent with how GCObservation assigns its ScopedPersistent with
Oilpan enabled, so I will go ahead and land this now.

[sof, 2014-09-09 18:27:36]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-09 18:27:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/558673003/20001

[commit-bot: I haz the power, 2014-09-09 18:28:27]

Committed patchset #2 (id:20001) as 181662

[haraken, 2014-09-10 01:41:22]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-09-10 01:41:23]

LGTM

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
File Source/core/inspector/PromiseTracker.cpp (right):

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
Source/core/inspector/PromiseTracker.cpp:86: return adoptPtrWillBeNoop(new
PromiseDataWrapper(data->createWeakPtr(), map));

I'd write:

#if ENABLE(OILPAN)
return adoptPtrWillBeNoop(new PromiseDataWrapper(data, map));
#else
return adoptPtrWillBeNoop(new PromiseDataWrapper(data->createWeakPtr(), map));
#endif

and completely remove createWeakPtr from oilpan builds.

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
Source/core/inspector/PromiseTracker.cpp:120:
WeakPtrWillBeMember<PromiseTracker::PromiseData> m_data;

Just to confirm: Is this WeakPtr just used to break a reference cycle and it
doesn't need any weak semantics, right?

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
Source/core/inspector/PromiseTracker.cpp:166: RawPtr<PromiseDataVector> vector =
nullptr;

PromiseDataVector* ?

It looks a bit strange that a "RawPtr<>" appears without any WillBe.

[sof, 2014-09-10 13:40:34]

Thanks for the review.

Incoming CL to address the issues raised.

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
File Source/core/inspector/PromiseTracker.cpp (right):

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
Source/core/inspector/PromiseTracker.cpp:86: return adoptPtrWillBeNoop(new
PromiseDataWrapper(data->createWeakPtr(), map));
On 2014/09/10 01:41:23, haraken wrote:
> 
> I'd write:
> 
> #if ENABLE(OILPAN)
> return adoptPtrWillBeNoop(new PromiseDataWrapper(data, map));
> #else
> return adoptPtrWillBeNoop(new PromiseDataWrapper(data->createWeakPtr(), map));
> #endif
> 
> and completely remove createWeakPtr from oilpan builds.

Let's do that.

https://codereview.chromium.org/558673003/diff/20001/Source/core/inspector/Pr...
Source/core/inspector/PromiseTracker.cpp:120:
WeakPtrWillBeMember<PromiseTracker::PromiseData> m_data;
On 2014/09/10 01:41:23, haraken wrote:
> 
> Just to confirm: Is this WeakPtr just used to break a reference cycle and it
> doesn't need any weak semantics, right?

As far as I understand the use, the unfulfilled promise shouldn't keep the
PromiseData alive, but allow it to be pre-emptively cleared.

This isn't provided in an Oilpan setting. Nor is this GCed wrapper object held
onto once created. Addressing both in an incoming follow-up CL.