Refactor code listening to platform events in content/renderer/.

Refactor code listening to platform events in content/renderer/.

This is introducing PlatformEventObserver, being used by code observing
and requesting platform events like Gamepad, Device Motion, Device
Orientation, Device Light and Battery.

BUG=400158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288555

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290563

[mlamouri (slow - plz ping), 2014-08-05 15:56:03]

Jochen and Tim, could you have a look at this CL? It is not entirely polished
but I would welcome early feedback.

(Note: I can't run tests because it depends on many other CLs but gamepad,
device motion, device orientation, device light and baterry layout tests are all
passing locally.)

[mlamouri (slow - plz ping), 2014-08-05 20:02:41]

+scottmg

[scottmg, 2014-08-05 21:09:19]

I don't think I'm an owner for anything here, but gamepad parts generally lg.
+bajones who's more familiar with gamepad of late.

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
File content/public/renderer/platform_event_observer.h (right):

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:53:
PlatformEventObserver(RenderThread* thread)
explicit

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:62: ~PlatformEventObserver() {
virtual, and out of line

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:105: ListenerType* listener_;
Oh, I see now. I don't know is this is against our rules, but I think it'd might
be clearer if this was private, with a protected listener().

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:108: bool is_observing;
is_observing_

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/renderer_gamepad_provider.h:26: // This
PlatformEventObserver implementation do not handle that.
nit; "do not handle that" -> "does not handle this"

https://codereview.chromium.org/446603002/diff/40001/content/renderer/gamepad...
File content/renderer/gamepad_shared_memory_reader.cc (right):

https://codereview.chromium.org/446603002/diff/40001/content/renderer/gamepad...
content/renderer/gamepad_shared_memory_reader.cc:127: if (listener_)
I'm sorry, I don't see where listener_ is set.

[bajones, 2014-08-05 21:19:56]

Gamepad changes LGTM with scottmg@'s comments addressed.

[mlamouri (slow - plz ping), 2014-08-05 21:32:37]

Thanks for the reviews.

Tim, could you have a look at the Device Event changes and the Battery changes?

Jochen, could you have a general architecture look?

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
File content/public/renderer/platform_event_observer.h (right):

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:53:
PlatformEventObserver(RenderThread* thread)
On 2014/08/05 21:09:18, scottmg wrote:
> explicit

Done.

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:62: ~PlatformEventObserver() {
On 2014/08/05 21:09:18, scottmg wrote:
> virtual, and out of line

Forgot the virtual... However, having it out of line given that I have to keep
that in the header doesn't sound particularly useful.

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:105: ListenerType* listener_;
On 2014/08/05 21:09:18, scottmg wrote:
> Oh, I see now. I don't know is this is against our rules, but I think it'd
might
> be clearer if this was private, with a protected listener().

Done.

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:108: bool is_observing;
On 2014/08/05 21:09:18, scottmg wrote:
> is_observing_

Done.

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/renderer_gamepad_provider.h:26: // This
PlatformEventObserver implementation do not handle that.
On 2014/08/05 21:09:18, scottmg wrote:
> nit; "do not handle that" -> "does not handle this"

Done.

https://codereview.chromium.org/446603002/diff/40001/content/renderer/gamepad...
File content/renderer/gamepad_shared_memory_reader.cc (right):

https://codereview.chromium.org/446603002/diff/40001/content/renderer/gamepad...
content/renderer/gamepad_shared_memory_reader.cc:127: if (listener_)
On 2014/08/05 21:09:19, scottmg wrote:
> I'm sorry, I don't see where listener_ is set.

I switched to |listener()|.

[scottmg, 2014-08-05 21:33:31]

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
File content/public/renderer/platform_event_observer.h (right):

https://codereview.chromium.org/446603002/diff/40001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:62: ~PlatformEventObserver() {
On 2014/08/05 21:32:36, Mounir Lamouri wrote:
> On 2014/08/05 21:09:18, scottmg wrote:
> > virtual, and out of line
> 
> Forgot the virtual... However, having it out of line given that I have to keep
> that in the header doesn't sound particularly useful.

Sorry, template, duh.

[mlamouri (slow - plz ping), 2014-08-07 09:27:18]

+jam@ given that jochen@ is marked as "slow - soon OOO"

[mlamouri (slow - plz ping), 2014-08-07 19:22:52]

jam@ is busy. Moving this to avi@.

[Avi (use Gerrit), 2014-08-07 19:56:11]

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
File content/public/renderer/platform_event_observer.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:42: // Creates a
PlatformEventObserver that doesn't listen to response from the
... listen to responses ...

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:61: // did not stop before.
... in case it did not stop before.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:68: // responses from the
browser process if any is expected.
... if any are expected.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:74: virtual void
Start(blink::WebPlatformEventListener* listener) {
Can you correctly type this parameter as ListenerType rather than
blink::WebPlatformEventListener?

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:95: virtual void
SendStartMessage() = 0;
It's "expected" to send? Do you mean that it should be overridden by subclasses
to do so? If so, say so, and if not, what do you mean?

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:98: /// know that it should
stop observing.
Same question as above. Also, fix ///.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/renderer_gamepad_provider.h:26: // This
PlatformEventObserver implementation do not handle this.
... does not handle this.

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:152: // the given |type|,
it will be overridden.
What does "overridden" mean here? Use "replaced" if that is what you mean.

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:153: // Note that
|observer| will be owned by this object after the call.
If you are taking ownership, indicate that in the code by making the parameter a
scoped_ptr. Don't annotate it with only comments.

[mlamouri (slow - plz ping), 2014-08-08 18:08:36]

Avi, I've applied your comments and also rebased this on ToT, which means that
it now includes the screen orientation api.

PTAL.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
File content/public/renderer/platform_event_observer.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:42: // Creates a
PlatformEventObserver that doesn't listen to response from the
On 2014/08/07 19:56:11, Avi wrote:
> ... listen to responses ...

Done.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:61: // did not stop before.
On 2014/08/07 19:56:11, Avi wrote:
> ... in case it did not stop before.

Done.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:68: // responses from the
browser process if any is expected.
On 2014/08/07 19:56:11, Avi wrote:
> ... if any are expected.

Done.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:74: virtual void
Start(blink::WebPlatformEventListener* listener) {
On 2014/08/07 19:56:11, Avi wrote:
> Can you correctly type this parameter as ListenerType rather than
> blink::WebPlatformEventListener?

Using ListenerType would require the callers to cast the listener to
ListenerType. RendererWebKitPlatformSupportImpl only has
blink::WebPlatformEventListener from ::startListening().

The goal is to keep the details of the implementation to the class implementing
this template.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:95: virtual void
SendStartMessage() = 0;
On 2014/08/07 19:56:11, Avi wrote:
> It's "expected" to send? Do you mean that it should be overridden by
subclasses
> to do so? If so, say so, and if not, what do you mean?

Done.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/platform_event_observer.h:98: /// know that it should
stop observing.
On 2014/08/07 19:56:11, Avi wrote:
> Same question as above. Also, fix ///.

Done.

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
File content/public/renderer/renderer_gamepad_provider.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/public/renderer/...
content/public/renderer/renderer_gamepad_provider.h:26: // This
PlatformEventObserver implementation do not handle this.
On 2014/08/07 19:56:11, Avi wrote:
> ... does not handle this.

Done.

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:152: // the given |type|,
it will be overridden.
On 2014/08/07 19:56:11, Avi wrote:
> What does "overridden" mean here? Use "replaced" if that is what you mean.

Done.

https://codereview.chromium.org/446603002/diff/60001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:153: // Note that
|observer| will be owned by this object after the call.
On 2014/08/07 19:56:11, Avi wrote:
> If you are taking ownership, indicate that in the code by making the parameter
a
> scoped_ptr. Don't annotate it with only comments.

Done.

[Avi (use Gerrit), 2014-08-08 18:13:15]

LGTM

[mlamouri (slow - plz ping), 2014-08-08 21:37:25]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-08 21:39:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/446603002/160001

[mlamouri (slow - plz ping), 2014-08-09 07:45:32]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-08-09 07:45:38]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-09 07:47:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/446603002/160001

[commit-bot: I haz the power, 2014-08-09 10:32:25]

Change committed as 288555

[Michael van Ouwerkerk, 2014-08-13 18:57:53]

Reverted here: https://codereview.chromium.org/470683002/

[mlamouri (slow - plz ping), 2014-08-18 15:16:56]

Michael, could you have a look at the changes I just made. I wasn't able to
reproduce the problem locally even with an ASan build but that CL is working and
it should fix the pointer owning issue.

(If you need a better interdiff, let me know, I can send you a patch.)

[mlamouri (slow - plz ping), 2014-08-19 09:33:10]

Michael can't review this and bajones@ didn't answer. I was able to reproduce
the ASAN error locally and have confirmation that my change fixed it. I will go
ahead and land this and apply any comment bajones@ might have in a follow-up.

[mlamouri (slow - plz ping), 2014-08-19 09:33:16]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-19 09:34:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/446603002/180001

[commit-bot: I haz the power, 2014-08-19 09:38:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-19 09:41:11]

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

[commit-bot: I haz the power, 2014-08-19 09:41:12]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[mlamouri (slow - plz ping), 2014-08-19 12:46:17]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-19 12:46:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/446603002/200001

[commit-bot: I haz the power, 2014-08-19 14:07:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-19 15:51:37]

Committed patchset #11 (200001) as 290563