Move PageOverlay::Delegate off the Oilpan heap.

Move PageOverlay::Delegate off the Oilpan heap.

Only one implementation holds a GC object, and it can hold it via a
Persistent<> handle.

Committed: https://crrev.com/7530d42c966108cc50dfc206c4c70190587672e3
Cr-Commit-Position: refs/heads/master@{#407889}

[jbroman, 2016-07-14 15:37:55]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 15:38:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-07-14 16:46:20]

jbroman@chromium.org changed reviewers:
+ sigbjornf@opera.com

[commit-bot: I haz the power, 2016-07-14 16:52:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 16:52:46]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-07-19 14:34:35]

ping

[sof, 2016-07-24 16:26:10]

I'd prefer moving PageOverlay::Delegate off the heap instead; did you consider
that? Its implementations don't present a strong case for it to be Blink GC heap
allocated.

(InspectorPageOverlayDelegate could instead keep a
Persistent<InspectorOverlay>.)

[sof, 2016-07-24 16:37:57]

On 2016/07/24 16:26:10, sof wrote:
> I'd prefer moving PageOverlay::Delegate off the heap instead; did you consider
> that? Its implementations don't present a strong case for it to be Blink GC
heap
> allocated.
> 
> (InspectorPageOverlayDelegate could instead keep a
> Persistent<InspectorOverlay>.)

( You could reasonably argue that this is an orthogonal issue to finalization,
but if we're fixing up this delegate interface, then let's change it into
something we're overall happy with. )

[jbroman, 2016-07-25 18:57:12]

jbroman@chromium.org changed reviewers:
+ yutak@chromium.org

[jbroman, 2016-07-25 18:57:13]

yutak moved it into the Oilpan heap in
https://chromium.googlesource.com/chromium/src/+/5cc66df4a8e8f8b2a97e39fecdd0...

It's not clear to me why PageOverlay::Delegate was made
GarbageCollectedFinalized, rather than just making
InspectorOverlayImpl::InspectorPageOverlayDelegate hold a
Persistent<InspectorOverlayImpl>.

yutak, was there a reason to prefer making the interface GC-ed rather than
holding a Persistent<>?

[Yuta Kitamura, 2016-07-26 02:43:04]

On 2016/07/25 18:57:13, jbroman wrote:
> yutak moved it into the Oilpan heap in
>
https://chromium.googlesource.com/chromium/src/+/5cc66df4a8e8f8b2a97e39fecdd0...
> 
> It's not clear to me why PageOverlay::Delegate was made
> GarbageCollectedFinalized, rather than just making
> InspectorOverlayImpl::InspectorPageOverlayDelegate hold a
> Persistent<InspectorOverlayImpl>.
> 
> yutak, was there a reason to prefer making the interface GC-ed rather than
> holding a Persistent<>?

I don't remember... I probably didn't think that that was possible.

[Yuta Kitamura, 2016-07-26 02:43:22]

yutak@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[Yuta Kitamura, 2016-07-26 02:43:22]

+oilpan-reviews

[haraken, 2016-07-26 05:18:59]

Yeah, I think we can move it back to off the heap.

[jbroman, 2016-07-26 17:57:07]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 17:57:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-07-26 18:00:24]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 18:00:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-07-26 18:01:10]

Description was changed from

==========
Make PageOverlay::Delegate not finalized.

No subclass requires finalization.
==========

to

==========
Move PageOverlay::Delegate off the Oilpan heap.

Only one implementation holds a GC object, and it can hold it via a
Persistent<> handle.
==========

[jbroman, 2016-07-26 18:03:39]

Latest patchset moves PageOverlay::Delegate off the Oilpan heap. PTAL.

[sof, 2016-07-26 18:09:16]

non-web/ owner lgtm; thanks.

[jbroman, 2016-07-26 18:14:03]

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

[jbroman, 2016-07-26 18:14:04]

haraken, since you've already commented, can you review for Source/web/?

[haraken, 2016-07-26 18:29:14]

LGTM

[jbroman, 2016-07-26 18:30:11]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-07-26 18:30:13]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-07-26 18:30:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 20:00:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-26 20:01:49]

Description was changed from

==========
Move PageOverlay::Delegate off the Oilpan heap.

Only one implementation holds a GC object, and it can hold it via a
Persistent<> handle.
==========

to

==========
Move PageOverlay::Delegate off the Oilpan heap.

Only one implementation holds a GC object, and it can hold it via a
Persistent<> handle.

Committed: https://crrev.com/7530d42c966108cc50dfc206c4c70190587672e3
Cr-Commit-Position: refs/heads/master@{#407889}
==========

[commit-bot: I haz the power, 2016-07-26 20:01:50]

Patchset 3 (id:??) landed as
https://crrev.com/7530d42c966108cc50dfc206c4c70190587672e3
Cr-Commit-Position: refs/heads/master@{#407889}