Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428383007:
[Chromecast] Add input support. (Closed)
|
Created:
4 years, 2 months ago by Joshua LeVasseur Modified:
3 years, 11 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, yukawa Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add input support.
Although this enables input, input won't be delivered to the
WebContents due to lack of an aura::client::FocusClient implementation.
BUG=internal b/33047358
TEST=confirm that app launching works as expected
Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd
Review-Url: https://codereview.chromium.org/2428383007
Cr-Commit-Position: refs/heads/master@{#444141}
Committed: https://chromium.googlesource.com/chromium/src/+/3a1dc66b17f7a027c762073355b32dcfb32af505
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added input support #Patch Set 3 : Some refactoring and code cleanup #Patch Set 4 : feature complete #Patch Set 5 : rebase #Patch Set 6 : minor cleanups #Patch Set 7 : fix android build #Patch Set 8 : Split CL; now only adds input support. #Patch Set 9 : formatting #Patch Set 10 : rebase #
Total comments: 9
Patch Set 11 : addressed feedback #Patch Set 12 : fix bad change #
Messages
Total messages: 56 (40 generated)
Description was changed from ========== [Chromecast] Reuse the Aura window manager across receiver apps. This is a starting point for building a window manager. Its initial functionality can show an overlay with its z-order enforced. BUG= Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f ========== to ========== [Chromecast] Reuse the Aura window manager across receiver apps. This is a starting point for building a window manager. Its initial functionality can show an overlay with its z-order enforced. BUG= Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f ==========
jlevasseur@chromium.org changed reviewers: + derekjchow@chromium.org, halliwell@chromium.org
jlevasseur@chromium.org changed reviewers: + zhengxiong@chromium.org
A few comments to get some discussion going. https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_bro... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_bro... chromecast/browser/cast_browser_main_parts.cc:459: cast_window_manager_.reset(new CastWindowManagerAura()); Any reason this can't be added in the contructor of CastBrowserMainParts? https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_con... File chromecast/browser/cast_content_window.h (right): https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_con... chromecast/browser/cast_content_window.h:21: class CastContentWindow : public content::WebContentsObserver { Is this class used publically anymore? https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... File chromecast/browser/cast_window_manager.h (right): https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... chromecast/browser/cast_window_manager.h:16: class CastWindowManager { Please add documentation for this class. What is it's purpose? What does the lifetime for this class look like? Should only one instance of this class exist per cast_shell? https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... chromecast/browser/cast_window_manager.h:22: virtual void StartWindowManager(); Should there be a StopWindowManager as well? https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... chromecast/browser/cast_window_manager.h:23: virtual void AddWebContents(content::WebContents* web_contents, How do we remove a WebContents from the window manager? Similarly, how do we add something that's not a WebContents? How is z-ordering enforced for multiple windows?
https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... File chromecast/browser/cast_window_manager.h (right): https://codereview.chromium.org/2428383007/diff/1/chromecast/browser/cast_win... chromecast/browser/cast_window_manager.h:16: class CastWindowManager { I'm slightly sceptical about this interface: * StartWindowManager seems to get called implicitly from AddWebContents, so it's not clear that it needs to be part of public interface * 'window manager' is an existing concept (both generally and in Chromium), but this doesn't really seem to match that. Are there plans for extending the interface? * Why do you add WebContents to it? Shouldn't you add a window? I would recommend syncing up with alokp on what the future of window management and display may look like under mojo, and figure out how this would fit into it.
Description was changed from
==========
[Chromecast] Reuse the Aura window manager across receiver apps.
This is a starting point for building a window manager. Its initial
functionality can show an overlay with its z-order enforced.
BUG=
Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========
to
==========
[Chromecast] Reuse the Aura window manager across receiver apps.
This is a starting point for building a window manager. Its initial
functionality can show an overlay with its z-order enforced.
This also adds input support for Cast.
BUG=internal b/33046596, internal b/33047358
Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========
The CQ bit was checked by jlevasseur@chromium.org 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: 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...)
The CQ bit was checked by jlevasseur@chromium.org 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: 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...)
The CQ bit was checked by jlevasseur@chromium.org 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.
Description was changed from
==========
[Chromecast] Reuse the Aura window manager across receiver apps.
This is a starting point for building a window manager. Its initial
functionality can show an overlay with its z-order enforced.
This also adds input support for Cast.
BUG=internal b/33046596, internal b/33047358
Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========
to
==========
[Chromecast] Reuse the Aura window manager across receiver apps.
Implements a basic window manager, which uses the window ID to determine z
order.
Also:
- Adds input support for Cast.
- Always makes the root window transparent for seeing the video plane
beneath it.
- Allows tearing down and rebuilding the window manager.
BUG=internal b/33046596, internal b/33047358, internal b/33835219
TEST=manually test all categories of receiver apps and mirroring.
Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========
jlevasseur@chromium.org changed reviewers: + spang@chromium.org
This change is ready for review, although there will be some future changes for merging with https://codereview.chromium.org/2570623003/
On 2017/01/12 02:10:50, Joshua LeVasseur wrote: > This change is ready for review, although there will be some future changes for > merging with https://codereview.chromium.org/2570623003/ Please split this up into smaller reviews. It seems like there could be at least 3 (add basic window manager, add z ordering, add input support). It will get reviewed much quicker that way ;)
On 2017/01/12 16:45:59, halliwell wrote: > On 2017/01/12 02:10:50, Joshua LeVasseur wrote: > > This change is ready for review, although there will be some future changes > for > > merging with https://codereview.chromium.org/2570623003/ > > Please split this up into smaller reviews. It seems like there could be at > least 3 (add basic window manager, add z ordering, add input support). > It will get reviewed much quicker that way ;) ok, I'm in the process of splitting them ...
The CQ bit was checked by jlevasseur@chromium.org to run a CQ dry run
Description was changed from
==========
[Chromecast] Reuse the Aura window manager across receiver apps.
Implements a basic window manager, which uses the window ID to determine z
order.
Also:
- Adds input support for Cast.
- Always makes the root window transparent for seeing the video plane
beneath it.
- Allows tearing down and rebuilding the window manager.
BUG=internal b/33046596, internal b/33047358, internal b/33835219
TEST=manually test all categories of receiver apps and mirroring.
Change-Id: I01d9ae64d3175298d439577e4c83bea4ada6203f
==========
to
==========
[Chromecast] Add input support.
Although this enables input, input won't be delivered to the
WebContents due to lack of an aura::client::FocusClient implementation.
BUG=internal b/33047358
TEST=confirm that app launching works as expected
Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd
==========
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: 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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jlevasseur@chromium.org to run a CQ dry run
Description was changed from
==========
[Chromecast] Add input support.
Although this enables input, input won't be delivered to the
WebContents due to lack of an aura::client::FocusClient implementation.
BUG=internal b/33047358
TEST=confirm that app launching works as expected
Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd
==========
to
==========
[Chromecast] Add input support.
Although this enables input, input won't be delivered to the
WebContents due to lack of an aura::client::FocusClient implementation.
BUG=internal b/33047358
TEST=confirm that app launching works as expected
Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd
==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Chromecast] Add input support. Although this enables input, input won't be delivered to the WebContents due to lack of an aura::client::FocusClient implementation. BUG=internal b/33047358 TEST=confirm that app launching works as expected Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd ========== to ========== [Chromecast] Add input support. Although this enables input, input won't be delivered to the WebContents due to lack of an aura::client::FocusClient implementation. BUG=internal b/33047358 TEST=confirm that app launching works as expected Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd ==========
jlevasseur@chromium.org changed reviewers: + yukawa@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jlevasseur@chromium.org 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.
Ready for review: it adds input support to Chromecast. Note: it adds a dependency on ui/base/ime, and makes a slight change to event_thread_evdev.cc
https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... File chromecast/browser/cast_content_window_linux.cc (right): https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... chromecast/browser/cast_content_window_linux.cc:68: explicit CastWindowTreeHost(bool enable_input, const gfx::Rect& bounds); No need for explicit for two parameter constructor. https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... chromecast/browser/cast_content_window_linux.cc:71: // aura::WindowTreeHostPlatform: aura::WindowTreeHostPlatform implementation: https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... File ui/events/ozone/evdev/event_thread_evdev.cc (right): https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... ui/events/ozone/evdev/event_thread_evdev.cc:46: if (cursor_) Can Cast provide its own CursorDelegateEvdev? If so, we could provide a no-op InitializeOnEvdev implementation and remove the need for this if-statement. https://codereview.chromium.org/2428383007/diff/180001/ui/ozone/platform/cast... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/2428383007/diff/180001/ui/ozone/platform/cast... ui/ozone/platform/cast/ozone_platform_cast.cc:104: base::WrapUnique(new StubKeyboardLayoutEngine())); MakeUnique would be more concise
https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... File chromecast/browser/cast_content_window_linux.cc (right): https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... chromecast/browser/cast_content_window_linux.cc:68: explicit CastWindowTreeHost(bool enable_input, const gfx::Rect& bounds); On 2017/01/13 18:12:28, derekjchow1 wrote: > No need for explicit for two parameter constructor. Done. https://codereview.chromium.org/2428383007/diff/180001/chromecast/browser/cas... chromecast/browser/cast_content_window_linux.cc:71: // aura::WindowTreeHostPlatform: On 2017/01/13 18:12:28, derekjchow1 wrote: > aura::WindowTreeHostPlatform implementation: Done. https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... File ui/events/ozone/evdev/event_thread_evdev.cc (right): https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... ui/events/ozone/evdev/event_thread_evdev.cc:46: if (cursor_) On 2017/01/13 18:12:28, derekjchow1 wrote: > Can Cast provide its own CursorDelegateEvdev? If so, we could provide a no-op > InitializeOnEvdev implementation and remove the need for this if-statement. Glancing through the evdev code, it seems to be designed to handle a null cursor_ (it totally used to handle a null cursor_ when I started prototyping this code). We don't support a mouse, so a nullptr seems appropriate, rather than a no-op CursorDelegateEvdev --- but I'm guessing about intentions of the evdev layer. https://codereview.chromium.org/2428383007/diff/180001/ui/ozone/platform/cast... File ui/ozone/platform/cast/ozone_platform_cast.cc (right): https://codereview.chromium.org/2428383007/diff/180001/ui/ozone/platform/cast... ui/ozone/platform/cast/ozone_platform_cast.cc:104: base::WrapUnique(new StubKeyboardLayoutEngine())); On 2017/01/13 18:12:28, derekjchow1 wrote: > MakeUnique would be more concise Done.
ui/events/ozone lgtm https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... File ui/events/ozone/evdev/event_thread_evdev.cc (right): https://codereview.chromium.org/2428383007/diff/180001/ui/events/ozone/evdev/... ui/events/ozone/evdev/event_thread_evdev.cc:46: if (cursor_) On 2017/01/13 23:38:13, Joshua LeVasseur wrote: > On 2017/01/13 18:12:28, derekjchow1 wrote: > > Can Cast provide its own CursorDelegateEvdev? If so, we could provide a no-op > > InitializeOnEvdev implementation and remove the need for this if-statement. > > Glancing through the evdev code, it seems to be designed to handle a null > cursor_ (it totally used to handle a null cursor_ when I started prototyping > this code). We don't support a mouse, so a nullptr seems appropriate, rather > than a no-op CursorDelegateEvdev --- but I'm guessing about intentions of the > evdev layer. Yes, it did support null, it just breaks from time to time because it has no integration test coverage (CrOS tests use the cursor, obviously, so the non-null case gets tested). Would be good to fix that. A stub CursorDelegateEvdev does not have quite the same effect as a null one - a stub that always reports a position of (0,0) would cause a CSS hover effect there, for example.
yukawa@chromium.org changed reviewers: + shuchen@chromium.org
I think shuchen@ should have better understanding of IME focus handling on Chromium than me. Over to him and moving me to CC.
(I'm not familiar with cast.) The part of CastWindowTreeHost implementation lgtm.
On 2017/01/17 03:16:46, Shu Chen wrote: > (I'm not familiar with cast.) > The part of CastWindowTreeHost implementation lgtm. lgtm
The CQ bit was checked by jlevasseur@chromium.org
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 jlevasseur@chromium.org
The CQ bit was checked by jlevasseur@chromium.org
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": 220001, "attempt_start_ts": 1484686956323090,
"parent_rev": "b36b88ee87e3a4b5445cd6e865736a011541a714", "commit_rev":
"3a1dc66b17f7a027c762073355b32dcfb32af505"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add input support. Although this enables input, input won't be delivered to the WebContents due to lack of an aura::client::FocusClient implementation. BUG=internal b/33047358 TEST=confirm that app launching works as expected Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd ========== to ========== [Chromecast] Add input support. Although this enables input, input won't be delivered to the WebContents due to lack of an aura::client::FocusClient implementation. BUG=internal b/33047358 TEST=confirm that app launching works as expected Change-Id: I33ee871d63b84bce1236e5673f3ea900d3259efd Review-Url: https://codereview.chromium.org/2428383007 Cr-Commit-Position: refs/heads/master@{#444141} Committed: https://chromium.googlesource.com/chromium/src/+/3a1dc66b17f7a027c762073355b3... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/3a1dc66b17f7a027c762073355b3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
