Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578503002:
[DeviceService] Mojofy left screen orientation IPC messages. (Closed)
|
Created:
4 years ago by leonhsl(Using Gerrit) Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, mlamouri+watch-screen-orientation_chromium.org, mlamouri+watch-content_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[DeviceService] Mojofy left screen orientation IPC messages.
This CL converts following IPCs into mojo interface:
ScreenOrientationHostMsg_StartListening
ScreenOrientationHostMsg_StopListening
BUG=612339
TBR=kinuko@chromium.org
Committed: https://crrev.com/bd4620dd0a74a6af494b51983903cb09e9aa75cb
Cr-Commit-Position: refs/heads/master@{#440003}
Patch Set 1 #Patch Set 2 #
Total comments: 11
Patch Set 3 : Address comments from mlamouri@ #
Total comments: 8
Patch Set 4 : Address nits from blundell@ #Messages
Total messages: 42 (24 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: + blundell@chromium.org, mlamouri@chromium.org, rockot@chromium.org
Hi, would you PTAL at this? Thanks! I'd like to land this CL firstly before decoupling codes into blink, because I need some more time to figure out how to enable blink to talk with content::BrowserAssociatedInterface. blundell@ and Ken: For overall review. mlamouri@ : For OWNER review of screen_orientation bits.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.cc (right): https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.cc:22: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Is there a requirement for the class to be created on the IO thread too? In which case would it make sense to use NonThreadSafe or ThreadChecker? https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.h (right): https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.h:24: bool OnMessageReceived(const IPC::Message& message) override; Why are you keeping this? Does it still need to be a BrowserMessageFilter? https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... File device/screen_orientation/public/interfaces/screen_orientation.mojom (right): https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:15: interface ScreenOrientationAccurate { Can you add some documentation explaining why and when this is needed: ``` ScreenOrientationAccurate is expected to be used when the platform requires heavy work in order to accurately know the screen orientation. For example, on Android, this is required for Jelly Bean, where there is no API to be notified of a screen orientation change of 180 degrees. ``` Feel free to change :) https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:20: // requires heavy work in order to accurately know the screen orientation. I think you can remove these last two lines (some for StopListening). The top documentation should be enough.
I'm wondering if we really need these as separate IPCs rather than implementation details of the host side. Mounir, I assume that there's some reason the following structure wouldn't work? - There's a host-side ScreenOrientationAccuracyController class. - Each ScreenOrientation instance notifies ScreenOrientationAccuracyController when it gets created and destroyed. - When the number of active ScreenOrientation instances flips (0 to non-zero, non-zero to 0), ScreenOrientationAccuracyController calls StartAccurateListening() and StopAccurateListening() respectively. - Those methods have default do-nothing implementations but can be overridden to have per-platform implementations (e.g., on Android) (or just have ifdefs in the one impl, either way).
https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.cc (right): https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.cc:22: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2016/12/15 10:30:56, mlamouri wrote: > Is there a requirement for the class to be created on the IO thread too? In > which case would it make sense to use NonThreadSafe or ThreadChecker? ScreenOrientationMessageFilterAndroid is a BrowserMessageFilter which is expected to be created on UI thread but run on IO thread. It's created in RenderProcessHostImpl::CreateMessageFilters(). # I'll explain why it must keep as BrowserMessageFilter while addressing another comment in screen_orientation_message_filter_android.h :) https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.h (right): https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.h:24: bool OnMessageReceived(const IPC::Message& message) override; On 2016/12/15 10:30:56, mlamouri wrote: > Why are you keeping this? Does it still need to be a BrowserMessageFilter? Yeah actually now screen_orientation_message_filter_android has no IPC message to handle, but I have to keep it as a BrowserMessageFilter, because we're using the mojofication approach described at [1], and we must combine both BrowserAssociatedInterface and BrowserMessageFilter according [2]. Ken has implemented this framework so maybe he can help confirm this. Thanks. [1] https://www.chromium.org/developers/design-documents/mojo/mojo-migration-guid... [2] https://cs.chromium.org/chromium/src/content/public/browser/browser_associate... https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... File device/screen_orientation/public/interfaces/screen_orientation.mojom (right): https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:15: interface ScreenOrientationAccurate { On 2016/12/15 10:30:56, mlamouri wrote: > Can you add some documentation explaining why and when this is needed: > ``` > ScreenOrientationAccurate is expected to be used when the platform requires > heavy work in order > to accurately know the screen orientation. For example, on Android, this is > required for Jelly Bean, > where there is no API to be notified of a screen orientation change of 180 > degrees. > ``` > > Feel free to change :) Acknowledged. https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:20: // requires heavy work in order to accurately know the screen orientation. On 2016/12/15 10:30:57, mlamouri wrote: > I think you can remove these last two lines (some for StopListening). The top > documentation should be enough. Acknowledged.
On 2016/12/15 18:36:55, blundell wrote: > I'm wondering if we really need these as separate IPCs rather than > implementation details of the host side. Mounir, I assume that there's some > reason the following structure wouldn't work? > > - There's a host-side ScreenOrientationAccuracyController class. > - Each ScreenOrientation instance notifies ScreenOrientationAccuracyController > when it gets created and destroyed. > - When the number of active ScreenOrientation instances flips (0 to non-zero, > non-zero to 0), ScreenOrientationAccuracyController calls > StartAccurateListening() and StopAccurateListening() respectively. > - Those methods have default do-nothing implementations but can be overridden to > have per-platform implementations (e.g., on Android) (or just have ifdefs in the > one impl, either way). Hi, Colin, Mounir, I checked around this and maybe the possible difference with current IPCs solution is the change of accurate listening time-span? ScreenOrientation lifecycle is bound with WebContentsImpl, which will last for relatively long time. While IPCs solution can help control the necessary time-span precisely. Another idea to make things simpler? : - Merge these 2 IPCs(start/stop) into existing device.mojom.ScreenOrientation interface. - content::ScreenOrientation handles start/stop by increasing/decreasing a counter, calls StartAccurateListening() and StopAccurateListening() respectively when the counter flips. - Eliminate blink::ScreenOrientationDispatcher, because it is only responsible for sending start/stop, but now blink::ScreenOrientationControllerImpl becomes able to do this task by itself. But seems this has a time-span problem, too: Currently, suppose some one render process got crashed incidentally, the corresponding render process host will destroy, then its ScreenOrientationMessageFilterAndroid will destroy, at [1] StopAccurateListening() will be called, means that even there has no stop IPC received, accurate listening can be stopped correctly. But if we handles start/stop in content::ScreenOrientation, we expect all render processes serving for this WebContentsImpl to send start/stop restrictively with same number. [1] https://cs.chromium.org/chromium/src/content/browser/screen_orientation/scree...
I'm curious if Mounir can detail exactly what the current lifetime between StartListening and StopListening IPCs is. That was difficult for me to tease out from the various layers.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Friendly ping Mounir for above discussion, Thanks. https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... File device/screen_orientation/public/interfaces/screen_orientation.mojom (right): https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:15: interface ScreenOrientationAccurate { On 2016/12/16 03:12:00, leonhsl wrote: > On 2016/12/15 10:30:56, mlamouri wrote: > > Can you add some documentation explaining why and when this is needed: > > ``` > > ScreenOrientationAccurate is expected to be used when the platform requires > > heavy work in order > > to accurately know the screen orientation. For example, on Android, this is > > required for Jelly Bean, > > where there is no API to be notified of a screen orientation change of 180 > > degrees. > > ``` > > > > Feel free to change :) > > Acknowledged. Done and Thanks! https://codereview.chromium.org/2578503002/diff/20001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:20: // requires heavy work in order to accurately know the screen orientation. On 2016/12/16 03:12:00, leonhsl wrote: > On 2016/12/15 10:30:57, mlamouri wrote: > > I think you can remove these last two lines (some for StopListening). The top > > documentation should be enough. > > Acknowledged. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
To answer blundell@ question, the accurate listening happens when `screen.orientation` object is created which is different from `screen.orientation.lock()`. It can't be merged with the locking interface. https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.h (right): https://codereview.chromium.org/2578503002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.h:24: bool OnMessageReceived(const IPC::Message& message) override; On 2016/12/16 at 03:12:00, leonhsl wrote: > On 2016/12/15 10:30:56, mlamouri wrote: > > Why are you keeping this? Does it still need to be a BrowserMessageFilter? > > Yeah actually now screen_orientation_message_filter_android has no IPC message to handle, but I have to keep it as a BrowserMessageFilter, because we're using the mojofication approach described at [1], and we must combine both BrowserAssociatedInterface and BrowserMessageFilter according [2]. > Ken has implemented this framework so maybe he can help confirm this. Thanks. > > [1] https://www.chromium.org/developers/design-documents/mojo/mojo-migration-guid... > [2] https://cs.chromium.org/chromium/src/content/public/browser/browser_associate... I would understand why we should have BrowserMessageFilter if it was still used but I can't understand why we need to keep the interface for no reason.
Hi Mounir, Thanks, I understand now. BrowserAssociatedInterface is the canonical way to have FIFO ordering between the messages of the Mojo interface in question and the RenderProcessHost's IPC channel. That ordering is implemented by the calling of AddAssociatedInterface() on the BrowserMessageFilter in question. That's why BrowserAssociatedInterface requires that there be a BrowserMessageFilter. It looks like it would be possible to tease out a variant of BrowserAssociatedInterface that just took in a ChannelProxy and directly associated itself with it. I'm not sure that refactoring is worth the effort at this time. There is at least one other example like this (where there's no Chrome IPC listened for): https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_di...
On 2016/12/19 14:18:22, blundell wrote: > Hi Mounir, > > Thanks, I understand now. > > BrowserAssociatedInterface is the canonical way to have FIFO ordering between > the messages of the Mojo interface in question and the RenderProcessHost's IPC > channel. That ordering is implemented by the calling of AddAssociatedInterface() > on the BrowserMessageFilter in question. That's why BrowserAssociatedInterface > requires that there be a BrowserMessageFilter. > > It looks like it would be possible to tease out a variant of > BrowserAssociatedInterface that just took in a ChannelProxy and directly > associated itself with it. I'm not sure that refactoring is worth the effort at > this time. There is at least one other example like this (where there's no > Chrome IPC listened for): > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_di... Thanks Colin a lot for explanations and the example, so clearer than mine :)
Thanks for the explanations :) lgtm
leon.han@intel.com changed reviewers: + tsepez@chromium.org
Hi, Tom, would you PTAL for OWNER review of mojom/message files? Thanks. Hi, Colin, do you have more comments about this CL? Thanks.
lgtm, just cosmetic nits. Thanks! https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.cc (right): https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.cc:22: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Let's use a base::ThreadChecker instead since we'll want to move this class out of //content at some point. In the constructor you can check base::MessageLoopForIO::IsCurrent(). https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_message_filter_android.h (right): https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_message_filter_android.h:15: class ScreenOrientationMessageFilterAndroid Let's rename this to ScreenOrientationListenerAndroid. The fact that it's a BrowserMessageFilter is basically an implementation detail at this point. https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati... File device/screen_orientation/public/interfaces/screen_orientation.mojom (right): https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:19: interface ScreenOrientationAccurate { I suggest we rename this ScreenOrientationListener. https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati... device/screen_orientation/public/interfaces/screen_orientation.mojom:23: StartListening(); We can rename these to Start and Stop. So we'll have calls like e.g. listener_->Start()
mojom LGTM
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
leon.han@intel.com changed reviewers: + kinuko@chromium.org
Uploaded ps#4 to address nits from Colin, almost are naming changes, so I want
to land ps#4 now without requesting Colin to take another look.
Also, let me TBR kinuko@ for changes of content/browser/BUILD.gn and
content/browser/renderer_host/render_process_host_impl.cc, because these are all
trivial naming changes:
screen_orientation_message_filter_android.{cc,h} -->
screen_orientation_listener_android.{cc,h}
ScreenOrientationMessageFilterAndroid --> ScreenOrientationListenerAndroid
Thanks all!
https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.cc
(right):
https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_message_filter_android.cc:22:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
On 2016/12/20 13:39:18, blundell wrote:
> Let's use a base::ThreadChecker instead since we'll want to move this class
out
> of //content at some point. In the constructor you can check
> base::MessageLoopForIO::IsCurrent().
Done.
Because ScreenOrientationMessageFilterAndroid is expected to be created on
render main thread while running all other member functions on IO thread, I
think base::ThreadChecker does not fit such case.
So I changed to use DCHECK(base::MessageLoopForIO::IsCurrent()) instead of
DCHECK_CURRENTLY_ON(content::BrowserThread::IO) throughout this file, to
explicitly check IO thread execution and can also avoid dependency on
content::BrowserThread.
https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_...
File
content/browser/screen_orientation/screen_orientation_message_filter_android.h
(right):
https://codereview.chromium.org/2578503002/diff/40001/content/browser/screen_...
content/browser/screen_orientation/screen_orientation_message_filter_android.h:15:
class ScreenOrientationMessageFilterAndroid
On 2016/12/20 13:39:18, blundell wrote:
> Let's rename this to ScreenOrientationListenerAndroid. The fact that it's a
> BrowserMessageFilter is basically an implementation detail at this point.
Done.
https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati...
File device/screen_orientation/public/interfaces/screen_orientation.mojom
(right):
https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati...
device/screen_orientation/public/interfaces/screen_orientation.mojom:19:
interface ScreenOrientationAccurate {
On 2016/12/20 13:39:18, blundell wrote:
> I suggest we rename this ScreenOrientationListener.
Done.
https://codereview.chromium.org/2578503002/diff/40001/device/screen_orientati...
device/screen_orientation/public/interfaces/screen_orientation.mojom:23:
StartListening();
On 2016/12/20 13:39:18, blundell wrote:
> We can rename these to Start and Stop. So we'll have calls like e.g.
>
> listener_->Start()
Done.
Description was changed from ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 ========== to ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 TBR=kinuko@chromium.org ==========
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, mlamouri@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2578503002/#ps80001 (title: "Address nits from blundell@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482288626850620,
"parent_rev": "11f08f2f24669169277b909e845862756c1cbf29", "commit_rev":
"448d7c4f64a6db4b5c4603970143de80eeff3139"}
Message was sent while issue was closed.
Description was changed from ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 TBR=kinuko@chromium.org ========== to ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 TBR=kinuko@chromium.org Review-Url: https://codereview.chromium.org/2578503002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 TBR=kinuko@chromium.org Review-Url: https://codereview.chromium.org/2578503002 ========== to ========== [DeviceService] Mojofy left screen orientation IPC messages. This CL converts following IPCs into mojo interface: ScreenOrientationHostMsg_StartListening ScreenOrientationHostMsg_StopListening BUG=612339 TBR=kinuko@chromium.org Committed: https://crrev.com/bd4620dd0a74a6af494b51983903cb09e9aa75cb Cr-Commit-Position: refs/heads/master@{#440003} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/bd4620dd0a74a6af494b51983903cb09e9aa75cb Cr-Commit-Position: refs/heads/master@{#440003} |
