Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(194)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2092343002: Touch HUD app for mustash (Closed)

Created:
4 years, 6 months ago by riajiang
Modified:
4 years, 5 months ago
Reviewers:
sadrul
CC:
chromium-reviews, rjkroege, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, tfarina, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Touch HUD app for mustash. BUG=588311

Patch Set 1 #

Total comments: 16

Patch Set 2 : Use PointerKindMatcher to get touch events; add layout manager #

Patch Set 3 : Change mus ws to not dispatch events to clients that set their accept_events to false. #

Patch Set 4 : Use a new touch event watcher instead. #

Patch Set 5 : Change from using components/mus to services/ui #

Patch Set 6 : Change similarity settings for git cl upload #

Patch Set 7 : Change window manager connection unittest to original #

Unified diffs Side-by-side diffs Delta from patch set Stats (+391 lines, -261 lines) Patch
M ash/mus/BUILD.gn View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M ash/mus/container_ids.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M ash/mus/root_window_controller.cc View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download
A + ash/mus/touch_hud_layout.h View 1 2 3 4 2 chunks +8 lines, -8 lines 0 comments Download
A + ash/mus/touch_hud_layout.cc View 1 2 3 4 2 chunks +4 lines, -10 lines 0 comments Download
A + ash/public/interfaces/OWNERS View 1 2 3 4 0 chunks +-1 lines, --1 lines 0 comments Download
M ash/public/interfaces/container.mojom View 1 chunk +1 line, -0 lines 0 comments Download
M ash/touch/touch_hud_projection.h View 3 chunks +6 lines, -4 lines 0 comments Download
M ash/touch/touch_hud_projection.cc View 1 2 2 chunks +10 lines, -143 lines 0 comments Download
M ash/touch/touch_observer_hud_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M chrome/app/mash/BUILD.gn View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download
M chrome/app/mash/mash_runner.cc View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M mash/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M mash/app_driver/BUILD.gn View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M mash/app_driver/app_driver.cc View 1 2 3 4 4 chunks +6 lines, -1 line 0 comments Download
A + mash/touch_hud/BUILD.gn View 1 2 3 4 3 chunks +7 lines, -5 lines 0 comments Download
A + mash/touch_hud/main.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
A + mash/touch_hud/manifest.json View 1 1 chunk +2 lines, -2 lines 0 comments Download
A + mash/touch_hud/touch_hud_application.h View 1 3 chunks +13 lines, -16 lines 0 comments Download
A mash/touch_hud/touch_hud_application.cc View 1 2 3 4 1 chunk +122 lines, -0 lines 0 comments Download
M services/ui/public/cpp/lib/window.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M services/ui/public/cpp/lib/window_tree_client.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M services/ui/public/cpp/tests/test_window_tree.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M services/ui/public/cpp/tests/test_window_tree.cc View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/public/cpp/window.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/public/cpp/window_tree_client.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/public/interfaces/window_tree.mojom View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M services/ui/ws/server_window.h View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download
M services/ui/ws/server_window.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M services/ui/ws/window_finder.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M services/ui/ws/window_tree.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M services/ui/ws/window_tree.cc View 1 2 3 4 3 chunks +28 lines, -6 lines 0 comments Download
A + ui/views/controls/touch/touch_hud_drawer.h View 1 2 3 4 5 1 chunk +29 lines, -13 lines 0 comments Download
A + ui/views/controls/touch/touch_hud_drawer.cc View 1 2 5 chunks +42 lines, -38 lines 0 comments Download
M ui/views/mus/native_widget_mus.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M ui/views/mus/native_widget_mus.cc View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M ui/views/mus/window_manager_connection.h View 1 2 3 4 4 chunks +6 lines, -0 lines 0 comments Download
M ui/views/mus/window_manager_connection.cc View 1 2 3 4 4 chunks +35 lines, -0 lines 0 comments Download
A + ui/views/touch_event_watcher.h View 1 2 3 4 5 1 chunk +14 lines, -11 lines 0 comments Download
M ui/views/views.gyp View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (4 generated)
sadrul
https://codereview.chromium.org/2092343002/diff/1/components/mus/public/interfaces/event_matcher.mojom File components/mus/public/interfaces/event_matcher.mojom (right): https://codereview.chromium.org/2092343002/diff/1/components/mus/public/interfaces/event_matcher.mojom#newcode52 components/mus/public/interfaces/event_matcher.mojom:52: EventMultTypeMatcher? types_matcher; Instead of this, it'd be better to ...
4 years, 5 months ago (2016-06-27 14:44:54 UTC) #3
riajiang
4 years, 5 months ago (2016-06-28 21:52:51 UTC) #4 
https://codereview.chromium.org/2092343002/diff/1/components/mus/public/inter...
File components/mus/public/interfaces/event_matcher.mojom (right):

https://codereview.chromium.org/2092343002/diff/1/components/mus/public/inter...
components/mus/public/interfaces/event_matcher.mojom:52: EventMultTypeMatcher?
types_matcher;
On 2016/06/27 14:44:54, sadrul wrote:
> Instead of this, it'd be better to use the PointerKindMatcher to match all
> PointerKind::TOUCH events. That should give us all touch-events that we care
> about.

Done.

https://codereview.chromium.org/2092343002/diff/1/mash/touch/manifest.json
File mash/touch/manifest.json (right):

https://codereview.chromium.org/2092343002/diff/1/mash/touch/manifest.json#ne...
mash/touch/manifest.json:3: "name": "mojo:touch",
On 2016/06/27 14:44:54, sadrul wrote:
> "touch" is too generic. Let's call this touch_hud

Done.

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
File mash/touch/touch_hud_application.cc (right):

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
mash/touch/touch_hud_application.cc:124:
connector_->ConnectToInterface("mojo:catalog", &catalog);
On 2016/06/27 14:44:54, sadrul wrote:
> You don't need this.

Done.

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
mash/touch/touch_hud_application.cc:126: if (!touch_hud_enabled) {
On 2016/06/27 14:44:54, sadrul wrote:
> You don't need |touch_hud_enabled_|. You can just look at whether |windows_|
is
> empty or not (and you should actually need just a single window).

Done and done.

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
mash/touch/touch_hud_application.cc:134: std::move(catalog));
On 2016/06/27 14:44:54, sadrul wrote:
> Try setting params.bounds to something (e.g. gfx::Rect(0, 0, 800, 600)).
> Ideally, we wouldn't have to specify the size for the window at all. Instead,
> the window-manager (//ash/mus) should install a mus::LayoutManager for the
> OVERLAY window (see
>
//ash/mus/root_window_controller.cc:RootWindowController::CreateLayoutManagers()),
> which sets the size to fill the display-size.

Done.

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
File mash/touch/touch_hud_application.h (right):

https://codereview.chromium.org/2092343002/diff/1/mash/touch/touch_hud_applic...
mash/touch/touch_hud_application.h:57: bool touch_hud_enabled = false;
On 2016/06/27 14:44:54, sadrul wrote:
> member variables should end with an underscore

Done.

https://codereview.chromium.org/2092343002/diff/1/ui/views/mus/window_manager...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2092343002/diff/1/ui/views/mus/window_manager...
ui/views/mus/window_manager_connection.cc:98:
client_->SetEventObserver(std::move(matcher));
On 2016/06/27 14:44:54, sadrul wrote:
> We currently have PointerWatcher instances that want only the POINTER_DOWN
> events. So for most of these cases, we should continue to register only for
> POINTER_DOWN. For the cases where we want additional types of events, we
should
> introduce a new explicit API, for example:
> WindowManagerConnection::AddTouchEventWatcher().
> 
> I think for now, we won't have clients that use both a PointerWatcher and a
> TouchEventWatcher. So WindowManagerConnection (WMC) does not need to merge the
> two requests. But in case some clients do want to install both types of
watcher,
> WMC would need to merge the two requests to make sure it receives the right
set
> of events.

Done.

https://codereview.chromium.org/2092343002/diff/1/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2092343002/diff/1/ui/views/pointer_watcher.h#...
ui/views/pointer_watcher.h:46: Widget* target) = 0;
On 2016/06/27 14:44:54, sadrul wrote:
> *Cancel
> 
> Instead of having type-specific callbacks, let's introduce a simpler
> OnTouchEvent(...) instead.

Done.

Powered by Google App Engine
This is Rietveld 408576698