[ScreenOrientation] Merge mojo interface ScreenOrientationListener into ScreenOrientation

[ScreenOrientation] Merge mojo interface ScreenOrientationListener into ScreenOrientation

This refactor's goal is to consolidate device.mojom.ScreenOrientationListener
together with device.mojom.ScreenOrientation, so that all the
mojo calls for screen orientation could run on single mojo connection.

Before, both of these two interfaces are associated with the legacy IPC channel
to keep ordering constraints between them, but after this CL because all
of the mojo calls reside in one interface, they just keep ordering naturally.

Thus, once we tackled ordering constraints between lock/unlock and legacy IPC
ViewMsg_Resize later, we could de-associate device.mojom.ScreenOrientation
interface from the legacy IPC channel entirely.
A follow-up CL would do this.

BUG=678545

[leonhsl(Using Gerrit), 2017-02-12 10:08:59]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-12 10:09:06]

Dry run: 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, 2017-02-12 11:58:36]

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

[commit-bot: I haz the power, 2017-02-12 11:58:37]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-02-12 12:01:00]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-12 12:01:05]

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

[leonhsl(Using Gerrit), 2017-02-12 12:01:45]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-12 13:35:50]

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

[commit-bot: I haz the power, 2017-02-12 13:35:53]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-02-12 13:43:31]

leon.han@intel.com changed reviewers:
+ blundell@chromium.org, mlamouri@chromium.org, rockot@chromium.org

[leonhsl(Using Gerrit), 2017-02-12 13:43:32]

Hi, mlamouri@, rockot@, would you please help to take an overall review? Thanks.

[Ken Rockot(use gerrit already), 2017-02-13 20:06:03]

mojom changes and relevant usage all LGTM

[leonhsl(Using Gerrit), 2017-02-15 05:43:44]

Friendly ping mlamouri@ :)

[haraken, 2017-02-15 07:02:49]

WebKit LGTM

[leonhsl(Using Gerrit), 2017-02-15 07:17:01]

On 2017/02/15 07:02:49, haraken wrote:
> WebKit LGTM

Hara-san, Thanks!

[mlamouri (slow - plz ping), 2017-02-15 10:58:40]

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

[mlamouri (slow - plz ping), 2017-02-15 10:58:41]

The code itself looks fine but I would like to know more about the long term
approach. I wrote this code a while ago but as far as I can remember, these two
interfaces were split because one was frame-bounded and the other was not. I
can't remember any messaging ordering issues related to this.

Do you have a design doc?

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider.cc:35: }
style: no { }

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider.h (right):

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider.h:72: int
accurate_listen_count_ = 0;
nit: `accurate_listener_count_`? feel free to ignore

[leonhsl(Using Gerrit), 2017-02-15 13:41:24]

On 2017/02/15 10:58:41, mlamouri wrote:
> The code itself looks fine but I would like to know more about the long term
> approach. I wrote this code a while ago but as far as I can remember, these
two
> interfaces were split because one was frame-bounded and the other was not. I
> can't remember any messaging ordering issues related to this.
> 
> Do you have a design doc?
> 
>
https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
> File content/browser/screen_orientation/screen_orientation_provider.cc
(right):
> 
>
https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
> content/browser/screen_orientation/screen_orientation_provider.cc:35: }
> style: no { }
> 
>
https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
> File content/browser/screen_orientation/screen_orientation_provider.h (right):
> 
>
https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
> content/browser/screen_orientation/screen_orientation_provider.h:72: int
> accurate_listen_count_ = 0;
> nit: `accurate_listener_count_`? feel free to ignore

Yeah I've created the design doc and shared to you and blundell@ :
https://docs.google.com/document/d/1u9S6AIxL3W2zo3IhqMrsDyrfJt-nbT_z3J-prLvJb...
The doc analyzes the IPC ordering constraints between mojo interface
ScreenOrientationListener, ScreenOrientation and legacy IPC ViewMsg_Resize. It's
true that ScreenOrientation is frame-level interface and
ScreenOrientationListener is process-level interface, but I think this CL does
not bring anything bad compared with before, this CL:
  - keeps IPC ordering exact with before,
  - counts those render frames wanting to start/stop accurate listening
per-WebContents, instead of counting them per-RendererProcess before. Before,
blink::ScreenOrientationDispatcher(singleton) counts all those accurate listener
frames per renderer process, once the count flips, triggers mojo call start/stop
to browser side to do actual start/stop; after this CL, every accurate listener
frame will send start/stop request to browser side by themselves, in browser
side, the per-WebContents impl of interface ScreenOrientation will incre/decre a
counter accordingly, once the counter flips, do actual start/stop. I suppose
this should also have no problem. Also, no matter counting per-WebContents or
per-RenderProcess, ScreenOrientationDelegateAndroid start/stop also have another
counter inside, to decide when to call Android system APIs to start/stop.

This CL is the first step, and the next step is to release interface
ScreenOrientation from depending on legacy IPC ViewMsg_Resize, according with
your suggestions before :)
“
Yes. That's what ScreenOrientationProviderAndroid is doing: when there is an
orientation change, if it matches the lock, it is marked as a success lock. The
reason why it's doing that is that the Android API doesn't tell us when the lock
is done. That logic could definitely live in the renderer process. It would
remove the need for synchronizing the IPCs.
“

By the way, would you please also help to answer the question at the end of doc?
Thanks.

[leonhsl(Using Gerrit), 2017-02-16 02:27:50]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 02:29:04]

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

[leonhsl(Using Gerrit), 2017-02-16 05:00:05]

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider.cc:35: }
On 2017/02/15 10:58:41, mlamouri wrote:
> style: no { }

Done.

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
File content/browser/screen_orientation/screen_orientation_provider.h (right):

https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_provider.h:72: int
accurate_listen_count_ = 0;
On 2017/02/15 10:58:41, mlamouri wrote:
> nit: `accurate_listener_count_`? feel free to ignore

Done.

[commit-bot: I haz the power, 2017-02-16 07:27:13]

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

[commit-bot: I haz the power, 2017-02-16 07:27:14]

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

[leonhsl(Using Gerrit), 2017-02-16 08:04:47]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 08:05:55]

Dry run: 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, 2017-02-16 11:03:05]

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

[commit-bot: I haz the power, 2017-02-16 11:03:06]

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

[leonhsl(Using Gerrit), 2017-02-16 13:01:33]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 13:01:58]

Dry run: 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, 2017-02-16 14:20:11]

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

[commit-bot: I haz the power, 2017-02-16 14:20:12]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-02-17 12:04:06]

Leon, could you specify exactly which questions you want an answer to?

I was hoping to find something summarizing the mojo service design.

I think it would make more sense to have two mojo service: one that would be
only created when "accurate listening" is needed. It would be connected to
render processes instead of render frames. The other mojo service would be for
the API itself (thus per frame).

The "accurate listening" feature is specific to Android J and having it in a
different service would probably be cleaner and avoid confusion. It would also
make it easier to remove. Does that sound compatible with your current plan?

[leonhsl(Using Gerrit), 2017-02-17 13:27:44]

On 2017/02/17 12:04:06, mlamouri wrote:
> Leon, could you specify exactly which questions you want an answer to?
> 
I was asking about the second bullet at
https://docs.google.com/document/d/1u9S6AIxL3W2zo3IhqMrsDyrfJt-nbT_z3J-prLvJb...,
the text 'One question' in red.

> I was hoping to find something summarizing the mojo service design.
> 
> I think it would make more sense to have two mojo service: one that would be
> only created when "accurate listening" is needed. It would be connected to
> render processes instead of render frames. The other mojo service would be for
> the API itself (thus per frame).
>
Yeah currently the "accurate listening" interface is bound per-RenderProcess,
but this CL is changing it to be bound per-RenderFrame, because I think the
"accurate listening" start/stop requests can be handled per-RenderFrame, too,
the behavior won't become worse than binding it per-RenderProcess. I'm not aware
yet why we must keep this interface per-RenderProcess.. Is there any special
reason for this?

Once both of these 2 mojo interfaces got bound per-RenderFrame, we can easily
keep FIFO ordering for all interface functions of them, either by combining them
into only 1 interface(i.e. this CL is doing so), or by using mojo associated
interfaces to associate these 2 interfaces with 1 underlying mojo message pipe.
Seems we can't associate one RenderProcess-level interface with another
RenderFrame-level interface..
 
> The "accurate listening" feature is specific to Android J and having it in a
> different service would probably be cleaner and avoid confusion. It would also
> make it easier to remove. Does that sound compatible with your current plan?
Currently, Blink code does not know it's running on Android or not, so it always
tries to connect to the "accurate listening" interface, but, in browser side the
interface impl is registered only for Android, so on any non-Android platforms
there always have such ERROR log "Request for unknown Channel-associated
interface: device::mojom::ScreenOrientationListener", which is generated here
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro....
A similar case is device.mojom.VibrationManager, it has concrete impl only for
Android, but it also provides an empty impl for other platforms. Maybe we can
say "accurate listening" interface has concrete impl only for Android, but the
interface itself is not Android specific, because its user(Blink) can't be aware
of this.

Of course we are still able to keep this interface separate with that interface
for screen orientation APIs, just as explained above, we can use mojo associated
interfaces to associate them. But seems it's not so necessary and somehow
becomes complicated, from my point of view :)

[leonhsl(Using Gerrit), 2017-02-18 16:45:05]

On 2017/02/17 13:27:44, leonhsl wrote:
> On 2017/02/17 12:04:06, mlamouri wrote:
> > Leon, could you specify exactly which questions you want an answer to?
> > 
> I was asking about the second bullet at
>
https://docs.google.com/document/d/1u9S6AIxL3W2zo3IhqMrsDyrfJt-nbT_z3J-prLvJb...,
> the text 'One question' in red.

I prepared another CL https://codereview.chromium.org/2702033002/ about this
question, maybe you can directly look there to see whether it makes sense :)