Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by leonhsl(Using Gerrit)
Modified:
8 months 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(Using Gerrit)
Hi, mlamouri@, rockot@, would you please help to take an overall review? Thanks.
8 months, 1 week ago (2017-02-12 13:43:32 UTC) #11
Ken Rockot(use gerrit already)
mojom changes and relevant usage all LGTM
8 months, 1 week ago (2017-02-13 20:06:03 UTC) #12
leonhsl(Using Gerrit)
Friendly ping mlamouri@ :)
8 months ago (2017-02-15 05:43:44 UTC) #13
haraken
WebKit LGTM
8 months ago (2017-02-15 07:02:49 UTC) #14
leonhsl(Using Gerrit)
On 2017/02/15 07:02:49, haraken wrote: > WebKit LGTM Hara-san, Thanks!
8 months ago (2017-02-15 07:17:01 UTC) #15
mlamouri (slow - plz ping)
The code itself looks fine but I would like to know more about the long ...
8 months ago (2017-02-15 10:58:41 UTC) #17
leonhsl(Using Gerrit)
On 2017/02/15 10:58:41, mlamouri wrote: > The code itself looks fine but I would like ...
8 months ago (2017-02-15 13:41:24 UTC) #18
leonhsl(Using Gerrit)
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 ...
8 months ago (2017-02-16 05:00:05 UTC) #21
mlamouri (slow - plz ping)
Leon, could you specify exactly which questions you want an answer to? I was hoping ...
8 months ago (2017-02-17 12:04:06 UTC) #32
leonhsl(Using Gerrit)
On 2017/02/17 12:04:06, mlamouri wrote: > Leon, could you specify exactly which questions you want ...
8 months ago (2017-02-17 13:27:44 UTC) #33
leonhsl(Using Gerrit)
8 months 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 81bcdb8aa