Poll for Display rotation on Android 4.0 and 4.1 when needed.

Poll for Display rotation on Android 4.0 and 4.1 when needed.

In this context, when needed means when Screen Orientation API is being
used.

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

[mlamouri (slow - plz ping), 2014-07-16 16:57:38]

Depends on https://codereview.chromium.org/398003002/

[mlamouri (slow - plz ping), 2014-07-16 17:00:37]

cevans@chromium.org: please review changes in 
* content/common/screen_orientation_messages.h

jam@chromium.org: please review changes in 
* content/renderer/
* content/browser/

mvanouwerkerk@chromium.org: please review changes in:
* content/browser/screen_orientation/
* content/public/android/

Thanks :)

[jam, 2014-07-16 21:29:14]

lgtm with comments

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:19:
{
nit: on previous line per style guide

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:37:
__android_log_print(ANDROID_LOG_WARN, "MOUNIR", "OnStartListening");
here and below, use our standard LOG macros

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_provider_android.h
(right):

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_provider_android.h:35:
static void ToggleAccurateMode();
nit: add comment

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1155:
//------------------------------------------------------------------------------
why?

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

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:158: virtual void
startScreenOrientationListening();
nit: all the overridden methods are beside each other with no blank lines, so
move this up to 153 (assuming it's at the end of the interface, otherwise put it
in the right order). ditto for the cc file

[mlamouri (slow - plz ping), 2014-07-17 09:44:16]

Chris and Michael, PTAL.

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:19:
{
On 2014/07/16 21:29:14, jam wrote:
> nit: on previous line per style guide

With { on the previous line, the line gets longer than 80 characters.

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:37:
__android_log_print(ANDROID_LOG_WARN, "MOUNIR", "OnStartListening");
On 2014/07/16 21:29:14, jam wrote:
> here and below, use our standard LOG macros

Oups, that did not meant to be here... It was only for debug purposes.

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
File content/browser/screen_orientation/screen_orientation_provider_android.h
(right):

https://codereview.chromium.org/398013002/diff/40001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_provider_android.h:35:
static void ToggleAccurateMode();
On 2014/07/16 21:29:14, jam wrote:
> nit: add comment

Done.

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1155:
//------------------------------------------------------------------------------
On 2014/07/16 21:29:14, jam wrote:
> why?

This seems to be a convention in this file where there are sections separated
like that. Just following the local style.

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

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.h:158: virtual void
startScreenOrientationListening();
On 2014/07/16 21:29:14, jam wrote:
> nit: all the overridden methods are beside each other with no blank lines, so
> move this up to 153 (assuming it's at the end of the interface, otherwise put
it
> in the right order). ditto for the cc file

Done.

[Michael van Ouwerkerk, 2014-07-17 10:35:03]

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:42:
if (listeners_count_ == 0)
I think there's a bug here. You have a message filter per render process, right?
If that's true, this counter could go to 0 for one process and disable polling,
while other processes still require it.

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.h
(right):

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.h:8:
#include "content/browser/battery_status/battery_status_service.h"
I don't think you need this.

[mlamouri (slow - plz ping), 2014-07-17 12:46:09]

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:42:
if (listeners_count_ == 0)
On 2014/07/17 10:35:02, Michael van Ouwerkerk wrote:
> I think there's a bug here. You have a message filter per render process,
right?
> If that's true, this counter could go to 0 for one process and disable
polling,
> while other processes still require it.

Oh. That's a good point. Someone, I had a mental model where we had one
RenderProcessHost in the browser process but we actually have one _per_ renderer
process. Will change the design to fix that.

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.h
(right):

https://codereview.chromium.org/398013002/diff/60001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.h:8:
#include "content/browser/battery_status/battery_status_service.h"
On 2014/07/17 10:35:02, Michael van Ouwerkerk wrote:
> I don't think you need this.

Oups.

[mlamouri (slow - plz ping), 2014-07-17 13:50:20]

PTAL

[jam, 2014-07-17 17:19:52]

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/398013002/diff/40001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1155:
//------------------------------------------------------------------------------
On 2014/07/17 09:44:16, Mounir Lamouri wrote:
> On 2014/07/16 21:29:14, jam wrote:
> > why?
> 
> This seems to be a convention in this file where there are sections separated
> like that. Just following the local style.

this file's convention is wrong. some files have "/-----..." between overridden
methods from different interfaces. but in this case, there are separaters
between overridden methods from the same interface which doesn't make sense.
please don't add a new one. if you're so inclined, you can remove the existing
ones.

also, order these new files to match the header, i.e. move them above the
previous method

[mlamouri (slow - plz ping), 2014-07-21 09:52:04]

nasko@, can you have a look at the IPC changes?

[nasko, 2014-07-21 10:00:00]

IPC LGTM when the comment is addressed.

https://codereview.chromium.org/398013002/diff/80001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:13:
: BrowserMessageFilter(ScreenOrientationMsgStart) {
No explicit initializer for listeners_count_.

[Michael van Ouwerkerk, 2014-07-21 10:56:51]

lgtm

[mlamouri (slow - plz ping), 2014-08-07 10:44:54]

I applied all the review comments and rebased the CL on top of cl 417213002 and
cl 419643008 which solves the comment about where
::startListeningForScreenOrientation and the stop counterpart were defined.

https://codereview.chromium.org/398013002/diff/80001/content/browser/screen_o...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):

https://codereview.chromium.org/398013002/diff/80001/content/browser/screen_o...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:13:
: BrowserMessageFilter(ScreenOrientationMsgStart) {
On 2014/07/21 10:00:00, nasko (in Munich) wrote:
> No explicit initializer for listeners_count_.

Done.

[mlamouri (slow - plz ping), 2014-08-07 10:45:59]

I forgot to point that it is also rebased on top of 421293010 which isn't landed
yet.

[mlamouri (slow - plz ping), 2014-08-07 16:18:02]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-07 16:19:01]

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

[commit-bot: I haz the power, 2014-08-07 20:00:25]

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

[commit-bot: I haz the power, 2014-08-07 20:25:24]

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

[commit-bot: I haz the power, 2014-08-07 20:25:30]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[mlamouri (slow - plz ping), 2014-08-07 20:42:30]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:54:05]

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

[commit-bot: I haz the power, 2014-08-08 08:01:45]

Change committed as 288243