Implement lifetime observer on RenderWidgetHostViewBase.

Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3912c64fb9b8e5ad166060a91ccb752914016029
Cr-Commit-Position: refs/heads/master@{#378133}

[wjmaclean, 2016-02-18 20:36:54]

Description was changed from

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
==========

to

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-02-18 20:38:12]

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

[commit-bot: I haz the power, 2016-02-18 20:42:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711103002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711103002/1

[commit-bot: I haz the power, 2016-02-18 22:39:28]

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

[commit-bot: I haz the power, 2016-02-18 22:39:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2016-02-19 14:29:52]

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

[commit-bot: I haz the power, 2016-02-19 14:30:38]

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

[wjmaclean, 2016-02-19 16:57:25]

Description was changed from

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2016-02-19 16:57:26]

wjmaclean@chromium.org changed reviewers:
+ kenrb@chromium.org

[wjmaclean, 2016-02-19 16:57:58]

kenrb@ - could you please give this an initial lookover?

[commit-bot: I haz the power, 2016-02-19 17:45:28]

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

[commit-bot: I haz the power, 2016-02-19 17:45:29]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-02-22 17:05:10]

A few minor comments. In general I think this is more robust than the original
way of registering/deregistering for surface hit testing. Also, this might make
writing some tests easier.

You'll have to make sure content/ owners are okay with the observer pattern
being used in this way.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:206: //
SurfaceIdNames registered with RenderWidgetHostInputEventRouter
nit: s/SurfaceIdNames/SurfaceIdNamespaces

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:26: //
next line.
Yes, it should be an invariant that a RWHV is only in the map once.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2490: // this next
call just returns.
Is this comment necessary? RWHVA::Destroy() doesn't seem to notify observers.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:391: //
de-register on the first call, and so the second call does nothing.
Do you know of any case where this is actually needed?

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1052: //
SurfaceIdNames registered with RenderWidgetHostInputEventRouter
nit: s/SurfaceIdNames/SurfaceIdNamespaces

[wjmaclean, 2016-02-22 20:48:48]

Patchset #3 (id:40001) has been deleted

[wjmaclean, 2016-02-22 21:02:05]

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

[commit-bot: I haz the power, 2016-02-22 21:04:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711103002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711103002/60001

[commit-bot: I haz the power, 2016-02-22 22:37:41]

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

[commit-bot: I haz the power, 2016-02-22 22:37:42]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-02-23 13:10:00]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org

[wjmaclean, 2016-02-23 13:10:37]

wjmaclean@chromium.org changed reviewers:
- creis@chromium.org

[wjmaclean, 2016-02-23 13:12:55]

wjmaclean@chromium.org changed reviewers:
+ nick@chromium.org

[wjmaclean, 2016-02-23 13:13:26]

nick - could you take a look at this for OWNERS approval?

https://codereview.chromium.org/1711103002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:206: //
SurfaceIdNames registered with RenderWidgetHostInputEventRouter
On 2016/02/22 17:05:10, kenrb wrote:
> nit: s/SurfaceIdNames/SurfaceIdNamespaces

Done.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_input_event_router.cc:26: //
next line.
On 2016/02/22 17:05:10, kenrb wrote:
> Yes, it should be an invariant that a RWHV is only in the map once.

Done.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2490: // this next
call just returns.
On 2016/02/22 17:05:10, kenrb wrote:
> Is this comment necessary? RWHVA::Destroy() doesn't seem to notify observers.

Done.

Ooops, slightly stale comment, removed. Actually, we may be able to remove this
call to NotifyObserversAboutShutdown() altogether, since we don't destroy
anything like a host_ here, and the RWHVB destructor will call this function
shortly.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.cc:391: //
de-register on the first call, and so the second call does nothing.
On 2016/02/22 17:05:10, kenrb wrote:
> Do you know of any case where this is actually needed?

My hope was that, by putting this here, then only sub-classes with special needs
around notifying earlier (e.g. RWHVCF, RWHVG, RWHVMac) would have to make
earlier calls, and all other sub-classes could just default to having it called
here.

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1711103002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1052: //
SurfaceIdNames registered with RenderWidgetHostInputEventRouter
On 2016/02/22 17:05:10, kenrb wrote:
> nit: s/SurfaceIdNames/SurfaceIdNamespaces

Done.

[wjmaclean, 2016-02-24 18:18:49]

wjmaclean@chromium.org changed reviewers:
+ jochen@chromium.org
- nick@chromium.org

[wjmaclean, 2016-02-24 18:19:31]

jochen@ - can you take a look? My usual reviewers for this sort of stuff are all
unavailable.

[jochen (gone - plz use gerrit), 2016-02-25 11:52:44]

jochen@chromium.org changed reviewers:
+ avi@chromium.org

[jochen (gone - plz use gerrit), 2016-02-25 11:52:45]

Avi is a better reviewer for this

[Avi (use Gerrit), 2016-02-25 22:03:15]

On 2016/02/25 11:52:45, jochen wrote:
> Avi is a better reviewer for this

I'm OK with this. Ken, what was your concern about observers being "used in this
way"?

[kenrb, 2016-02-26 14:11:52]

On 2016/02/25 22:03:15, Avi wrote:
> On 2016/02/25 11:52:45, jochen wrote:
> > Avi is a better reviewer for this
> 
> I'm OK with this. Ken, what was your concern about observers being "used in
this
> way"?

Nothing in particular, I was just recalling a reduction in use of some other
kinds of observers, and wasn't sure of the implications of that.

When this lands I would like to add a notifier for OnSwapCompositorFrame to
this.

[Avi (use Gerrit), 2016-02-26 15:48:58]

On 2016/02/26 14:11:52, kenrb wrote:
> On 2016/02/25 22:03:15, Avi wrote:
> > On 2016/02/25 11:52:45, jochen wrote:
> > > Avi is a better reviewer for this
> > 
> > I'm OK with this. Ken, what was your concern about observers being "used in
> this
> > way"?
> 
> Nothing in particular, I was just recalling a reduction in use of some other
> kinds of observers, and wasn't sure of the implications of that.
> 
> When this lands I would like to add a notifier for OnSwapCompositorFrame to
> this.

LGTM then.

I would much rather have observers than play the game of weak pointers.

[wjmaclean, 2016-02-27 00:34:26]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-02-27 00:44:22]

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

[commit-bot: I haz the power, 2016-02-27 01:12:15]

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

[commit-bot: I haz the power, 2016-02-27 01:12:17]

Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[wjmaclean, 2016-02-27 12:17:32]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-02-27 12:17:40]

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

[commit-bot: I haz the power, 2016-02-27 12:23:58]

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

[commit-bot: I haz the power, 2016-02-27 12:23:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wjmaclean, 2016-02-27 18:55:11]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2016-02-27 18:55:11]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1711103002/#ps80001
(title: "Rebased to r378132.")

[commit-bot: I haz the power, 2016-02-27 18:55:48]

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

[commit-bot: I haz the power, 2016-02-27 23:26:49]

Description was changed from

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-02-27 23:26:51]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-02-27 23:28:24]

Description was changed from

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement lifetime observer on RenderWidgetHostViewBase.

It is important for RenderWidgetHostInputEventRouter to know about the
destruction of RenderWidgetHostViewBase objects it owns pointers to.
In the past we have encountered issues with stale pointers, and until
now dealt with this using WeakPtrs to the views. But this approach still
requires checking pointers before use to see if they are stale.

This CL makes RenderWidgetHostInputEventRouter an observer of the
RenderWidgetHostViewBase objects it holds pointers to in order to avoid
holding stale pointers.

BUG=583379
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/3912c64fb9b8e5ad166060a91ccb752914016029
Cr-Commit-Position: refs/heads/master@{#378133}
==========

[commit-bot: I haz the power, 2016-02-27 23:28:26]

Patchset 4 (id:??) landed as
https://crrev.com/3912c64fb9b8e5ad166060a91ccb752914016029
Cr-Commit-Position: refs/heads/master@{#378133}