Chromium Code Reviews
Help | Chromium Project | Sign in
(50)

Issue 2688383002: [ScreenOrientation] Merge mojo interface ScreenOrientationListener into ScreenOrientation

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 6 days ago by leonhsl
Modified:
1 week ago
CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, einbinder+watch-test-runner_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-test-runner_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-screen-orientation_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[ScreenOrientation] Merge mojo interface ScreenOrientationListener into ScreenOrientation This refactor's goal is to consolidate device.mojom.ScreenOrientationListener together with device.mojom.ScreenOrientation, so that all the mojo calls for screen orientation could run on single mojo connection. Before, both of these two interfaces are associated with the legacy IPC channel to keep ordering constraints between them, but after this CL because all of the mojo calls reside in one interface, they just keep ordering naturally. Thus, once we tackled ordering constraints between lock/unlock and legacy IPC ViewMsg_Resize later, we could de-associate device.mojom.ScreenOrientation interface from the legacy IPC channel entirely. A follow-up CL would do this. BUG=678545

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address comments from mlamouri@ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -308 lines) Patch
M components/test_runner/mock_screen_orientation_client.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 2 chunks +0 lines, -2 lines 0 comments Download
D content/browser/screen_orientation/screen_orientation_listener_android.h View 1 chunk +0 lines, -40 lines 0 comments Download
D content/browser/screen_orientation/screen_orientation_listener_android.cc View 1 chunk +0 lines, -46 lines 0 comments Download
M content/browser/screen_orientation/screen_orientation_provider.h View 1 2 chunks +6 lines, -0 lines 0 comments Download
M content/browser/screen_orientation/screen_orientation_provider.cc View 1 3 chunks +27 lines, -1 line 0 comments Download
M content/renderer/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M content/renderer/renderer_blink_platform_impl.cc View 2 chunks +0 lines, -3 lines 0 comments Download
M content/renderer/screen_orientation/screen_orientation_dispatcher.h View 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/screen_orientation/screen_orientation_dispatcher.cc View 2 chunks +14 lines, -5 lines 0 comments Download
D content/renderer/screen_orientation/screen_orientation_observer.h View 1 chunk +0 lines, -38 lines 0 comments Download
D content/renderer/screen_orientation/screen_orientation_observer.cc View 1 chunk +0 lines, -42 lines 0 comments Download
M device/screen_orientation/public/interfaces/screen_orientation.mojom View 1 chunk +9 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/modules/screen_orientation/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.h View 1 4 chunks +6 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationControllerImpl.cpp View 5 chunks +22 lines, -27 lines 0 comments Download
D third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.h View 1 chunk +0 lines, -41 lines 0 comments Download
D third_party/WebKit/Source/modules/screen_orientation/ScreenOrientationDispatcher.cpp View 1 chunk +0 lines, -31 lines 0 comments Download
M third_party/WebKit/public/platform/WebPlatformEventType.h View 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/public/platform/modules/screen_orientation/WebScreenOrientationClient.h View 1 chunk +4 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 34 (23 generated)
leonhsl
Hi, mlamouri@, rockot@, would you please help to take an overall review? Thanks.
1 week, 6 days ago (2017-02-12 13:43:32 UTC) #11
Ken Rockot
mojom changes and relevant usage all LGTM
1 week, 5 days ago (2017-02-13 20:06:03 UTC) #12
leonhsl
Friendly ping mlamouri@ :)
1 week, 3 days ago (2017-02-15 05:43:44 UTC) #13
haraken
WebKit LGTM
1 week, 3 days ago (2017-02-15 07:02:49 UTC) #14
leonhsl
On 2017/02/15 07:02:49, haraken wrote: > WebKit LGTM Hara-san, Thanks!
1 week, 3 days ago (2017-02-15 07:17:01 UTC) #15
mlamouri
The code itself looks fine but I would like to know more about the long ...
1 week, 3 days ago (2017-02-15 10:58:41 UTC) #17
leonhsl
On 2017/02/15 10:58:41, mlamouri wrote: > The code itself looks fine but I would like ...
1 week, 3 days ago (2017-02-15 13:41:24 UTC) #18
leonhsl
https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_orientation/screen_orientation_provider.cc File content/browser/screen_orientation/screen_orientation_provider.cc (right): https://codereview.chromium.org/2688383002/diff/20001/content/browser/screen_orientation/screen_orientation_provider.cc#newcode35 content/browser/screen_orientation/screen_orientation_provider.cc:35: } On 2017/02/15 10:58:41, mlamouri wrote: > style: no ...
1 week, 2 days ago (2017-02-16 05:00:05 UTC) #21
mlamouri
Leon, could you specify exactly which questions you want an answer to? I was hoping ...
1 week, 1 day ago (2017-02-17 12:04:06 UTC) #32
leonhsl
On 2017/02/17 12:04:06, mlamouri wrote: > Leon, could you specify exactly which questions you want ...
1 week, 1 day ago (2017-02-17 13:27:44 UTC) #33
leonhsl
1 week ago (2017-02-18 16:45:05 UTC) #34
On 2017/02/17 13:27:44, leonhsl wrote:
> On 2017/02/17 12:04:06, mlamouri wrote:
> > Leon, could you specify exactly which questions you want an answer to?
> > 
> I was asking about the second bullet at
>
https://docs.google.com/document/d/1u9S6AIxL3W2zo3IhqMrsDyrfJt-nbT_z3J-prLvJb...,
> the text 'One question' in red.

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

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd