EventSender<T> singletons are better off on the Oilpan heap.

EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515
Committed: https://crrev.com/2d9b40462e8b813e7d9bc3ebc4be6b4c40ecd0d0
Cr-Commit-Position: refs/heads/master@{#367282}

[sof, 2015-12-31 22:04:42]

Description was changed from

==========
EventSender<T> is better of being on the Oilpan heap.

R=
BUG=
==========

to

==========
EventSender<T> is better off being on the Oilpan heap.

R=
BUG=
==========

[sof, 2016-01-02 06:19:33]

Description was changed from

==========
EventSender<T> is better off being on the Oilpan heap.

R=
BUG=
==========

to

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=
BUG=
==========

[sof, 2016-01-02 06:22:00]

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

[sof, 2016-01-02 06:22:00]

please take a look.

The interesting question to clarify here is the use of Member<> on EventSender<>
- is the (slightly extended) lifetime of that what we want to support with
Oilpan?

[haraken, 2016-01-02 08:30:39]

LGTM

[sof, 2016-01-02 08:46:02]

On 2016/01/02 06:22:00, sof wrote:
> please take a look.
> 
> The interesting question to clarify here is the use of Member<> on
EventSender<>
> - is the (slightly extended) lifetime of that what we want to support with
> Oilpan?

Notice that we keep a weak reference non-Oilpan, but not with.

I haven't seen (nor come up with) a clear motivation why that change is
acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e., keeping the
sender alive until the event has been dispatched.

[haraken, 2016-01-02 08:51:55]

On 2016/01/02 08:46:02, sof wrote:
> On 2016/01/02 06:22:00, sof wrote:
> > please take a look.
> > 
> > The interesting question to clarify here is the use of Member<> on
> EventSender<>
> > - is the (slightly extended) lifetime of that what we want to support with
> > Oilpan?
> 
> Notice that we keep a weak reference non-Oilpan, but not with.
> 
> I haven't seen (nor come up with) a clear motivation why that change is
> acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e., keeping
the
> sender alive until the event has been dispatched.

EventSender is used by layout tests only, right? If so, I don't worry about the
lifetime issue if all layout tests pass :)

[sof, 2016-01-02 08:59:06]

On 2016/01/02 08:51:55, haraken wrote:
> On 2016/01/02 08:46:02, sof wrote:
> > On 2016/01/02 06:22:00, sof wrote:
> > > please take a look.
> > > 
> > > The interesting question to clarify here is the use of Member<> on
> > EventSender<>
> > > - is the (slightly extended) lifetime of that what we want to support with
> > > Oilpan?
> > 
> > Notice that we keep a weak reference non-Oilpan, but not with.
> > 
> > I haven't seen (nor come up with) a clear motivation why that change is
> > acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e., keeping
> the
> > sender alive until the event has been dispatched.
> 
> EventSender is used by layout tests only, right? If so, I don't worry about
the
> lifetime issue if all layout tests pass :)

You're thinking of some other abstraction wrt test-only use? cf. use of
EventSender to handle image&stylesheet onload delivery.

[haraken, 2016-01-02 09:15:57]

On 2016/01/02 08:59:06, sof wrote:
> On 2016/01/02 08:51:55, haraken wrote:
> > On 2016/01/02 08:46:02, sof wrote:
> > > On 2016/01/02 06:22:00, sof wrote:
> > > > please take a look.
> > > > 
> > > > The interesting question to clarify here is the use of Member<> on
> > > EventSender<>
> > > > - is the (slightly extended) lifetime of that what we want to support
with
> > > > Oilpan?
> > > 
> > > Notice that we keep a weak reference non-Oilpan, but not with.
> > > 
> > > I haven't seen (nor come up with) a clear motivation why that change is
> > > acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e.,
keeping
> > the
> > > sender alive until the event has been dispatched.
> > 
> > EventSender is used by layout tests only, right? If so, I don't worry about
> the
> > lifetime issue if all layout tests pass :)
> 
> You're thinking of some other abstraction wrt test-only use? cf. use of
> EventSender to handle image&stylesheet onload delivery.

ah, now I understand what you mean.

So, are you suggesting replacing the Members on the EventSender with
WeakMembers? That sounds like a reasonable change.

[sof, 2016-01-02 12:57:01]

On 2016/01/02 09:15:57, haraken wrote:
> On 2016/01/02 08:59:06, sof wrote:
> > On 2016/01/02 08:51:55, haraken wrote:
> > > On 2016/01/02 08:46:02, sof wrote:
> > > > On 2016/01/02 06:22:00, sof wrote:
> > > > > please take a look.
> > > > > 
> > > > > The interesting question to clarify here is the use of Member<> on
> > > > EventSender<>
> > > > > - is the (slightly extended) lifetime of that what we want to support
> with
> > > > > Oilpan?
> > > > 
> > > > Notice that we keep a weak reference non-Oilpan, but not with.
> > > > 
> > > > I haven't seen (nor come up with) a clear motivation why that change is
> > > > acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e.,
> keeping
> > > the
> > > > sender alive until the event has been dispatched.
> > > 
> > > EventSender is used by layout tests only, right? If so, I don't worry
about
> > the
> > > lifetime issue if all layout tests pass :)
> > 
> > You're thinking of some other abstraction wrt test-only use? cf. use of
> > EventSender to handle image&stylesheet onload delivery.
> 
> ah, now I understand what you mean.
> 
> So, are you suggesting replacing the Members on the EventSender with
> WeakMembers? That sounds like a reasonable change.

I want to determine if it is required first; understanding why it is a weak
reference non-Oilpan, seems key.

[sof, 2016-01-02 14:31:10]

On 2016/01/02 12:57:01, sof wrote:
> On 2016/01/02 09:15:57, haraken wrote:
> > On 2016/01/02 08:59:06, sof wrote:
> > > On 2016/01/02 08:51:55, haraken wrote:
> > > > On 2016/01/02 08:46:02, sof wrote:
> > > > > On 2016/01/02 06:22:00, sof wrote:
> > > > > > please take a look.
> > > > > > 
> > > > > > The interesting question to clarify here is the use of Member<> on
> > > > > EventSender<>
> > > > > > - is the (slightly extended) lifetime of that what we want to
support
> > with
> > > > > > Oilpan?
> > > > > 
> > > > > Notice that we keep a weak reference non-Oilpan, but not with.
> > > > > 
> > > > > I haven't seen (nor come up with) a clear motivation why that change
is
> > > > > acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e.,
> > keeping
> > > > the
> > > > > sender alive until the event has been dispatched.
> > > > 
> > > > EventSender is used by layout tests only, right? If so, I don't worry
> about
> > > the
> > > > lifetime issue if all layout tests pass :)
> > > 
> > > You're thinking of some other abstraction wrt test-only use? cf. use of
> > > EventSender to handle image&stylesheet onload delivery.
> > 
> > ah, now I understand what you mean.
> > 
> > So, are you suggesting replacing the Members on the EventSender with
> > WeakMembers? That sounds like a reasonable change.
> 
> I want to determine if it is required first; understanding why it is a weak
> reference non-Oilpan, seems key.

The need for weak references stems from way-back-when these delayed event
dispatching ImageLoader lists were kept on Document (
http://trac.webkit.org/changeset/39296 ), avoiding ref-cycle risks.

When EventSender<> singletons were eventually introduced, the weakness could
have been retired, but was kept that way for no real reasons, as far as I've
been able to determine.

It would be a simpler story to have the (non)dispatching of these delayed events
not be dependent of GC timings, hence I suggest we keep this as it is on the
Oilpan side and not introduce WeakMember<> on EventSender<>.

[haraken, 2016-01-02 14:40:05]

On 2016/01/02 14:31:10, sof wrote:
> On 2016/01/02 12:57:01, sof wrote:
> > On 2016/01/02 09:15:57, haraken wrote:
> > > On 2016/01/02 08:59:06, sof wrote:
> > > > On 2016/01/02 08:51:55, haraken wrote:
> > > > > On 2016/01/02 08:46:02, sof wrote:
> > > > > > On 2016/01/02 06:22:00, sof wrote:
> > > > > > > please take a look.
> > > > > > > 
> > > > > > > The interesting question to clarify here is the use of Member<> on
> > > > > > EventSender<>
> > > > > > > - is the (slightly extended) lifetime of that what we want to
> support
> > > with
> > > > > > > Oilpan?
> > > > > > 
> > > > > > Notice that we keep a weak reference non-Oilpan, but not with.
> > > > > > 
> > > > > > I haven't seen (nor come up with) a clear motivation why that change
> is
> > > > > > acceptable (cf. https://codereview.chromium.org/320253002/ ), i.e.,
> > > keeping
> > > > > the
> > > > > > sender alive until the event has been dispatched.
> > > > > 
> > > > > EventSender is used by layout tests only, right? If so, I don't worry
> > about
> > > > the
> > > > > lifetime issue if all layout tests pass :)
> > > > 
> > > > You're thinking of some other abstraction wrt test-only use? cf. use of
> > > > EventSender to handle image&stylesheet onload delivery.
> > > 
> > > ah, now I understand what you mean.
> > > 
> > > So, are you suggesting replacing the Members on the EventSender with
> > > WeakMembers? That sounds like a reasonable change.
> > 
> > I want to determine if it is required first; understanding why it is a weak
> > reference non-Oilpan, seems key.
> 
> The need for weak references stems from way-back-when these delayed event
> dispatching ImageLoader lists were kept on Document (
> http://trac.webkit.org/changeset/39296 ), avoiding ref-cycle risks.
> 
> When EventSender<> singletons were eventually introduced, the weakness could
> have been retired, but was kept that way for no real reasons, as far as I've
> been able to determine.
> 
> It would be a simpler story to have the (non)dispatching of these delayed
events
> not be dependent of GC timings, hence I suggest we keep this as it is on the
> Oilpan side and not introduce WeakMember<> on EventSender<>.

Thanks for the digging. Makes sense. LGTM.

[sof, 2016-01-02 14:44:52]

Description was changed from

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=
BUG=
==========

to

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515
==========

[sof, 2016-01-02 14:45:15]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-01-02 14:45:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1554903002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1554903002/20001

[commit-bot: I haz the power, 2016-01-02 15:32:25]

Description was changed from

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515
==========

to

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515
==========

[commit-bot: I haz the power, 2016-01-02 15:32:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-02 15:33:38]

Description was changed from

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515
==========

to

==========
EventSender<T> singletons are better off on the Oilpan heap.

Rather than keeping two off-heap persistent collections per EventSender
singleton, have the singletons reside on the Oilpan heap instead.

This also removes no-op cancelEvent()s from various destructors.

R=haraken
BUG=420515

Committed: https://crrev.com/2d9b40462e8b813e7d9bc3ebc4be6b4c40ecd0d0
Cr-Commit-Position: refs/heads/master@{#367282}
==========

[commit-bot: I haz the power, 2016-01-02 15:33:39]

Patchset 2 (id:??) landed as
https://crrev.com/2d9b40462e8b813e7d9bc3ebc4be6b4c40ecd0d0
Cr-Commit-Position: refs/heads/master@{#367282}