|
|
Created:
3 years, 8 months ago by xc Modified:
3 years, 8 months ago CC:
chromium-reviews, sadrul, Matt Giuca, viettrungluu+watch_chromium.org, tfarina, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review), qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd app list button long press action
TEST=locally on device, try long press and see the action handled
BUG=b:36522311
Review-Url: https://codereview.chromium.org/2780053002
Cr-Commit-Position: refs/heads/master@{#461285}
Committed: https://chromium.googlesource.com/chromium/src/+/dad576a51b2a29433e8c86fd29b092d32559c628
Patch Set 1 #Patch Set 2 : fix ripple after finger lift #
Total comments: 21
Patch Set 3 : fix comments and add tests #
Total comments: 30
Patch Set 4 : fix comments #
Total comments: 17
Patch Set 5 : fix comments #
Total comments: 8
Patch Set 6 : fix comments #Patch Set 7 : missed updating ExampleAppListPresenter #Messages
Total messages: 46 (22 generated)
Description was changed from ========== Add app list button long press action TEST=locally on device, try long press and see the action handled BUG=b:36522311 ========== to ========== Add app list button long press action TEST=locally on device, try long press and see the action handled BUG=b:36522311 ==========
xiaohuic@chromium.org changed reviewers: + msw@chromium.org, xiyuan@chromium.org
Please take a first look at if this is the approach we agreed on. If it looks good, I am going to work on some tests.
The approach seems fine, my comments are mostly about naming. https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:107: LOG(ERROR) << "XXC: event type " << event->type(); Remove this logging (or formalize some [D]VLOG statement) https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:132: LOG(ERROR) << "XXC: handle long press"; ditto https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:140: // presses and lifts the finger. We already handled the long press nit: one space after period https://codereview.chromium.org/2780053002/diff/20001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2780053002/diff/20001/ash/shell.h#newcode548 ash/shell.h:548: void OnLongPress(); Do not add this to Shell, instead inline Shell::Get()->app_list()->OnLongPress() or similar. (Shell is overused as a catch-all place for miscellaneous functions, and should not be used as such, especially when there is an accessor for the relevant member; the other app list functions should be removed separately) https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/app_list/app_list_presenter_service.cc (right): https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:7: #include <utility> q: remove this if it's not needed https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:49: auto* framework_instance = Please avoid auto here, specifying the concrete type would be very nice, given the complexity of the macro and chain of accessors used. https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:54: if (!framework_instance) optional nit: invert the condition and nest the call below. https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.cc (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.cc:7: #include <utility> what is this used for? https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.h (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.h:32: void OnLongPress(); Rename this something like StartVoiceInteractionSession, add a display id parameter to support multiple displays. https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list_presenter.mojom (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list_presenter.mojom:38: OnLongPress(); Ditto: Rename this something like StartVoiceInteractionSession, add a display id parameter to support multiple displays.
PTAL https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:107: LOG(ERROR) << "XXC: event type " << event->type(); On 2017/03/29 00:24:52, msw wrote: > Remove this logging (or formalize some [D]VLOG statement) oops, thought already removed these. https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:132: LOG(ERROR) << "XXC: handle long press"; On 2017/03/29 00:24:52, msw wrote: > ditto Done. https://codereview.chromium.org/2780053002/diff/20001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:140: // presses and lifts the finger. We already handled the long press On 2017/03/29 00:24:52, msw wrote: > nit: one space after period Done. https://codereview.chromium.org/2780053002/diff/20001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2780053002/diff/20001/ash/shell.h#newcode548 ash/shell.h:548: void OnLongPress(); On 2017/03/29 00:24:52, msw wrote: > Do not add this to Shell, instead inline Shell::Get()->app_list()->OnLongPress() > or similar. (Shell is overused as a catch-all place for miscellaneous functions, > and should not be used as such, especially when there is an accessor for the > relevant member; the other app list functions should be removed separately) Done. https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... File chrome/browser/ui/ash/app_list/app_list_presenter_service.cc (right): https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:7: #include <utility> On 2017/03/29 00:24:52, msw wrote: > q: remove this if it's not needed It is complained by git cl lint. Some where in the file is using move or something I cannot remember. https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:49: auto* framework_instance = On 2017/03/29 00:24:52, msw wrote: > Please avoid auto here, specifying the concrete type would be very nice, given > the complexity of the macro and chain of accessors used. Done. https://codereview.chromium.org/2780053002/diff/20001/chrome/browser/ui/ash/a... chrome/browser/ui/ash/app_list/app_list_presenter_service.cc:54: if (!framework_instance) On 2017/03/29 00:24:52, msw wrote: > optional nit: invert the condition and nest the call below. Done. https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.cc (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.cc:7: #include <utility> On 2017/03/29 00:24:52, msw wrote: > what is this used for? Same as the other, git cl lint complaining about include what you use on some other lines in the file. https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.h (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.h:32: void OnLongPress(); On 2017/03/29 00:24:53, msw wrote: > Rename this something like StartVoiceInteractionSession, add a display id > parameter to support multiple displays. Done renaming. There is currently no multi display support in arc++ nor voice interaction service we are using. Can we add display id when we actually use it? https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list_presenter.mojom (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list_presenter.mojom:38: OnLongPress(); On 2017/03/29 00:24:53, msw wrote: > Ditto: Rename this something like StartVoiceInteractionSession, add a display id > parameter to support multiple displays. Done.
Nice, I have lots of nits, but that's just how I review :) https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.h (right): https://codereview.chromium.org/2780053002/diff/20001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.h:32: void OnLongPress(); On 2017/03/29 21:34:49, xc wrote: > On 2017/03/29 00:24:53, msw wrote: > > Rename this something like StartVoiceInteractionSession, add a display id > > parameter to support multiple displays. > > Done renaming. There is currently no multi display support in arc++ nor voice > interaction service we are using. Can we add display id when we actually use it? What happens when a user long-presses the app list button on a secondary display? If some UI pops up, presumably it should be on the same display? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:137: if (IsVoiceInteractionEnabled()) { Should this also have an else case to invoke ImageButton::OnGestureEvent? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:58: friend class test::AppListButtonTest; Avoid this if possible; see my SendGestureEvent suggestion in the test file, or use |static_cast<ui::EventHandler>(app_list_button_)->OnGestureEvent(event);|, or make the OnGestureEvent (and other overrides?) above public and regroup/reorder the decls and definitions to match the base class(es). https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button_unittest.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:9: #include "ash/common/test/test_shelf_item_delegate.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:10: #include "ash/common/wm_shell.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:13: #include "ash/test/shelf_view_test_api.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:15: #include "base/memory/ptr_util.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:20: #include "ui/events/test/event_generator.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:21: #include "ui/views/bubble/bubble_dialog_delegate.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:22: #include "ui/views/widget/widget.h" nit: needed? https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:28: class GestureEventForTest : public ui::GestureEvent { optional nit: TestGestureEvent? or make a factory function: ui::GestureEvent CreateGestureEvent(...) { return ui::GestureEvent(...); } (also drop the x and y params if they're always 0, 0) https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:31: : GestureEvent(x, y, 0, base::TimeTicks(), details) {} nit: s/0/ui::EF_NONE/ https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:37: class AppListButtonTest : public AshTestBase { optional nit: simplify this with something like: AppListButton* GetPrimaryAppListButton() { return AshTestBase::GetPrimaryShelf()->GetShelfViewForTesting()->GetAppListButton(); } void SendGestureEvent(ui::EventHandler* handler, ui::GestureEvent* event) { handler->OnGestureEvent(event); } using AppListButtonTest = test::AshTestBase; https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:65: OnGestureEvent(&long_press); nit: call RunAllPendingInMessageLoop(); after this for parity with below. https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.cc:41: if (presenter_) { nit: drop curlies for consistency with others, eg. ToggleAppList https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/t... File ui/app_list/presenter/test/test_app_list_presenter.h (right): https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/t... ui/app_list/presenter/test/test_app_list_presenter.h:33: size_t voice_interaction_session_count() const { optional nit: shorten with voice_count_ / voice_count().
PTAL https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:137: if (IsVoiceInteractionEnabled()) { On 2017/03/29 22:17:41, msw wrote: > Should this also have an else case to invoke ImageButton::OnGestureEvent? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:58: friend class test::AppListButtonTest; On 2017/03/29 22:17:41, msw wrote: > Avoid this if possible; see my SendGestureEvent suggestion in the test file, or > use |static_cast<ui::EventHandler>(app_list_button_)->OnGestureEvent(event);|, > or make the OnGestureEvent (and other overrides?) above public and > regroup/reorder the decls and definitions to match the base class(es). Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button_unittest.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:9: #include "ash/common/test/test_shelf_item_delegate.h" On 2017/03/29 22:17:41, msw wrote: > nit: needed? now you found out where I copy/pasted all these test code. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:10: #include "ash/common/wm_shell.h" On 2017/03/29 22:17:41, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:13: #include "ash/test/shelf_view_test_api.h" On 2017/03/29 22:17:41, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:15: #include "base/memory/ptr_util.h" On 2017/03/29 22:17:42, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:20: #include "ui/events/test/event_generator.h" On 2017/03/29 22:17:42, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:21: #include "ui/views/bubble/bubble_dialog_delegate.h" On 2017/03/29 22:17:41, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:22: #include "ui/views/widget/widget.h" On 2017/03/29 22:17:42, msw wrote: > nit: needed? Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:28: class GestureEventForTest : public ui::GestureEvent { On 2017/03/29 22:17:41, msw wrote: > optional nit: TestGestureEvent? or make a factory function: > ui::GestureEvent CreateGestureEvent(...) { return ui::GestureEvent(...); } > (also drop the x and y params if they're always 0, 0) Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:31: : GestureEvent(x, y, 0, base::TimeTicks(), details) {} On 2017/03/29 22:17:42, msw wrote: > nit: s/0/ui::EF_NONE/ Done. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:37: class AppListButtonTest : public AshTestBase { On 2017/03/29 22:17:41, msw wrote: > optional nit: simplify this with something like: > > AppListButton* GetPrimaryAppListButton() { > return > AshTestBase::GetPrimaryShelf()->GetShelfViewForTesting()->GetAppListButton(); > } > > void SendGestureEvent(ui::EventHandler* handler, ui::GestureEvent* event) { > handler->OnGestureEvent(event); > } > > using AppListButtonTest = test::AshTestBase; Acknowledged. https://codereview.chromium.org/2780053002/diff/40001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:65: OnGestureEvent(&long_press); On 2017/03/29 22:17:42, msw wrote: > nit: call RunAllPendingInMessageLoop(); after this for parity with below. Done. https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/a... File ui/app_list/presenter/app_list.cc (right): https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/a... ui/app_list/presenter/app_list.cc:41: if (presenter_) { On 2017/03/29 22:17:42, msw wrote: > nit: drop curlies for consistency with others, eg. ToggleAppList Done. https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/t... File ui/app_list/presenter/test/test_app_list_presenter.h (right): https://codereview.chromium.org/2780053002/diff/40001/ui/app_list/presenter/t... ui/app_list/presenter/test/test_app_list_presenter.h:33: size_t voice_interaction_session_count() const { On 2017/03/29 22:17:42, msw wrote: > optional nit: shorten with voice_count_ / voice_count(). Done.
I suggest you take the easy road via "// views::ImageButton overrides:" https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:145: } oops; this case should have a break; (perhaps that's why it didn't have a separate call to ImageButton::OnGestureEvent before, sorry I missed that) https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:36: // ui::EventHandler overrides: I know I mentioned this as a possible approach, but it's probably more work than worthwhile to do this correctly (if you reorder declarations here, you should reorder the definitions to match in the cc file). Feel free to list all these as views::ImageButton overrides, just move OnGestureEvent public up into the public section with a separate "// views::ImageButton overrides:" comment, and otherwise leave the order and comments alone in this CL. Breaking these out to the specific base classes (that originally define each function) doesn't really help users and causes lots of churn in the long run. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:40: std::unique_ptr<views::InkDrop> CreateInkDrop() override; nit: this is actually from views::InkDropHost, if you're trying to get specific. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:42: std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override; nit: this is actually from views::InkDropHost, if you're trying to get specific. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:44: // views::CustomButton overrides: nit: these are actually from views::View, if you're trying to get specific. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:48: bool OnMouseDragged(const ui::MouseEvent& event) override; nit: OnMouseDragged belongs above OnMouseReleased, if you're reordering these. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:50: // views::ImageButton overrides: nit: these are both from views::View, merge with the list above in the correct order if you're making ordering changes here. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:57: void NotifyClick(const ui::Event& event) override; nit: this is actually from views::Button, if you're trying to get specific. https://codereview.chromium.org/2780053002/diff/60001/ui/app_list/presenter/t... File ui/app_list/presenter/test/test_app_list_presenter.h (right): https://codereview.chromium.org/2780053002/diff/60001/ui/app_list/presenter/t... ui/app_list/presenter/test/test_app_list_presenter.h:33: size_t voice_count() const { return voice_count_; } very optional nit: |voice_session_count| (sorry for my bike-shedding here)
https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.cc (right): https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.cc:145: } On 2017/03/30 01:07:05, msw wrote: > oops; this case should have a break; (perhaps that's why it didn't have a > separate call to ImageButton::OnGestureEvent before, sorry I missed that) Done. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button.h (right): https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:36: // ui::EventHandler overrides: On 2017/03/30 01:07:05, msw wrote: > I know I mentioned this as a possible approach, but it's probably more work than > worthwhile to do this correctly (if you reorder declarations here, you should > reorder the definitions to match in the cc file). Feel free to list all these as > views::ImageButton overrides, just move OnGestureEvent public up into the public > section with a separate "// views::ImageButton overrides:" comment, and > otherwise leave the order and comments alone in this CL. Breaking these out to > the specific base classes (that originally define each function) doesn't really > help users and causes lots of churn in the long run. Done. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:40: std::unique_ptr<views::InkDrop> CreateInkDrop() override; On 2017/03/30 01:07:05, msw wrote: > nit: this is actually from views::InkDropHost, if you're trying to get specific. Acknowledged. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:42: std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override; On 2017/03/30 01:07:05, msw wrote: > nit: this is actually from views::InkDropHost, if you're trying to get specific. Acknowledged. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:44: // views::CustomButton overrides: On 2017/03/30 01:07:05, msw wrote: > nit: these are actually from views::View, if you're trying to get specific. Acknowledged. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:48: bool OnMouseDragged(const ui::MouseEvent& event) override; On 2017/03/30 01:07:05, msw wrote: > nit: OnMouseDragged belongs above OnMouseReleased, if you're reordering these. Acknowledged. https://codereview.chromium.org/2780053002/diff/60001/ash/common/shelf/app_li... ash/common/shelf/app_list_button.h:57: void NotifyClick(const ui::Event& event) override; On 2017/03/30 01:07:05, msw wrote: > nit: this is actually from views::Button, if you're trying to get specific. Acknowledged. https://codereview.chromium.org/2780053002/diff/60001/ui/app_list/presenter/t... File ui/app_list/presenter/test/test_app_list_presenter.h (right): https://codereview.chromium.org/2780053002/diff/60001/ui/app_list/presenter/t... ui/app_list/presenter/test/test_app_list_presenter.h:33: size_t voice_count() const { return voice_count_; } On 2017/03/30 01:07:09, msw wrote: > very optional nit: |voice_session_count| (sorry for my bike-shedding here) Done.
lgtm, thanks for putting up with my comments!
Thank you for the quick and detailed reviews.
The CQ bit was checked by xiaohuic@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...
xiyuan@chromium.org changed reviewers: + tsepez@chromium.org
lgtm with nits tsepez@, could you review the mojom changes? https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button_unittest.cc (right): https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:35: void OnGestureEvent(ui::GestureEvent* event) { nit: OnGestureEvent -> SimulateGestureEvent or SendGestureEvent https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:40: AppListButton* app_list_button_; private: ? https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:46: TEST_F(AppListButtonTest, LongPressGestureWithoutFlag) { nit: LongPressGestureWithoutFlag -> LongPressGestureWithout*VoiceInteraction*Flag https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:57: TEST_F(AppListButtonTest, LongPressGestureWithFlag) { nit: LongPressGestureWithFlag -> LongPressGestureWith*VoiceInteraction*Flag
what's a voice interaction session? Can an untrusted client start one without making the end user aware that this is happening? Thanks.
https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... File ash/common/shelf/app_list_button_unittest.cc (right): https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:35: void OnGestureEvent(ui::GestureEvent* event) { On 2017/03/30 18:14:24, xiyuan wrote: > nit: OnGestureEvent -> SimulateGestureEvent or SendGestureEvent Done. https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:40: AppListButton* app_list_button_; On 2017/03/30 18:14:24, xiyuan wrote: > private: ? Done. https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:46: TEST_F(AppListButtonTest, LongPressGestureWithoutFlag) { On 2017/03/30 18:14:24, xiyuan wrote: > nit: LongPressGestureWithoutFlag -> > LongPressGestureWithout*VoiceInteraction*Flag Done. https://codereview.chromium.org/2780053002/diff/80001/ash/common/shelf/app_li... ash/common/shelf/app_list_button_unittest.cc:57: TEST_F(AppListButtonTest, LongPressGestureWithFlag) { On 2017/03/30 18:14:24, xiyuan wrote: > nit: LongPressGestureWithFlag -> LongPressGestureWith*VoiceInteraction*Flag Done.
LGTM per offline discussion.
The CQ bit was checked by xiaohuic@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaohuic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2780053002/#ps100001 (title: "fix comments")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaohuic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, msw@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2780053002/#ps120001 (title: "missed updating ExampleAppListPresenter")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaohuic@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 commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
xiaohuic@chromium.org changed reviewers: + oshima@chromium.org
Add oshima@ for owners review on ash/shell/example_app_list_presenter.*
xiaohuic@chromium.org changed reviewers: + derat@chromium.org
Saw oshima@ OOO this pm, also adding derat@ for owners review on ash/shell/example_app_list_presenter.*
lgtm for //ash/shell
The CQ bit was checked by xiaohuic@google.com
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": 120001, "attempt_start_ts": 1490995755569250, "parent_rev": "7e883e151b6e43336ff4252a14212fe61d2605f8", "commit_rev": "dad576a51b2a29433e8c86fd29b092d32559c628"}
Message was sent while issue was closed.
Description was changed from ========== Add app list button long press action TEST=locally on device, try long press and see the action handled BUG=b:36522311 ========== to ========== Add app list button long press action TEST=locally on device, try long press and see the action handled BUG=b:36522311 Review-Url: https://codereview.chromium.org/2780053002 Cr-Commit-Position: refs/heads/master@{#461285} Committed: https://chromium.googlesource.com/chromium/src/+/dad576a51b2a29433e8c86fd29b0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/dad576a51b2a29433e8c86fd29b0... |