Mojo-ify implementation of screen orientation locking/unlocking.

Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be passed from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d4a2cc818e19f277ea030e98cd86c477740ed1a6
Cr-Commit-Position: refs/heads/master@{#433714}

[lunalu1, 2016-10-05 20:51:16]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lunalu1, 2016-10-05 20:53:56]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
==========

[lunalu1, 2016-10-05 20:54:56]

lunalu@chromium.org changed reviewers:
+ blundell@chromium.org

[lunalu1, 2016-10-05 21:35:39]

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

[commit-bot: I haz the power, 2016-10-05 21:36:31]

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, 2016-10-05 21:40:04]

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

[commit-bot: I haz the power, 2016-10-05 21:40:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-10-06 19:36:48]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lunalu1, 2016-10-06 19:46:30]

Patchset #1 (id:1) has been deleted

[lunalu1, 2016-10-06 19:46:39]

Patchset #1 (id:20001) has been deleted

[lunalu1, 2016-10-06 19:46:47]

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

[commit-bot: I haz the power, 2016-10-06 19:47:28]

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, 2016-10-06 19:52:13]

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

[commit-bot: I haz the power, 2016-10-06 19:52:13]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2016-10-12 22:43:01]

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

[commit-bot: I haz the power, 2016-10-12 22:43:44]

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

[lunalu1, 2016-10-12 22:44:16]

Hi blundell,

Please help me take a look at this CL.

I moved the mojo enum definition from content/public/common to content/common
due to dependency issue.

Thanks

[commit-bot: I haz the power, 2016-10-12 22:55:31]

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

[commit-bot: I haz the power, 2016-10-12 22:55:32]

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

[lunalu1, 2016-10-13 13:56:26]

Patchset #3 (id:80001) has been deleted

[lunalu1, 2016-10-13 13:56:33]

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

[commit-bot: I haz the power, 2016-10-13 13:56:45]

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, 2016-10-13 14:10:23]

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

[commit-bot: I haz the power, 2016-10-13 14:10:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-10-13 15:17:49]

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

[commit-bot: I haz the power, 2016-10-13 15:18:03]

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

[lunalu1, 2016-10-13 15:21:22]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-10-13 15:33:59]

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

[commit-bot: I haz the power, 2016-10-13 15:34:00]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-10-13 17:34:42]

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

[commit-bot: I haz the power, 2016-10-13 17:35:08]

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

[lunalu1, 2016-10-13 17:36:08]

Patchset #3 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-10-13 17:51:57]

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

[commit-bot: I haz the power, 2016-10-13 17:51:58]

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

[blundell, 2016-10-14 15:16:07]

Hi Luna,

This is a good start! The biggest issue that I see is that this CL doesn't
address the ordering requirement between ViewMsg_Resize and the screen
orientation messages. I'm going to comment on the bug to figure out the right
way to address that with Ken. Once we have that info, we can proceed here.

[lunalu1, 2016-10-14 15:18:56]

On 2016/10/14 15:16:07, blundell wrote:
> Hi Luna,
> 
> This is a good start! The biggest issue that I see is that this CL doesn't
> address the ordering requirement between ViewMsg_Resize and the screen
> orientation messages. I'm going to comment on the bug to figure out the right
> way to address that with Ken. Once we have that info, we can proceed here.

Great! Thanks!

I also realized that I can't leave the mojom/struct_traits files in
content/public/common. I am in the process of moving it to WebKit (where the c++
definition for the enums are).

[blundell, 2016-10-14 15:23:13]

On 2016/10/14 15:18:56, loonybear wrote:
> On 2016/10/14 15:16:07, blundell wrote:
> > Hi Luna,
> > 
> > This is a good start! The biggest issue that I see is that this CL doesn't
> > address the ordering requirement between ViewMsg_Resize and the screen
> > orientation messages. I'm going to comment on the bug to figure out the
right
> > way to address that with Ken. Once we have that info, we can proceed here.
> 
> Great! Thanks!
> 
> I also realized that I can't leave the mojom/struct_traits files in
> content/public/common. I am in the process of moving it to WebKit (where the
c++
> definition for the enums are).

Sounds good. I would avoid making other significant changes to the CL because
the solution for associated ordering is likely to be intrusive (but it won't
affect the struct issues of course).

[Ken Rockot(use gerrit already), 2016-10-14 15:38:15]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-10-14 15:38:16]

Per blundell's question on the bug, the pattern you want to follow here is
probably https://codereview.chromium.org/2310583002, because this is a routed
per-frame message.

Note that while that CL takes advantage of the fact that NetErrorTabHelper is
already a WebContentsObserver, you don't need to worry about that.

The key points are:

- Use WebContentsFrameBindingSet<T> on the browser side
- Use GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr)
on the renderer side.
- When processing an incoming message, you can ask the
WebContentsFrameBindingSet which RenderFrameHost it's targeting.

For some background, this approach makes your interface share the same
underlying pipe as the legacy IPC Channel. It's not something we want to keep
around long-term, but it's great for converting messages without breaking
ordering.

https://codereview.chromium.org/2391883006/diff/140001/content/browser/screen...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(left):

https://codereview.chromium.org/2391883006/diff/140001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:50:
IPC_MESSAGE_HANDLER(ScreenOrientationHostMsg_LockRequest, OnLockRequest)
nit: Please also delete these message definitions from
screen_orientation_messages.h

https://codereview.chromium.org/2391883006/diff/140001/content/common/screen_...
File content/common/screen_orientation_service.mojom (right):

https://codereview.chromium.org/2391883006/diff/140001/content/common/screen_...
content/common/screen_orientation_service.mojom:9: interface
ScreenOrientationService {
naming nit: This is not a service, it's an interface. We can just call it
ScreenOrientation.

[lunalu1, 2016-10-17 14:29:50]

Patchset #4 (id:160001) has been deleted

[lunalu1, 2016-10-17 18:11:07]

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

[commit-bot: I haz the power, 2016-10-17 18:11:35]

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, 2016-10-17 18:20:03]

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

[commit-bot: I haz the power, 2016-10-17 18:20:04]

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

[lunalu1, 2016-10-17 18:31:27]

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

[lunalu1, 2016-10-17 18:31:39]

Patchset #5 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-10-17 18:32:47]

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, 2016-10-17 18:39:12]

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

[commit-bot: I haz the power, 2016-10-17 18:39:14]

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

[lunalu1, 2016-10-17 21:51:31]

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

[lunalu1, 2016-10-17 21:51:52]

Patchset #5 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-10-17 21:52:22]

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, 2016-10-17 22:03:15]

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

[commit-bot: I haz the power, 2016-10-17 22:03:16]

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

[lunalu1, 2016-10-18 14:35:27]

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

[commit-bot: I haz the power, 2016-10-18 14:35:53]

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, 2016-10-18 14:44:57]

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

[commit-bot: I haz the power, 2016-10-18 14:44:58]

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

[lunalu1, 2016-10-18 15:50:28]

Patchset #5 (id:240001) has been deleted

[lunalu1, 2016-10-18 17:23:49]

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

[lunalu1, 2016-10-18 17:23:56]

Patchset #5 (id:260001) has been deleted

[commit-bot: I haz the power, 2016-10-18 17:24:12]

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, 2016-10-18 17:32:26]

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

[commit-bot: I haz the power, 2016-10-18 17:32:27]

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

[lunalu1, 2016-10-20 17:37:54]

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

[commit-bot: I haz the power, 2016-10-20 17:38:39]

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, 2016-10-20 18:00:41]

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

[commit-bot: I haz the power, 2016-10-20 18:00:42]

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

[lunalu1, 2016-10-20 18:31:47]

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

[commit-bot: I haz the power, 2016-10-20 18:32:37]

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, 2016-10-20 18:44:20]

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

[commit-bot: I haz the power, 2016-10-20 18:44:21]

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

[lunalu1, 2016-10-20 19:02:04]

Patchset #6 (id:300001) has been deleted

[lunalu1, 2016-10-20 19:37:00]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
screen_orientation_service.mojom.
- The browser-side ScreenOrientationServiceImpl is per-RenderFrameHost, as
opposed to ScreenOrientationDispatcherHostImpl's being per-WebContents. This
changes is due to the fact that the connection established from the renderer is
a per-render frame connection. As a result, ScreenOrientationServiceImpl does
not have to do the muxing or RenderFrameHosts that
ScreenOrientationDispatcherHostImpl did.
- Request ID no longer have to be paased from the renderer to the browser, as
the renderer binds the request ID associated with a request in the response
callback that it creates for the request.

This CL does not attempt to decouple the screen orientation service from
//content. Decoupling ScreenOrientationServiceImpl and ScreenOrientationProvider
will be relatively easy: it will just require some fiddling so that
ScreenOrientationProvider::Create() no longer needs to take a WebContents.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be paased from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lunalu1, 2016-10-20 22:03:40]

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

[commit-bot: I haz the power, 2016-10-20 22:04:17]

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, 2016-10-20 22:27:11]

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

[commit-bot: I haz the power, 2016-10-20 22:27:13]

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

[lunalu1, 2016-10-20 23:19:38]

Patchset #6 (id:320001) has been deleted

[lunalu1, 2016-10-20 23:28:48]

Hi, 
Thank you for reviewing this CL.

I made two major changes:
1. Renamed screen orientation service to screen orientation (interface)
2. Used GetRemoteAssociatedInterface and WebContentsFrameBindingSet<T> to
re-ensure the ordering between ViewMsg_Resize and screen orientation messages. 

Please take another look.

Thanks

https://codereview.chromium.org/2391883006/diff/140001/content/browser/screen...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(left):

https://codereview.chromium.org/2391883006/diff/140001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:50:
IPC_MESSAGE_HANDLER(ScreenOrientationHostMsg_LockRequest, OnLockRequest)
On 2016/10/14 15:38:16, Ken Rockot wrote:
> nit: Please also delete these message definitions from
> screen_orientation_messages.h

Done.

https://codereview.chromium.org/2391883006/diff/140001/content/common/screen_...
File content/common/screen_orientation_service.mojom (right):

https://codereview.chromium.org/2391883006/diff/140001/content/common/screen_...
content/common/screen_orientation_service.mojom:9: interface
ScreenOrientationService {
On 2016/10/14 15:38:16, Ken Rockot wrote:
> naming nit: This is not a service, it's an interface. We can just call it
> ScreenOrientation.

Done.

[blundell, 2016-10-21 18:16:31]

Thanks!

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:174: virtual
ScreenOrientationImpl* GetScreenOrientation();
nit: I would just expose the ScreenOrientationProvider directly.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:55: #include
"content/browser/screen_orientation/screen_orientation_impl.h"
nit: why is this needed?

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(left):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:60:
if (!provider_ || details.is_in_page)
It seems like this logic is missing from the new impl?

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:27:
DCHECK(on_result_callback_.is_null());
This object is per-WebContents, the same as the old impl was. So you'll still
need to handle the case where a request comes in while a request was already
outstanding. The old impl handled that by simply raising an error on the
previously-outstanding request before handling the new request, so you can do
the same.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:56:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
Why this short-circuit?

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:18: class
ScreenOrientationImpl : public mojom::ScreenOrientation {
nit: the new convention is just to name this ScreenOrientation (that's why
there's the internal mojom:: namespace on generated code).

https://codereview.chromium.org/2391883006/diff/340001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1589:
screen_orientation_dispatcher_host_.reset(
Is there a reason that you moved where this object was created?

https://codereview.chromium.org/2391883006/diff/340001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/2391883006/diff/340001/content/common/BUILD.g...
content/common/BUILD.gn:560:
"//third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom",
Why is this built in //content/common and not in //third_party/WebKit?

https://codereview.chromium.org/2391883006/diff/340001/content/public/common/...
File content/public/common/screen_orientation.mojom (right):

https://codereview.chromium.org/2391883006/diff/340001/content/public/common/...
content/public/common/screen_orientation.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Is this duplicated with the one in //third_party/WebKit?

[lunalu1, 2016-10-24 15:27:04]

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

[commit-bot: I haz the power, 2016-10-24 15:27:27]

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, 2016-10-24 15:32:59]

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

[commit-bot: I haz the power, 2016-10-24 15:33:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[lunalu1, 2016-10-24 15:39:06]

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

[lunalu1, 2016-10-24 15:39:16]

Patchset #7 (id:360001) has been deleted

[commit-bot: I haz the power, 2016-10-24 15:39:27]

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, 2016-10-24 16:06:09]

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

[commit-bot: I haz the power, 2016-10-24 16:06:10]

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

[lunalu1, 2016-10-24 16:56:26]

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

[lunalu1, 2016-10-24 16:56:31]

Patchset #7 (id:380001) has been deleted

[commit-bot: I haz the power, 2016-10-24 16:56:54]

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

[lunalu1, 2016-10-24 16:57:36]

Thanks for reviewing my CL. I have made a few changes based on your comments.
Please take another look.

Thanks

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:174: virtual
ScreenOrientationImpl* GetScreenOrientation();
On 2016/10/21 18:16:31, blundell wrote:
> nit: I would just expose the ScreenOrientationProvider directly.

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:55: #include
"content/browser/screen_orientation/screen_orientation_impl.h"
On 2016/10/21 18:16:31, blundell wrote:
> nit: why is this needed?

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
File
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc
(left):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_dispatcher_host_impl.cc:60:
if (!provider_ || details.is_in_page)
On 2016/10/21 18:16:31, blundell wrote:
> It seems like this logic is missing from the new impl?

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:27:
DCHECK(on_result_callback_.is_null());
On 2016/10/21 18:16:31, blundell wrote:
> This object is per-WebContents, the same as the old impl was. So you'll still
> need to handle the case where a request comes in while a request was already
> outstanding. The old impl handled that by simply raising an error on the
> previously-outstanding request before handling the new request, so you can do
> the same.

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:56:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
On 2016/10/21 18:16:31, blundell wrote:
> Why this short-circuit?

This is done to make sure the lock success messages are received in the same
channel as the ViewMsg_Resize. Please let me know if I did something wrong here.

https://codereview.chromium.org/2391883006/diff/340001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2391883006/diff/340001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1589:
screen_orientation_dispatcher_host_.reset(
On 2016/10/21 18:16:31, blundell wrote:
> Is there a reason that you moved where this object was created?

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/2391883006/diff/340001/content/common/BUILD.g...
content/common/BUILD.gn:560:
"//third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom",
On 2016/10/21 18:16:31, blundell wrote:
> Why is this built in //content/common and not in //third_party/WebKit?

Done.

https://codereview.chromium.org/2391883006/diff/340001/content/public/common/...
File content/public/common/screen_orientation.mojom (right):

https://codereview.chromium.org/2391883006/diff/340001/content/public/common/...
content/public/common/screen_orientation.mojom:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/10/21 18:16:31, blundell wrote:
> Is this duplicated with the one in //third_party/WebKit?

Done.

[commit-bot: I haz the power, 2016-10-24 17:23:38]

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

[commit-bot: I haz the power, 2016-10-24 17:23:39]

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

[blundell, 2016-10-25 16:09:27]

Hi Luna,

This is looking really good! My bigest question is still about the
short-circuiting out early in NotifyLockResult().

https://codereview.chromium.org/2391883006/diff/400001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/BUILD....
content/browser/BUILD.gn:1150: "screen_orientation/screen_orientation_impl.cc",
nit: this can just be screen_orientation (and class named ScreenOrientation).

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:17: :
web_contents_(web_contents),
I think you need to call the WCO constructor here and pass it |web_contents|,
right?

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:28: // When a
request comes in while another request was already outstanding.
nit: I don't think that we need this comment.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:30:
NotifyLockResult(ScreenOrientationLockResult::
is this formatting thanks to "git cl format"?

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:44:
weak_factory_.GetWeakPtr()));
This object owns |provider_|, correct? If so there's no need to pass a WeakPtr
here.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:70:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
This still seems strange to me. Can you go through the explanation again?

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:19: class
ScreenOrientation
ah you already renamed the class, great! so it's just the filename.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:33: //
WebContentsObserver:
nit: blank line before

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:39: WebContents*
web_contents_;
nit: if it's a WCO it can just use web_contents() instead.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:42:
WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
is this called binding_ or bindings_ in other classes?

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (left):

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:96: pending_lock_.reset();
Should you reset pending_lock_orientation_ to Default here?

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:32:
NotifyLockResult(ScreenOrientationLockResult::
There shouldn't be any pending request at this point because ScreenOrientation
will have already cancelled it if so?

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1192: if (screen_orientation_dispatcher_)
{
why this added code?

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
content/renderer/render_frame_impl.h:31: #include
"content/common/screen_orientation_interface.mojom.h"
nit: why these added includes?

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(left):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:35: delete
this;
i guess this is why you had to add the code in RenderFrameImpl?

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:79: void
ScreenOrientationDispatcher::setScreenOrientationInterface() {
nit: this should be Set... (and similar for Get... below).

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(left):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:229:
OnMessageReceived(ScreenOrientationMsg_LockSuccess(routing_id(),
I think that this test isn't testing anything meaningful over
CancelPending_DoubleLock without this line. Is there a way that you can obtain
request_id1 as was previously done in line 222? If so, I suggest that you call
OnLockOrientationResult(request_id1, SUCCESS) directly to preserve the spirit of
the test. If not, the test should be removed.

https://codereview.chromium.org/2391883006/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationEnumTraits.h
(right):

https://codereview.chromium.org/2391883006/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationEnumTraits.h:14:
struct EnumTraits<::blink::mojom::ScreenOrientationLockType,
Why not do this for ScreenOrientationLockResult as well? That should save you
having to convert in screen_orientation_dispatcher.cc, right?

[chromium-reviews, 2016-10-25 23:40:47]

That I will have to ask @rockot if I did it correctly.


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Tue, Oct 25, 2016 at 12:09 PM, <blundell@chromium.org> wrote:

> Hi Luna,
>
> This is looking really good! My bigest question is still about the
> short-circuiting out early in NotifyLockResult().
>
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/BUILD.gn
> File content/browser/BUILD.gn (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/BUILD.gn#newcode1150
> content/browser/BUILD.gn:1150:
> "screen_orientation/screen_orientation_impl.cc",
> nit: this can just be screen_orientation (and class named
> ScreenOrientation).
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc
> File content/browser/screen_orientation/screen_orientation_impl.cc
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode17
> content/browser/screen_orientation/screen_orientation_impl.cc:17: :
> web_contents_(web_contents),
> I think you need to call the WCO constructor here and pass it
> |web_contents|, right?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode28
> content/browser/screen_orientation/screen_orientation_impl.cc:28: //
> When a request comes in while another request was already outstanding.
> nit: I don't think that we need this comment.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode30
> content/browser/screen_orientation/screen_orientation_impl.cc:30:
> NotifyLockResult(ScreenOrientationLockResult::
> is this formatting thanks to "git cl format"?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode44
> content/browser/screen_orientation/screen_orientation_impl.cc:44:
> weak_factory_.GetWeakPtr()));
> This object owns |provider_|, correct? If so there's no need to pass a
> WeakPtr here.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode70
> content/browser/screen_orientation/screen_orientation_impl.cc:70:
> binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
> This still seems strange to me. Can you go through the explanation
> again?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h
> File content/browser/screen_orientation/screen_orientation_impl.h
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode19
> content/browser/screen_orientation/screen_orientation_impl.h:19: class
> ScreenOrientation
> ah you already renamed the class, great! so it's just the filename.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode33
> content/browser/screen_orientation/screen_orientation_impl.h:33: //
> WebContentsObserver:
> nit: blank line before
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode39
> content/browser/screen_orientation/screen_orientation_impl.h:39:
> WebContents* web_contents_;
> nit: if it's a WCO it can just use web_contents() instead.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode42
> content/browser/screen_orientation/screen_orientation_impl.h:42:
> WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
> is this called binding_ or bindings_ in other classes?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc
> File content/public/browser/screen_orientation_provider.cc (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc#oldcode96
> content/public/browser/screen_orientation_provider.cc:96:
> pending_lock_.reset();
> Should you reset pending_lock_orientation_ to Default here?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc
> File content/public/browser/screen_orientation_provider.cc (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc#newcode32
> content/public/browser/screen_orientation_provider.cc:32:
> NotifyLockResult(ScreenOrientationLockResult::
> There shouldn't be any pending request at this point because
> ScreenOrientation will have already cancelled it if so?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.cc
> File content/renderer/render_frame_impl.cc (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.cc#newcode1192
> content/renderer/render_frame_impl.cc:1192: if
> (screen_orientation_dispatcher_) {
> why this added code?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.h
> File content/renderer/render_frame_impl.h (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.h#newcode31
> content/renderer/render_frame_impl.h:31: #include
> "content/common/screen_orientation_interface.mojom.h"
> nit: why these added includes?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc#
> oldcode35
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc:35:
> delete this;
> i guess this is why you had to add the code in RenderFrameImpl?
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc#
> newcode79
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc:79:
> void ScreenOrientationDispatcher::setScreenOrientationInterface() {
> nit: this should be Set... (and similar for Get... below).
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc
> (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc#oldcode229
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc:229:
> OnMessageReceived(ScreenOrientationMsg_LockSuccess(routing_id(),
> I think that this test isn't testing anything meaningful over
> CancelPending_DoubleLock without this line. Is there a way that you can
> obtain request_id1 as was previously done in line 222? If so, I suggest
> that you call OnLockOrientationResult(request_id1, SUCCESS) directly to
> preserve the spirit of the test. If not, the test should be removed.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h
> File
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h#newcode14
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h:14:
> struct EnumTraits<::blink::mojom::ScreenOrientationLockType,
> Why not do this for ScreenOrientationLockResult as well? That should
> save you having to convert in screen_orientation_dispatcher.cc, right?
>
> https://codereview.chromium.org/2391883006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-10-26 21:59:54]

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:70:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
On 2016/10/25 at 16:09:26, blundell wrote:
> This still seems strange to me. Can you go through the explanation again?

This is correct. It's how WebContentsFrameBindingSet plumbs RenderFrameHost
context that used to be plumbed via WebContentsObserver::OnMessageReceived.

[Ken Rockot(use gerrit already), 2016-10-26 22:00:45]

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:42:
WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
On 2016/10/25 at 16:09:26, blundell wrote:
> is this called binding_ or bindings_ in other classes?

bindings_ is probably a better name. There's a binding per RenderFrameHost.

[blundell, 2016-10-26 23:07:44]

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:70:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
On 2016/10/26 21:59:54, Ken Rockot wrote:
> On 2016/10/25 at 16:09:26, blundell wrote:
> > This still seems strange to me. Can you go through the explanation again?
> 
> This is correct. It's how WebContentsFrameBindingSet plumbs RenderFrameHost
> context that used to be plumbed via WebContentsObserver::OnMessageReceived.
> 

I still don't think this is correct. This callback is happening in response to
the orientation changing. It's not occurring (in general) during dispatch of an
incoming Mojo message.

The way that the old code worked here is that it saved the (RFH ID, RPH ID) of
the latest incoming lock request from the renderer and it used those to obtain
the correct RFH to send back the response on (cf.
ScreenOrientationDispatcherHostImpl::NotifyLockSuccess(int request_id)). What
this corresponds to in the new world is simply calling |on_result_callback_|,
since |on_result_callback_| is implicitly bound to the most recent renderer-side
caller.

In short, unless I'm missing something, lines 68-70 here should just be omitted.

[lunalu1, 2016-10-27 22:22:26]

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:70:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
On 2016/10/26 21:59:54, Ken Rockot wrote:
> On 2016/10/25 at 16:09:26, blundell wrote:
> > This still seems strange to me. Can you go through the explanation again?
> 
> This is correct. It's how WebContentsFrameBindingSet plumbs RenderFrameHost
> context that used to be plumbed via WebContentsObserver::OnMessageReceived.
> 

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:70:
binding_.GetCurrentTargetFrame() != web_contents_->GetMainFrame())
On 2016/10/26 23:07:43, blundell wrote:
> On 2016/10/26 21:59:54, Ken Rockot wrote:
> > On 2016/10/25 at 16:09:26, blundell wrote:
> > > This still seems strange to me. Can you go through the explanation again?
> > 
> > This is correct. It's how WebContentsFrameBindingSet plumbs RenderFrameHost
> > context that used to be plumbed via WebContentsObserver::OnMessageReceived.
> > 
> 
> I still don't think this is correct. This callback is happening in response to
> the orientation changing. It's not occurring (in general) during dispatch of
an
> incoming Mojo message.
> 
> The way that the old code worked here is that it saved the (RFH ID, RPH ID) of
> the latest incoming lock request from the renderer and it used those to obtain
> the correct RFH to send back the response on (cf.
> ScreenOrientationDispatcherHostImpl::NotifyLockSuccess(int request_id)). What
> this corresponds to in the new world is simply calling |on_result_callback_|,
> since |on_result_callback_| is implicitly bound to the most recent
renderer-side
> caller.
> 
> In short, unless I'm missing something, lines 68-70 here should just be
omitted.

Seems like Colin is right. With this 4 lines of code, the lock success callback
was never successfully returned. Without the 4 lines, the lock/unlock
orientation works as expected. (In the scenario of calling lock within an
iframe).

[lunalu1, 2016-10-28 19:02:46]

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

[commit-bot: I haz the power, 2016-10-28 19:03:22]

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

[lunalu1, 2016-10-28 19:04:13]

I removed the four lines as Colin suggested and re-verified that mojo and IPC
messages are still received in the order expected. 

Please take another look at this CL. 

Thanks

[commit-bot: I haz the power, 2016-10-28 19:15:11]

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

[commit-bot: I haz the power, 2016-10-28 19:15:12]

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

[blundell, 2016-10-28 21:21:32]

Hi Luna,

Thanks! It looks like you still need to address the comments I left on the last
review round.

lgtm with those changes made

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:69: // Reset the
callback.
nit: I don't think this comment is necessary.

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:5: #ifndef
CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_IMPL_H
nit: this file should be called screen_orientation.*

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:41:
WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
nit: |bindings_|

[lunalu1, 2016-10-31 17:44:35]

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

[commit-bot: I haz the power, 2016-10-31 17:45:03]

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

[lunalu1, 2016-10-31 17:48:18]

Patchset #9 (id:440001) has been deleted

[lunalu1, 2016-10-31 17:49:09]

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

[commit-bot: I haz the power, 2016-10-31 17:49:41]

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, 2016-10-31 17:57:37]

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

[commit-bot: I haz the power, 2016-10-31 17:57:39]

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

[lunalu1, 2016-10-31 18:02:52]

lunalu@chromium.org changed reviewers:
+ pfeldman@chromium.org, rsesek@chromium.org

[lunalu1, 2016-10-31 18:02:54]

Thanks for reviewing my CL Colin, I have made the changes according to your
comments.

pfeldman, rsesek, could you please take a look at my CL, I am missing owner
reviews.

Thanks

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:69: // Reset the
callback.
On 2016/10/28 21:21:32, blundell wrote:
> nit: I don't think this comment is necessary.

Done.

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:5: #ifndef
CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_IMPL_H
On 2016/10/28 21:21:32, blundell wrote:
> nit: this file should be called screen_orientation.*

Done.

https://codereview.chromium.org/2391883006/diff/420001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:41:
WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
On 2016/10/28 21:21:32, blundell wrote:
> nit: |bindings_|

Done.

[lunalu1, 2016-10-31 18:03:03]

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

[commit-bot: I haz the power, 2016-10-31 18:03:32]

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

[lunalu1, 2016-10-31 18:05:37]

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

[commit-bot: I haz the power, 2016-10-31 18:05: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, 2016-10-31 18:13:54]

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

[commit-bot: I haz the power, 2016-10-31 18:13:57]

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

[blundell, 2016-10-31 22:16:22]

Hi Luna,

It looks like you still need to address most of my comments from PS7 (for
example, about calling the WebContentsObserver constructor from the
ScreenOrientation constructor).

When adding OWNERS, it's "best practice" to specify what you're asking them to
review, as usually it's not the entire CL. In this CL, I think that mlamouri@
would be the best OWNER review for //content/*/screen_orientation, since he
wrote all of that code. Thanks!

[lunalu1, 2016-11-01 18:51:54]

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

[commit-bot: I haz the power, 2016-11-01 18:52:25]

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, 2016-11-01 20:18:07]

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

[commit-bot: I haz the power, 2016-11-01 20:18:08]

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

[lunalu1, 2016-11-01 21:20:29]

Patchset #10 (id:480001) has been deleted

[lunalu1, 2016-11-01 21:20:37]

Patchset #9 (id:460001) has been deleted

[lunalu1, 2016-11-01 21:21:07]

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

[commit-bot: I haz the power, 2016-11-01 21:21:41]

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

[lunalu1, 2016-11-01 21:22:20]

Patchset #9 (id:500001) has been deleted

[commit-bot: I haz the power, 2016-11-01 23:00:53]

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

[commit-bot: I haz the power, 2016-11-01 23:00:55]

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_...)

[lunalu1, 2016-11-02 04:55:43]

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

[commit-bot: I haz the power, 2016-11-02 04:55:57]

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

[lunalu1, 2016-11-02 04:57:55]

The CQ bit was unchecked by lunalu@chromium.org

[lunalu1, 2016-11-03 23:15:24]

lunalu@chromium.org changed reviewers:
+ mlamouri@chromium.org
- pfeldman@chromium.org, rsesek@chromium.org

[lunalu1, 2016-11-03 23:15:26]

@blundell, thanks for reviewing my code. I have made all the changes based on
your previous comments. Could you please take a second look just in case I did
something wrong. 

mlamouri@, I changed a lot of stuff on the files you wrote. Could you please
take a look at them?

Thanks

https://codereview.chromium.org/2391883006/diff/400001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/BUILD....
content/browser/BUILD.gn:1150: "screen_orientation/screen_orientation_impl.cc",
On 2016/10/25 16:09:26, blundell wrote:
> nit: this can just be screen_orientation (and class named ScreenOrientation). 

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:17: :
web_contents_(web_contents),
On 2016/10/25 16:09:26, blundell wrote:
> I think you need to call the WCO constructor here and pass it |web_contents|,
> right?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:28: // When a
request comes in while another request was already outstanding.
On 2016/10/25 16:09:26, blundell wrote:
> nit: I don't think that we need this comment.

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:30:
NotifyLockResult(ScreenOrientationLockResult::
On 2016/10/25 16:09:26, blundell wrote:
> is this formatting thanks to "git cl format"? 

Unfortunately yes. But I will make it less ugly.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.cc:44:
weak_factory_.GetWeakPtr()));
On 2016/10/25 16:09:26, blundell wrote:
> This object owns |provider_|, correct? If so there's no need to pass a WeakPtr
> here.

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation_impl.h (right):

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:19: class
ScreenOrientation
On 2016/10/25 16:09:26, blundell wrote:
> ah you already renamed the class, great! so it's just the filename.

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:33: //
WebContentsObserver:
On 2016/10/25 16:09:26, blundell wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:39: WebContents*
web_contents_;
On 2016/10/25 16:09:26, blundell wrote:
> nit: if it's a WCO it can just use web_contents() instead.

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/browser/screen...
content/browser/screen_orientation/screen_orientation_impl.h:42:
WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
On 2016/10/25 16:09:26, blundell wrote:
> is this called binding_ or bindings_ in other classes?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (left):

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:96: pending_lock_.reset();
On 2016/10/25 16:09:26, blundell wrote:
> Should you reset pending_lock_orientation_ to Default here?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:32:
NotifyLockResult(ScreenOrientationLockResult::
On 2016/10/25 16:09:26, blundell wrote:
> There shouldn't be any pending request at this point because ScreenOrientation
> will have already cancelled it if so?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1192: if (screen_orientation_dispatcher_)
{
On 2016/10/25 16:09:27, blundell wrote:
> why this added code?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/rende...
content/renderer/render_frame_impl.h:31: #include
"content/common/screen_orientation_interface.mojom.h"
On 2016/10/25 16:09:27, blundell wrote:
> nit: why these added includes?

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(left):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:35: delete
this;
On 2016/10/25 16:09:27, blundell wrote:
> i guess this is why you had to add the code in RenderFrameImpl?

my bad.

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:79: void
ScreenOrientationDispatcher::setScreenOrientationInterface() {
On 2016/10/25 16:09:27, blundell wrote:
> nit: this should be Set... (and similar for Get... below).

Done.

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(left):

https://codereview.chromium.org/2391883006/diff/400001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:229:
OnMessageReceived(ScreenOrientationMsg_LockSuccess(routing_id(),
On 2016/10/25 16:09:27, blundell wrote:
> I think that this test isn't testing anything meaningful over
> CancelPending_DoubleLock without this line. Is there a way that you can obtain
> request_id1 as was previously done in line 222? If so, I suggest that you call
> OnLockOrientationResult(request_id1, SUCCESS) directly to preserve the spirit
of
> the test. If not, the test should be removed.

Done.

https://codereview.chromium.org/2391883006/diff/400001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationEnumTraits.h
(right):

https://codereview.chromium.org/2391883006/diff/400001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationEnumTraits.h:14:
struct EnumTraits<::blink::mojom::ScreenOrientationLockType,
On 2016/10/25 16:09:27, blundell wrote:
> Why not do this for ScreenOrientationLockResult as well? That should save you
> having to convert in screen_orientation_dispatcher.cc, right?

This is because before OnLockSuccess and OnLockError were handled separately and
there wasn't a enum type that had both LockSuccess and LockError's so we will
just leave it the way it is for now.

[chromium-reviews, 2016-11-03 23:18:42]

Sorry forgot to mention,
the
org.chromium.chrome.browser.webapps.ManifestUpgradeDetectorTest#testCanonicalUrlsIdenticalShouldNotUpgrade
keeps failing (I tried to run it a few times). I can't think of a reason
how my chance is causing the test to fail. But I do notice that a lot of
other CL's also are failing the same test.

Could anyone please help me figure out if there's something I changed that
made the test to fail or the test is just being flaky.

Thanks


Luna Lu | Software Engineer | lunalu@google.com | +1 519 513 5751

On Thu, Nov 3, 2016 at 4:15 PM, <lunalu@chromium.org> wrote:

> @blundell, thanks for reviewing my code. I have made all the changes based
> on
> your previous comments. Could you please take a second look just in case I
> did
> something wrong.
>
> mlamouri@, I changed a lot of stuff on the files you wrote. Could you
> please
> take a look at them?
>
> Thanks
>
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/BUILD.gn
> File content/browser/BUILD.gn (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/BUILD.gn#newcode1150
> content/browser/BUILD.gn:1150:
> "screen_orientation/screen_orientation_impl.cc",
> On 2016/10/25 16:09:26, blundell wrote:
> > nit: this can just be screen_orientation (and class named
> ScreenOrientation).
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc
> File content/browser/screen_orientation/screen_orientation_impl.cc
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode17
> content/browser/screen_orientation/screen_orientation_impl.cc:17: :
> web_contents_(web_contents),
> On 2016/10/25 16:09:26, blundell wrote:
> > I think you need to call the WCO constructor here and pass it
> |web_contents|,
> > right?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode28
> content/browser/screen_orientation/screen_orientation_impl.cc:28: //
> When a request comes in while another request was already outstanding.
> On 2016/10/25 16:09:26, blundell wrote:
> > nit: I don't think that we need this comment.
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode30
> content/browser/screen_orientation/screen_orientation_impl.cc:30:
> NotifyLockResult(ScreenOrientationLockResult::
> On 2016/10/25 16:09:26, blundell wrote:
> > is this formatting thanks to "git cl format"?
>
> Unfortunately yes. But I will make it less ugly.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.cc#newcode44
> content/browser/screen_orientation/screen_orientation_impl.cc:44:
> weak_factory_.GetWeakPtr()));
> On 2016/10/25 16:09:26, blundell wrote:
> > This object owns |provider_|, correct? If so there's no need to pass a
> WeakPtr
> > here.
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h
> File content/browser/screen_orientation/screen_orientation_impl.h
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode19
> content/browser/screen_orientation/screen_orientation_impl.h:19: class
> ScreenOrientation
> On 2016/10/25 16:09:26, blundell wrote:
> > ah you already renamed the class, great! so it's just the filename.
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode33
> content/browser/screen_orientation/screen_orientation_impl.h:33: //
> WebContentsObserver:
> On 2016/10/25 16:09:26, blundell wrote:
> > nit: blank line before
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode39
> content/browser/screen_orientation/screen_orientation_impl.h:39:
> WebContents* web_contents_;
> On 2016/10/25 16:09:26, blundell wrote:
> > nit: if it's a WCO it can just use web_contents() instead.
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/browser/screen_orientation/screen_orientation_impl.h#newcode42
> content/browser/screen_orientation/screen_orientation_impl.h:42:
> WebContentsFrameBindingSet<mojom::ScreenOrientation> binding_;
> On 2016/10/25 16:09:26, blundell wrote:
> > is this called binding_ or bindings_ in other classes?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc
> File content/public/browser/screen_orientation_provider.cc (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc#oldcode96
> content/public/browser/screen_orientation_provider.cc:96:
> pending_lock_.reset();
> On 2016/10/25 16:09:26, blundell wrote:
> > Should you reset pending_lock_orientation_ to Default here?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc
> File content/public/browser/screen_orientation_provider.cc (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/public/browser/screen_orientation_provider.cc#newcode32
> content/public/browser/screen_orientation_provider.cc:32:
> NotifyLockResult(ScreenOrientationLockResult::
> On 2016/10/25 16:09:26, blundell wrote:
> > There shouldn't be any pending request at this point because
> ScreenOrientation
> > will have already cancelled it if so?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.cc
> File content/renderer/render_frame_impl.cc (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.cc#newcode1192
> content/renderer/render_frame_impl.cc:1192: if
> (screen_orientation_dispatcher_) {
> On 2016/10/25 16:09:27, blundell wrote:
> > why this added code?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.h
> File content/renderer/render_frame_impl.h (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/render_frame_impl.h#newcode31
> content/renderer/render_frame_impl.h:31: #include
> "content/common/screen_orientation_interface.mojom.h"
> On 2016/10/25 16:09:27, blundell wrote:
> > nit: why these added includes?
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc#
> oldcode35
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc:35:
> delete this;
> On 2016/10/25 16:09:27, blundell wrote:
> > i guess this is why you had to add the code in RenderFrameImpl?
>
> my bad.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc#
> newcode79
> content/renderer/screen_orientation/screen_orientation_dispatcher.cc:79:
> void ScreenOrientationDispatcher::setScreenOrientationInterface() {
> On 2016/10/25 16:09:27, blundell wrote:
> > nit: this should be Set... (and similar for Get... below).
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc
> File
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc
> (left):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc#oldcode229
> content/renderer/screen_orientation/screen_orientation_dispatcher_
> browsertest.cc:229:
> OnMessageReceived(ScreenOrientationMsg_LockSuccess(routing_id(),
> On 2016/10/25 16:09:27, blundell wrote:
> > I think that this test isn't testing anything meaningful over
> > CancelPending_DoubleLock without this line. Is there a way that you
> can obtain
> > request_id1 as was previously done in line 222? If so, I suggest that
> you call
> > OnLockOrientationResult(request_id1, SUCCESS) directly to preserve the
> spirit of
> > the test. If not, the test should be removed.
>
> Done.
>
> https://codereview.chromium.org/2391883006/diff/400001/
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h
> File
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h
> (right):
>
> https://codereview.chromium.org/2391883006/diff/400001/
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h#newcode14
> third_party/WebKit/public/platform/modules/screen_orientation/
> WebScreenOrientationEnumTraits.h:14:
> struct EnumTraits<::blink::mojom::ScreenOrientationLockType,
> On 2016/10/25 16:09:27, blundell wrote:
> > Why not do this for ScreenOrientationLockResult as well? That should
> save you
> > having to convert in screen_orientation_dispatcher.cc, right?
>
> This is because before OnLockSuccess and OnLockError were handled
> separately and there wasn't a enum type that had both LockSuccess and
> LockError's so we will just leave it the way it is for now.
>
> https://codereview.chromium.org/2391883006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2016-11-04 17:44:34]

Thanks iclelland@ for reaching out to me about the confusing
ManifestUpgradeDetectorTest_testCanonicalUrlsIdenticalShouldNotUpgrade test
failure. Thankfully debugging the test failure was not too hard because the test
failure repros locally.

The test is failing due to the change in
content/common/manifest_manager_messages.h With this CL applied, the
content::Manifest fetched via WebContentsImpl::GetManifest() always uses the
default orientation.

[pkotwicz, 2016-11-04 17:44:41]

Thanks iclelland@ for reaching out to me about the confusing
ManifestUpgradeDetectorTest_testCanonicalUrlsIdenticalShouldNotUpgrade test
failure. Thankfully debugging the test failure was not too hard because the test
failure repros locally.

The test is failing due to the change in
content/common/manifest_manager_messages.h With this CL applied, the
content::Manifest fetched via WebContentsImpl::GetManifest() always uses the
default orientation.

[blundell, 2016-11-07 13:10:23]

On 2016/11/04 17:44:41, pkotwicz wrote:
> Thanks iclelland@ for reaching out to me about the confusing
> ManifestUpgradeDetectorTest_testCanonicalUrlsIdenticalShouldNotUpgrade test
> failure. Thankfully debugging the test failure was not too hard because the
test
> failure repros locally.
> 
> The test is failing due to the change in
> content/common/manifest_manager_messages.h With this CL applied, the
> content::Manifest fetched via WebContentsImpl::GetManifest() always uses the
> default orientation.

Thanks for the debugging, pkotwicz@! Luna, it looks to me like the change to
manifest_manager_messages.h just shouldn't be in this CL. Was there a problem
that caused it to be included?

Changes look good otherwise.

[blundell, 2016-11-07 13:10:54]

https://codereview.chromium.org/2391883006/diff/520001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.cc (right):

https://codereview.chromium.org/2391883006/diff/520001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:19: weak_factory_(this)
{
nit: I don't think you need this anymore.

https://codereview.chromium.org/2391883006/diff/520001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/520001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:31: // ScreenOrientation
should have cancelled all pending request at thie point.
nit: "all prior pending requests at this point"

[blundell, 2016-11-07 13:11:54]

mlamouri@: To be explicit, could you review all of the changes to screen
orientation directories here (and as much of the rest of the CL as you want/need
to)? Thanks!

[lunalu1, 2016-11-07 16:05:00]

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

[commit-bot: I haz the power, 2016-11-07 16:05:20]

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, 2016-11-07 19:37:54]

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

[commit-bot: I haz the power, 2016-11-07 19:37:55]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2016-11-07 21:02:45]

lunalu@chromium.org changed reviewers:
+ pfeldman@chromium.org, tsepez@chromium.org

[lunalu1, 2016-11-07 21:02:47]

Hi tsepez@, could you please review my changes in 
  content/common/screen_orientation_interface.mojom
  content/common/screen_orientation_messages.h
 
third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom

and pfeldman@, could you please review everything else.

Thanks!

[Tom Sepez, 2016-11-07 21:05:55]

mojom/messages LGTM.

[mlamouri (slow - plz ping), 2016-11-12 23:39:40]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be paased from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be passed from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mlamouri (slow - plz ping), 2016-11-13 00:26:36]

First pass. It's a huge CL with a lot of changes so it might require a couple of
iterations. Hope it is okay.

https://codereview.chromium.org/2391883006/diff/540001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:36: #include
"content/browser/screen_orientation/screen_orientation.h"
Why?

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:69: on_result_callback_
= LockOrientationCallback();
`base::ResetAndReturn(&on_result_callback_).Run(result);` maybe?

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.h (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.h:22:
ScreenOrientation(WebContents* web_contents);
explicit

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.h:29: void
LockOrientation(::blink::WebScreenOrientationLockType orientation,
why :: in front of Blink?

https://codereview.chromium.org/2391883006/diff/540001/content/common/screen_...
File content/common/screen_orientation_interface.mojom (right):

https://codereview.chromium.org/2391883006/diff/540001/content/common/screen_...
content/common/screen_orientation_interface.mojom:9: interface ScreenOrientation
{
Can we call this screen_orientation.mojom? Would be clearer IMO.

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (left):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:77: pending_lock_.reset();
Are you handling this?

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:109:
on_result_callback_.Run(result);
`base::ResetAndReturn(&on_result_callback_).Run(result);` maybe?

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.h:76:
blink::WebScreenOrientationLockType pending_lock_orientation_;
I would feel slightly more comfortable having
base::Optional<blink::WebScreenOrientationLockType> because it would be
semantically clearer instead of using "Default" but both would be correct so
take this as a suggestion ;)

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:72:
SetScreenOrientationInterface();
Maybe "EnsureScreenOrientationService" would be a better name?

Alternatively:
```
GetScreenOrientationService()->LockOrientation(orientation, base::bind(...));
```

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:86: if
(!screen_orientation_)
style: add { }

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.h
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:38:
ScreenOrientationAssociatedPtr& screen_orientation_for_tests) {
What do you think of moving this to private and make the test a friend?

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:55:
class ScreenOrientationDispatcherTest : public RenderViewTest {
Why do you need this to be a RenderViewTest?

https://codereview.chromium.org/2391883006/diff/540001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom
(right):

https://codereview.chromium.org/2391883006/diff/540001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
FWIW, I would call this screen_orientation_lock_types.mojom

[lunalu1, 2016-11-15 16:03:13]

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

[commit-bot: I haz the power, 2016-11-15 16:03:41]

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, 2016-11-15 16:06:11]

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

[commit-bot: I haz the power, 2016-11-15 16:06:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[lunalu1, 2016-11-15 17:02:27]

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

[commit-bot: I haz the power, 2016-11-15 17:03:13]

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, 2016-11-15 17:06:56]

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

[commit-bot: I haz the power, 2016-11-15 17:06:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2016-11-15 18:10:04]

Patchset #12 (id:580001) has been deleted

[lunalu1, 2016-11-15 19:22:34]

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

[lunalu1, 2016-11-15 19:22:49]

Patchset #11 (id:560001) has been deleted

[commit-bot: I haz the power, 2016-11-15 19:23:20]

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

[Tom Sepez, 2016-11-15 19:27:39]

mojom/messages LGTM.

[commit-bot: I haz the power, 2016-11-15 19:36:47]

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

[commit-bot: I haz the power, 2016-11-15 19:36:48]

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

[lunalu1, 2016-11-16 16:15:39]

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

[commit-bot: I haz the power, 2016-11-16 16:16:20]

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, 2016-11-16 17:33:21]

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

[commit-bot: I haz the power, 2016-11-16 17:33:23]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lunalu1, 2016-11-16 18:46:59]

Hi mlamouri@, 

thanks for reviewing my CL. I have made the changes you suggested. Please take
another look.

Thanks

https://codereview.chromium.org/2391883006/diff/540001/content/browser/androi...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/androi...
content/browser/android/content_view_core_impl.cc:36: #include
"content/browser/screen_orientation/screen_orientation.h"
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> Why?

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:69: on_result_callback_
= LockOrientationCallback();
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> `base::ResetAndReturn(&on_result_callback_).Run(result);` maybe?

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.h (right):

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.h:22:
ScreenOrientation(WebContents* web_contents);
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> explicit

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.h:29: void
LockOrientation(::blink::WebScreenOrientationLockType orientation,
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> why :: in front of Blink?
Done.

https://codereview.chromium.org/2391883006/diff/540001/content/common/screen_...
File content/common/screen_orientation_interface.mojom (right):

https://codereview.chromium.org/2391883006/diff/540001/content/common/screen_...
content/common/screen_orientation_interface.mojom:9: interface ScreenOrientation
{
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> Can we call this screen_orientation.mojom? Would be clearer IMO.

Yes, and renamed the other file screen_orientation_lock_types as suggested.

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (left):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:77: pending_lock_.reset();
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> Are you handling this?

Yes. this is handled in the lock orientation mojo interface.

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:109:
on_result_callback_.Run(result);
On 2016/11/13 00:26:35, mlamouri (slow) wrote:
> `base::ResetAndReturn(&on_result_callback_).Run(result);` maybe?

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/2391883006/diff/540001/content/public/browser...
content/public/browser/screen_orientation_provider.h:76:
blink::WebScreenOrientationLockType pending_lock_orientation_;
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> I would feel slightly more comfortable having
> base::Optional<blink::WebScreenOrientationLockType> because it would be
> semantically clearer instead of using "Default" but both would be correct so
> take this as a suggestion ;)

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.cc
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:72:
SetScreenOrientationInterface();
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> Maybe "EnsureScreenOrientationService" would be a better name?
> 
> Alternatively:
> ```
> GetScreenOrientationService()->LockOrientation(orientation, base::bind(...));
> ```

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.cc:86: if
(!screen_orientation_)
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> style: add { }

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File content/renderer/screen_orientation/screen_orientation_dispatcher.h
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher.h:38:
ScreenOrientationAssociatedPtr& screen_orientation_for_tests) {
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> What do you think of moving this to private and make the test a friend?

Done.

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(right):

https://codereview.chromium.org/2391883006/diff/540001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:55:
class ScreenOrientationDispatcherTest : public RenderViewTest {
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> Why do you need this to be a RenderViewTest?

Because we need to set up dummy proxy to test calling mojo interface now.

https://codereview.chromium.org/2391883006/diff/540001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom
(right):

https://codereview.chromium.org/2391883006/diff/540001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/screen_orientation/screen_orientation.mojom:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/11/13 00:26:36, mlamouri (slow) wrote:
> FWIW, I would call this screen_orientation_lock_types.mojom

Done.

[lunalu1, 2016-11-16 20:54:07]

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

[commit-bot: I haz the power, 2016-11-16 20:54:47]

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, 2016-11-16 22:37:58]

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

[commit-bot: I haz the power, 2016-11-16 22:38:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mlamouri (slow - plz ping), 2016-11-18 14:51:20]

Still have a few comments but I will be OOO for a week so lgtm in order for you
to land but please address the comments :)

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:34:
DCHECK(pending_lock_orientation_.has_value());
Shouldn't be negative?

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:95: if
(pending_lock_orientation_.has_value())
Shouldn't this be `if (!pending_lock_orientation_.has_value())`

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
File content/public/browser/screen_orientation_provider.h (right):

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
content/public/browser/screen_orientation_provider.h:29:
ScreenOrientationProvider(WebContents* web_contents);
explicit

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/rende...
content/renderer/render_frame_impl.h:42: #include
"content/renderer/screen_orientation/screen_orientation_dispatcher.h"
Why is this needed?

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(left):

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:182:
TEST_F(ScreenOrientationDispatcherTest, SuccessForUnknownRequest) {
Why was this test removed?

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:197:
TEST_F(ScreenOrientationDispatcherTest, ErrorForUnknownRequest) {
... and this one?

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(right):

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:55:
class ScreenOrientationDispatcherTest : public RenderViewTest {
It would be great if in the future we don't need to have a RenderViewTest when
we use a mojo service. It seems to be unnecessary overhead.

https://codereview.chromium.org/2391883006/diff/600001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/2391883006/diff/600001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:10: #include
"../platform/modules/permissions/WebPermissionType.h"
Why is this change required?

[lunalu1, 2016-11-21 18:31:31]

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

[commit-bot: I haz the power, 2016-11-21 18:32:15]

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

[lunalu1, 2016-11-21 20:23:52]

Hi, 
thanks for reviewing my CL. I have made the changes based on the comments.
Please take another look.

Thanks

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:34:
DCHECK(pending_lock_orientation_.has_value());
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> Shouldn't be negative?

Done.

https://codereview.chromium.org/2391883006/diff/600001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:95: if
(pending_lock_orientation_.has_value())
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> Shouldn't this be `if (!pending_lock_orientation_.has_value())`

Done.

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/rende...
content/renderer/render_frame_impl.h:42: #include
"content/renderer/screen_orientation/screen_orientation_dispatcher.h"
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> Why is this needed?

Done.

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
File
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc
(left):

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:182:
TEST_F(ScreenOrientationDispatcherTest, SuccessForUnknownRequest) {
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> Why was this test removed?
Because the edge case no longer exits: request_id is no longer used to look up
pending lock requests.

https://codereview.chromium.org/2391883006/diff/600001/content/renderer/scree...
content/renderer/screen_orientation/screen_orientation_dispatcher_browsertest.cc:197:
TEST_F(ScreenOrientationDispatcherTest, ErrorForUnknownRequest) {
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> ... and this one?

Done.

https://codereview.chromium.org/2391883006/diff/600001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameOwnerProperties.h (right):

https://codereview.chromium.org/2391883006/diff/600001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameOwnerProperties.h:10: #include
"../platform/modules/permissions/WebPermissionType.h"
On 2016/11/18 14:51:20, mlamouri (OOO until 29-11) wrote:
> Why is this change required?

Because the compiler complained. I don't remember exactly when, but cause this
is now referenced else where and the compiler doesn't know where to find the
references any more.

[commit-bot: I haz the power, 2016-11-21 20:38:11]

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

[commit-bot: I haz the power, 2016-11-21 20:38:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pfeldman, 2016-11-21 21:30:26]

lgtm % comments

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.cc (right):

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:35: if (!provider_) {
This is never the case.

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:50: if (provider_)
Always true

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:57: if (!provider_ ||
details.is_in_page)
useless check.

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.h (right):

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.h:42:
base::WeakPtrFactory<ScreenOrientation> weak_factory_;
Unused.

https://codereview.chromium.org/2391883006/diff/620001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/620001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:107: if
(on_result_callback_.is_null())
Reset this callback field before firing it?

[lunalu1, 2016-11-21 21:40:51]

Done. Thanks

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
File content/browser/screen_orientation/screen_orientation.cc (right):

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:35: if (!provider_) {
On 2016/11/21 21:30:25, pfeldman wrote:
> This is never the case.

Done.

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:50: if (provider_)
On 2016/11/21 21:30:26, pfeldman wrote:
> Always true

Done.

https://codereview.chromium.org/2391883006/diff/620001/content/browser/screen...
content/browser/screen_orientation/screen_orientation.cc:57: if (!provider_ ||
details.is_in_page)
On 2016/11/21 21:30:25, pfeldman wrote:
> useless check.

Done.

https://codereview.chromium.org/2391883006/diff/620001/content/public/browser...
File content/public/browser/screen_orientation_provider.cc (right):

https://codereview.chromium.org/2391883006/diff/620001/content/public/browser...
content/public/browser/screen_orientation_provider.cc:107: if
(on_result_callback_.is_null())
On 2016/11/21 21:30:26, pfeldman wrote:
> Reset this callback field before firing it?

Done.

[lunalu1, 2016-11-21 21:41:32]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2016-11-21 21:41:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, tsepez@chromium.org, mlamouri@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2391883006/#ps640001
(title: "clup")

[commit-bot: I haz the power, 2016-11-21 21:42:05]

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, 2016-11-22 00:27:27]

CQ is committing da patch.

Bot data: {"patchset_id": 640001, "attempt_start_ts": 1479764492319020,
"parent_rev": "9a56e86cf80eb4b65efb9f10a2bc0d4d4d336b13", "commit_rev":
"5b81f275c4eaadac2c30d4ba568ead501c77995e"}

[commit-bot: I haz the power, 2016-11-22 00:27:55]

Committed patchset #13 (id:640001)

[commit-bot: I haz the power, 2016-11-22 00:30:46]

Description was changed from

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be passed from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Mojo-ify implementation of screen orientation locking/unlocking.

This CL creates a Mojo service to replace the //content-based IPC currently used
for locking/unlocking screen orientation. Major changes:
- Renderer and browser communicate via a Mojo interface defined in
  screen_orientation_service.mojom.
- The browser-side ScreenOrientationImpl implements a Mojo interface that sends
  request to ScreenOrientationProvider in replacement of
  ScreenOrientationDispatcherHostImpl.
- Request ID no longer have to be passed from the renderer to the browser, as
  the renderer binds the request ID associated with a request in the response
  callback that it creates for the request.
- To ensure the ordering between ViewMsg_Resize and the screen orientation
  messages, WebContentsFrameBindingSet<T> is used on the browser side and
  GetRemoteAssociatedInterface (and mojom::ScreenOrientationAssociatedPtr) is
  is used on the renderer side to make the mojo interface share the same
  underlying pipe as the legacy IPC Channel. As a result, the unit tests for
  ScreenOrientationDispatcher is moved under content_browsertests as it requires
  a dummy remote associated interface for the tests to be run.

BUG=612339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d4a2cc818e19f277ea030e98cd86c477740ed1a6
Cr-Commit-Position: refs/heads/master@{#433714}
==========

[commit-bot: I haz the power, 2016-11-22 00:30:49]

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