Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898743002:
Draggable peeking/fullscreen launcher with transparent background. (Closed)
DescriptionDraggable peeking/fullscreen launcher with transparent background.
UI tests are not yet implemented, waiting on final behavior from UX.
Enable the new launcher with --enable-features=EnableFullscreenAppList
BUG=721781
Review-Url: https://codereview.chromium.org/2898743002
Cr-Original-Commit-Position: refs/heads/master@{#478372}
Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f3a8fa64dabce
Review-Url: https://codereview.chromium.org/2898743002
Cr-Commit-Position: refs/heads/master@{#478665}
Committed: https://chromium.googlesource.com/chromium/src/+/2febb9832367123799f3e553274c4449f91a5634
Patch Set 1 #
Total comments: 123
Patch Set 2 : Migrated from wm_shelf to shelf and addressed comments! #
Total comments: 52
Patch Set 3 : adressed comments. #
Total comments: 54
Patch Set 4 : manually resolve merge conflict. #
Total comments: 2
Patch Set 5 : Reverted changes to all_apps_tile_item_view.cc to fix merge conflicts. #Patch Set 6 : Restored changes to all_apps_tile_item_view.cc #Patch Set 7 : Rebased properly. #Patch Set 8 : Addressed comments. #
Total comments: 18
Patch Set 9 : Rebased to fix merge conflicts and addressed comments. #
Total comments: 24
Patch Set 10 : Rebased to resolve merge conflicts, Refactored to use observers, addressed comments, and disabled a… #
Total comments: 5
Patch Set 11 : removed app_list_delegate_impl and made shell an AppListDelegate #
Total comments: 14
Patch Set 12 : Addressed comments. #
Total comments: 92
Patch Set 13 : Addressed comments. #Patch Set 14 : addressed msw@'s comments. #
Total comments: 46
Patch Set 15 : addressed msw@'s comments. #
Total comments: 24
Patch Set 16 : addressed msw@'s comments. #
Total comments: 4
Patch Set 17 : Reverted Mash/Ash code path merge. #Patch Set 18 : Rebased and reverted mash/ash code paths. #Patch Set 19 : Fixed case where Esc key was not closing app list. #
Total comments: 2
Patch Set 20 : Added todo for deletion of app_list_delegate_impl.cc/h #Patch Set 21 : Removed header include in shell.h that was causing gn check failure. #Messages
Total messages: 177 (128 generated)
The CQ bit was checked by newcomer@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...
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent black background. supports orientation changes. UI tests are not yet implemented, waiting on animation curves. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 patch from issue 2876603006 at patchset 20001 (http://crrev.com/2876603006#ps20001) built on https://codereview.chromium.org/2802903003/ ========== to ========== Draggable peeking/fullscreen launcher with transparent black background. supports orientation changes. UI tests are not yet implemented, waiting on animation curves. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 patch from issue 2876603006 at patchset 20001 (http://crrev.com/2876603006#ps20001) built on https://codereview.chromium.org/2802903003/ ==========
newcomer@chromium.org changed reviewers: + sky@chromium.org, vadimt@chromium.org, xiyuan@chromium.org
Instructions to enable the changes are in the CL description. -Alex
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent black background. supports orientation changes. UI tests are not yet implemented, waiting on animation curves. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 patch from issue 2876603006 at patchset 20001 (http://crrev.com/2876603006#ps20001) built on https://codereview.chromium.org/2802903003/ ========== to ========== Draggable peeking/fullscreen launcher with transparent black background. supports orientation changes. UI tests are not yet implemented, waiting on animation curves. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 ==========
https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:154: shelf->AppListIsActive(false); Just asking: isn't OnShown a counterpart of OnDismissed, and should we move the AppListIsActive(true) to OnShown? https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:197: //EnableFullscreenAppList(); Remove/uncomment? https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:65: void AppListIsActive(bool active); Also: rename to OnAppListIsActive. You don't have to write things like "to prevent XXX" since the purpose can change in future. https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:319: bool app_list_is_active_ = false; Is there a reasonable way to find out from here whether launcher is active without introducing a variable? https://codereview.chromium.org/2898743002/diff/1/ash/shelf/wm_shelf.h File ash/shelf/wm_shelf.h (right): https://codereview.chromium.org/2898743002/diff/1/ash/shelf/wm_shelf.h#newcod... ash/shelf/wm_shelf.h:102: void AppListIsActive(bool hideBackground); Should be called something like SetBackgroundOpacity? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:22: } // namespace fullscreen_constants Empty line after? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... File ui/app_list/app_list_switches.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.h:37: //bool APP_LIST_EXPORT IsFullscreenApplistEnabled(); Remove? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... File ui/app_list/views/all_apps_tile_item_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... ui/app_list/views/all_apps_tile_item_view.cc:70: if(features::IsFullscreenAppListEnabled()){ Space before {? Git cl format? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... ui/app_list/views/all_apps_tile_item_view.cc:72: (AppListView*)GetAncestorWithClassName("AppListView"); Please try properly plumbing AppListView as opposed to use non-type-safe approaches. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:59: int kShelfSize = 48; Comment pls. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:62: int kPeekingLauncherHeight = 320; Remove 'used in'. Leave the meaning. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:66: // int kHalfLauncherHeight = 561; I see. Please don't leave commented blocks like this. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:187: initial_drag_separation_(0), Use initialization in header. type var = val; https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:437: ComputeConstants(); Sounds strange... UpdateDimensions? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:464: 3; Should have a constant for '3'? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:466: display::Screen::GetScreen()->AddObserver(this); This line is paired with removing observer in destructor, but it's not clear from the code, as this like isn't seemingly protected by 'IsFullscreen' condition. Could you do something to make it clearer? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:526: if (event->type() == ui::ET_MOUSE_PRESSED || I'd prefer switch statement. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:552: // Calculate the new position of the launcher which may or may not be used. Use {}, as the statement in 'if' takes more than 1 line. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:553: new_y_position = 'else' it will be uninitialized? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:574: fullscreen_widget_bounds_.set_y(new_y_position); I assume you manually reproed cases when you need to go at all branches in the new code? Also going to add tests for full coverage? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:577: return; Please use break as you don't use return as in early return pattern. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:586: if (std::abs(last_fling_velocity_) > 25) { Constant for 25? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:589: Remove empty line. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:593: } else if (!is_fullscreen_launcher_) { No need to check !is_fullscreen_launcher_ https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:637: This method is over 100 lines; please split. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:698: app_list_main_view_->contents_view()->start_page_view()->parent()->Layout(); I'd explicitly plumb that view to here. Hierarchy may change. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:713: launcher_threshold_ = (fullscreen_widget_bounds_.height() + kShelfSize) / 3; 3 again https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:720: // height is wrong Need a better comment. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:726: bool AppListView::IsFullscreen() { Should be const method https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:92: void ComputeConstants(); Please don't place it between views::View overrides. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:101: bool IsFullscreen(); I assume there are 2 states Fullscreen and peeking. Then we need to have a matching SetFullScreen(bool), not ToFullScreen/ToPeeking pair. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:129: void OnMouseEvent(ui::MouseEvent* event) override; You don't need comments for overrides. BTW are they overrides for views::WidgetDelegateView? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:135: void HandleDrag(gfx::Point location, ui::EventType type); Is this override? If so, add override keyword; else, move. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:158: views::View* launcher_background_shield_; // Owned by the app list's widget. I assume all added fields can't be made const fields? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:160: int initial_drag_separation_; I know that other fields don't have comments, but it's hard to deduce the meaning from the name. Please add comments for all added fields. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:70: features::IsFullscreenAppListEnabled() Is Paint() called frequently enough to start worrying about the price of features::IsFullscreenAppListEnabled()? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:125: AppListViewDelegate* view_delegate ) git cl format . to remove added space https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:134: app_list_view_(), This leaves it uninitialized. Replace with '=nullptr' in header. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.h:126: app_list::AppListView* app_list_view_; How owned? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:46: int kStartPageSearchBoxWidth = 480; constexpr? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:164: if (features::IsFullscreenAppListEnabled()) Move condition into param calculation as a ternary expression. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:261: if (features::IsFullscreenAppListEnabled() && i == 5) { Const for 5. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:280: tiles_layout_manager->SkipColumns(3); ... and 3
https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:195: if(!app_list::features::IsFullscreenAppListEnabled()) If the test case does not run for non-fullscreen, how about change TEST_P -> TEST_*F* and explicitly call EnableFullscreenAppList() as the test below. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:219: // Grab the bounds of the search box, nit: insert an empty line before https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:220: // which is guarunteed to be inside the launcher. nit: guarunteed -> guaranteed https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:227: // Tapping inside the bounds doesn't close the launcher. nit: insert an empty line before. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:230: // Clicking inside the bounds doesn't close the launcher. nit: insert an empty line before. https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:319: bool app_list_is_active_ = false; On 2017/05/22 23:36:56, vadimt wrote: > Is there a reasonable way to find out from here whether launcher is active > without introducing a variable? Think we can use Shell::Get()->app_list()->IsVisible() (or GetTargetVisibility()). https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.cc (left): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.cc:6: nit: please restore this. Prevailing code style is to have an empty line between different seconds of includes. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:21: nit: get rid of this empty line (or insert one before line 20) to make it symmetric around opening and closing of namespace https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... File ui/app_list/app_list_switches.cc (left): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.cc:6: nit: restore https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.cc:75: nit: restore https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_main_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_main_view.cc:57: SetPaintToLayer(); Remove this. Or remove the call in AppListView::InitContents. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:202: display::Screen::GetScreen()->RemoveObserver(this); Suggest to use ScopedObserver so that this happens automatically. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:525: if (features::IsFullscreenAppListEnabled()) { nit: reverse condition and bail out early. i.e. if (!features::IsFullscreenAppListEnabled()) return; ... https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:536: if (features::IsFullscreenAppListEnabled()) { ditto https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:158: views::View* launcher_background_shield_; // Owned by the app list's widget. Initialize it to nullptr, i.e. views::View* launcher_background_shield_ = nullptr So that we can use |launcher_background_shield_| in Layout etc instead of checking fullscreen feature everywhere. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:261: if (features::IsFullscreenAppListEnabled() && i == 5) { Make |apps_num| 5 somehow (either here or at call site CreateAppsGrid) of when fullscreen feature is enabled.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by newcomer@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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 newcomer@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: 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 newcomer@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...
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
Had to migrate some code from ash/shelf/wm_shelf.cc/h to ash/shelf/shelf.cc/h https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:154: shelf->AppListIsActive(false); On 2017/05/22 23:36:56, vadimt wrote: > Just asking: isn't OnShown a counterpart of OnDismissed, and should we move the > AppListIsActive(true) to OnShown? You're correct! I put it in OnShown. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:195: if(!app_list::features::IsFullscreenAppListEnabled()) On 2017/05/24 18:28:17, xiyuan wrote: > If the test case does not run for non-fullscreen, how about change TEST_P -> > TEST_*F* and explicitly call EnableFullscreenAppList() as the test below. Done! I was experimenting with tests and left that in. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:197: //EnableFullscreenAppList(); On 2017/05/22 23:36:56, vadimt wrote: > Remove/uncomment? Done. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:219: // Grab the bounds of the search box, On 2017/05/24 18:28:17, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:220: // which is guarunteed to be inside the launcher. On 2017/05/24 18:28:17, xiyuan wrote: > nit: guarunteed -> guaranteed Done. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:227: // Tapping inside the bounds doesn't close the launcher. On 2017/05/24 18:28:17, xiyuan wrote: > nit: insert an empty line before. Done. https://codereview.chromium.org/2898743002/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:230: // Clicking inside the bounds doesn't close the launcher. On 2017/05/24 18:28:17, xiyuan wrote: > nit: insert an empty line before. Done. https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:65: void AppListIsActive(bool active); On 2017/05/22 23:36:56, vadimt wrote: > Also: rename to OnAppListIsActive. > You don't have to write things like "to prevent XXX" since the purpose can > change in future. Done. https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:319: bool app_list_is_active_ = false; AppListIsActive (now OnAppListIsActive) actually forces the shelf to hide it's transparent background. I think it is more efficient to make a call from the presenter delegate every time the applist is created(and set app_list_is_active_) instead of polling for the applist every time GetShelfBackgroundType is called. I'd be happy to hear arguments against this! https://codereview.chromium.org/2898743002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager.h:319: bool app_list_is_active_ = false; On 2017/05/24 18:28:17, xiyuan wrote: > On 2017/05/22 23:36:56, vadimt wrote: > > Is there a reasonable way to find out from here whether launcher is active > > without introducing a variable? > > Think we can use Shell::Get()->app_list()->IsVisible() (or > GetTargetVisibility()). Acknowledged. https://codereview.chromium.org/2898743002/diff/1/ash/shelf/wm_shelf.h File ash/shelf/wm_shelf.h (right): https://codereview.chromium.org/2898743002/diff/1/ash/shelf/wm_shelf.h#newcod... ash/shelf/wm_shelf.h:102: void AppListIsActive(bool hideBackground); On 2017/05/22 23:36:56, vadimt wrote: > Should be called something like SetBackgroundOpacity? It just lets the ShelfLayoutManager know that the applist is active in order to restrict the background behavior (so it doesn't strictly speaking set the opacity). Updated comment! LMK if you still think it should be renamed. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:21: On 2017/05/24 18:28:17, xiyuan wrote: > nit: get rid of this empty line (or insert one before line 20) to make it > symmetric around opening and closing of namespace Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.h:22: } // namespace fullscreen_constants On 2017/05/22 23:36:56, vadimt wrote: > Empty line after? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... File ui/app_list/app_list_switches.cc (left): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.cc:6: On 2017/05/24 18:28:17, xiyuan wrote: > nit: restore Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.cc:75: On 2017/05/24 18:28:18, xiyuan wrote: > nit: restore Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... File ui/app_list/app_list_switches.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_switch... ui/app_list/app_list_switches.h:37: //bool APP_LIST_EXPORT IsFullscreenApplistEnabled(); On 2017/05/22 23:36:56, vadimt wrote: > Remove? Done! ty. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... File ui/app_list/views/all_apps_tile_item_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... ui/app_list/views/all_apps_tile_item_view.cc:70: if(features::IsFullscreenAppListEnabled()){ On 2017/05/22 23:36:57, vadimt wrote: > Space before {? Git cl format? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/all_apps_... ui/app_list/views/all_apps_tile_item_view.cc:72: (AppListView*)GetAncestorWithClassName("AppListView"); On 2017/05/22 23:36:57, vadimt wrote: > Please try properly plumbing AppListView as opposed to use non-type-safe > approaches. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_main_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_main_view.cc:57: SetPaintToLayer(); On 2017/05/24 18:28:18, xiyuan wrote: > Remove this. Or remove the call in AppListView::InitContents. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:59: int kShelfSize = 48; On 2017/05/22 23:36:58, vadimt wrote: > Comment pls. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:62: int kPeekingLauncherHeight = 320; On 2017/05/22 23:36:57, vadimt wrote: > Remove 'used in'. Leave the meaning. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:66: // int kHalfLauncherHeight = 561; On 2017/05/22 23:36:58, vadimt wrote: > I see. Please don't leave commented blocks like this. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:187: initial_drag_separation_(0), On 2017/05/22 23:36:57, vadimt wrote: > Use initialization in header. > type var = val; Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:202: display::Screen::GetScreen()->RemoveObserver(this); On 2017/05/24 18:28:18, xiyuan wrote: > Suggest to use ScopedObserver so that this happens automatically. Cool! and done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:437: ComputeConstants(); On 2017/05/22 23:36:57, vadimt wrote: > Sounds strange... UpdateDimensions? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:464: 3; On 2017/05/22 23:36:57, vadimt wrote: > Should have a constant for '3'? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:466: display::Screen::GetScreen()->AddObserver(this); On 2017/05/22 23:36:57, vadimt wrote: > This line is paired with removing observer in destructor, but it's not clear > from the code, as this like isn't seemingly protected by 'IsFullscreen' > condition. > Could you do something to make it clearer? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:525: if (features::IsFullscreenAppListEnabled()) { On 2017/05/24 18:28:18, xiyuan wrote: > nit: reverse condition and bail out early. > > i.e. > > if (!features::IsFullscreenAppListEnabled()) > return; > > ... Done, also swapped if(features::IsFullscreenAppListEnabled()) for if (launcher_background_shield_) because this gets called a lot. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:526: if (event->type() == ui::ET_MOUSE_PRESSED || On 2017/05/22 23:36:57, vadimt wrote: > I'd prefer switch statement. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:536: if (features::IsFullscreenAppListEnabled()) { On 2017/05/24 18:28:18, xiyuan wrote: > ditto Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:552: // Calculate the new position of the launcher which may or may not be used. On 2017/05/22 23:36:57, vadimt wrote: > Use {}, as the statement in 'if' takes more than 1 line. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:553: new_y_position = On 2017/05/22 23:36:57, vadimt wrote: > 'else' it will be uninitialized? Moved the initialization into the switch statement. The variable isn't used for ET_GESTURE_SCROLL_BEGIN or ET_MOUSE_PRESSED. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:574: fullscreen_widget_bounds_.set_y(new_y_position); On 2017/05/22 23:36:57, vadimt wrote: > I assume you manually reproed cases when you need to go at all branches in the > new code? Also going to add tests for full coverage? Correct! The next CL will have full test coverage because it also includes start_page_view state changes and the half launcher. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:577: return; On 2017/05/22 23:36:57, vadimt wrote: > Please use break as you don't use return as in early return pattern. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:586: if (std::abs(last_fling_velocity_) > 25) { On 2017/05/22 23:36:57, vadimt wrote: > Constant for 25? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:589: On 2017/05/22 23:36:57, vadimt wrote: > Remove empty line. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:593: } else if (!is_fullscreen_launcher_) { On 2017/05/22 23:36:57, vadimt wrote: > No need to check !is_fullscreen_launcher_ Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:637: On 2017/05/22 23:36:57, vadimt wrote: > This method is over 100 lines; please split. It is now 81 lines. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:698: app_list_main_view_->contents_view()->start_page_view()->parent()->Layout(); On 2017/05/22 23:36:57, vadimt wrote: > I'd explicitly plumb that view to here. Hierarchy may change. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:713: launcher_threshold_ = (fullscreen_widget_bounds_.height() + kShelfSize) / 3; On 2017/05/22 23:36:57, vadimt wrote: > 3 again Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:720: // height is wrong On 2017/05/22 23:36:57, vadimt wrote: > Need a better comment. That was a prototyping comment. Removed! https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:726: bool AppListView::IsFullscreen() { On 2017/05/22 23:36:57, vadimt wrote: > Should be const method Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:92: void ComputeConstants(); On 2017/05/22 23:36:58, vadimt wrote: > Please don't place it between views::View overrides. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:101: bool IsFullscreen(); On 2017/05/22 23:36:58, vadimt wrote: > I assume there are 2 states Fullscreen and peeking. Then we need to have a > matching SetFullScreen(bool), not ToFullScreen/ToPeeking pair. In the next CL there will be five states (INITIAL, PEEKING, HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH). Can I push this to the next CL? It will have to get refactored in the next CL anyways. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:129: void OnMouseEvent(ui::MouseEvent* event) override; On 2017/05/22 23:36:58, vadimt wrote: > You don't need comments for overrides. BTW are they overrides for > views::WidgetDelegateView? They are ui::EventHandler overrides. Done! https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:135: void HandleDrag(gfx::Point location, ui::EventType type); On 2017/05/22 23:36:58, vadimt wrote: > Is this override? If so, add override keyword; else, move. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:158: views::View* launcher_background_shield_; // Owned by the app list's widget. On 2017/05/22 23:36:58, vadimt wrote: > I assume all added fields can't be made const fields? Correct. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:158: views::View* launcher_background_shield_; // Owned by the app list's widget. On 2017/05/24 18:28:18, xiyuan wrote: > Initialize it to nullptr, > > i.e. > views::View* launcher_background_shield_ = nullptr > > So that we can use |launcher_background_shield_| in Layout etc instead of > checking fullscreen feature everywhere. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:160: int initial_drag_separation_; On 2017/05/22 23:36:58, vadimt wrote: > I know that other fields don't have comments, but it's hard to deduce the > meaning from the name. Please add comments for all added fields. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:70: features::IsFullscreenAppListEnabled() On 2017/05/22 23:36:58, vadimt wrote: > Is Paint() called frequently enough to start worrying about the price of > features::IsFullscreenAppListEnabled()? Good question. I'm not sure but I'll ask around. In the meantime I've moved it to a member variable that is checked on initialization. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:125: AppListViewDelegate* view_delegate ) On 2017/05/22 23:36:58, vadimt wrote: > git cl format . to remove added space Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:134: app_list_view_(), On 2017/05/22 23:36:58, vadimt wrote: > This leaves it uninitialized. Replace with '=nullptr' in header. The other pointers are set to NULL in the constructor, is this fine? https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.h:126: app_list::AppListView* app_list_view_; On 2017/05/22 23:36:58, vadimt wrote: > How owned? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:46: int kStartPageSearchBoxWidth = 480; On 2017/05/22 23:36:58, vadimt wrote: > constexpr? Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:164: if (features::IsFullscreenAppListEnabled()) On 2017/05/22 23:36:58, vadimt wrote: > Move condition into param calculation as a ternary expression. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:261: if (features::IsFullscreenAppListEnabled() && i == 5) { On 2017/05/24 18:28:18, xiyuan wrote: > Make |apps_num| 5 somehow (either here or at call site CreateAppsGrid) of when > fullscreen feature is enabled. Done! I think I left this in by accident while experimenting. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:261: if (features::IsFullscreenAppListEnabled() && i == 5) { On 2017/05/24 18:28:18, xiyuan wrote: > Make |apps_num| 5 somehow (either here or at call site CreateAppsGrid) of when > fullscreen feature is enabled. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:280: tiles_layout_manager->SkipColumns(3); On 2017/05/22 23:36:58, vadimt wrote: > ... and 3 Done.
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/2898743002/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:101: bool IsFullscreen(); On 2017/05/25 23:10:53, newcomer wrote: > On 2017/05/22 23:36:58, vadimt wrote: > > I assume there are 2 states Fullscreen and peeking. Then we need to have a > > matching SetFullScreen(bool), not ToFullScreen/ToPeeking pair. > > In the next CL there will be five states (INITIAL, PEEKING, HALF, > FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH). Can I push this to the next CL? It > will have to get refactored in the next CL anyways. Acknowledged. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:70: features::IsFullscreenAppListEnabled() And make sure that a SearchBoxBackground instance is never shared by 2 consequential parameterized tests (with different values of IsFullscreenAppListEnabled()). https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:134: app_list_view_(), Just old code. nullptr is a new way, and is my preference. https://codereview.chromium.org/2898743002/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:135: Shelf* shelf = Shelf::ForWindow(root_window); You can move this inside if. https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:138: // Notifies ShelfLayoutManager that the app list is active in order to You either call it NotifyXXX and keep the comment or leave the name as OnXXX, and change the comment to "called when XXX". https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.h:63: // Called from wm_shelf. Called when, not whence. Or change the name to a verb, and comment what it does. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/all_a... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/all_a... ui/app_list/views/all_apps_tile_item_view.h:32: AppListView* app_list_view_; AppListView* const app_list_view_; Also a comment on whether it's owned or not by this class. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: int kShelfSize = 48; I hope there is no constant or literal with the same value and purpose elsewhere? https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: int kLauncherThresholdDenominator = 3; Comments. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:66: int kLauncherDragVelocityThreshold = 25; Comment; it should make it clear what the units are. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:187: fullscreen_widget_bounds_(), No need for default constructor here. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:198: // display::Screen::GetScreen()->AddObserver(this); Remove https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:205: // display::Screen::GetScreen()->RemoveObserver(this); Remove https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:362: launcher_background_shield_->layer()->SetOpacity(0.8f); Constant. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:493: int new_y_position; Make a constant variable in each switch branches, move from here. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:532: ToPeeking(); {}, as this is a complex if https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:534: if (last_fling_velocity_ > 0) { No need for placing 'else' contents in {}, unless you have higher reasons. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:627: switch (event->type()) { Empty line before. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:629: last_fling_velocity_ = event->details().velocity_y(); break; If this is what you wanted, this don't use the not-using-break pattern. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:96: void UpdateDimensions(); Group with the rest non-overrides; sort same way in .cc. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:152: views::Widget* fullscreen_widget_; Unowned? And a comment. = nullptr https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:168: int launcher_threshold_; Initialize. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:205: AppListView* app_list_view_; // Owned by the views hierarchy. const Comment in a separate line. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:77: bool is_fullscreen_enabled_; const https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:173: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); Does it compile? I don't see a declaration of is_fullscreen_enabled_. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:57: constexpr int kCenterColumnOfStartPageAppGrid = 3; Can it be calculated from other constants? Then you don't need a separate constant for it. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:267: // if (features::IsFullscreenAppListEnabled() && i == 5) { Remove.
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by newcomer@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...
Addressed comments! https://codereview.chromium.org/2898743002/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:135: Shelf* shelf = Shelf::ForWindow(root_window); On 2017/05/26 01:27:56, vadimt wrote: > You can move this inside if. Done. https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:138: // Notifies ShelfLayoutManager that the app list is active in order to On 2017/05/26 01:27:56, vadimt wrote: > You either call it NotifyXXX and keep the comment or leave the name as OnXXX, > and change the comment to "called when XXX". Done. https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/80001/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.h:63: // Called from wm_shelf. On 2017/05/26 01:27:56, vadimt wrote: > Called when, not whence. > Or change the name to a verb, and comment what it does. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/all_a... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/all_a... ui/app_list/views/all_apps_tile_item_view.h:32: AppListView* app_list_view_; On 2017/05/26 01:27:56, vadimt wrote: > AppListView* const app_list_view_; > > Also a comment on whether it's owned or not by this class. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: int kShelfSize = 48; On 2017/05/26 01:27:57, vadimt wrote: > I hope there is no constant or literal with the same value and purpose > elsewhere? There is, but it is in Ash which we cannot access (UI is below Ash in the dependency graph.) I talked to james about eventually migrating app_list to ash, which I would like to do once we make 61. This is a temporary workaround. It uses the same name as Ash/shelf/shelf_constants. I could put it in the fullscreen_constants namespace or rename it? https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: int kLauncherThresholdDenominator = 3; On 2017/05/26 01:27:57, vadimt wrote: > Comments. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:66: int kLauncherDragVelocityThreshold = 25; On 2017/05/26 01:27:57, vadimt wrote: > Comment; it should make it clear what the units are. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:187: fullscreen_widget_bounds_(), On 2017/05/26 01:27:57, vadimt wrote: > No need for default constructor here. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:198: // display::Screen::GetScreen()->AddObserver(this); On 2017/05/26 01:27:57, vadimt wrote: > Remove Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:205: // display::Screen::GetScreen()->RemoveObserver(this); On 2017/05/26 01:27:56, vadimt wrote: > Remove Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:362: launcher_background_shield_->layer()->SetOpacity(0.8f); On 2017/05/26 01:27:57, vadimt wrote: > Constant. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:493: int new_y_position; On 2017/05/26 01:27:57, vadimt wrote: > Make a constant variable in each switch branches, move from here. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:532: ToPeeking(); On 2017/05/26 01:27:57, vadimt wrote: > {}, as this is a complex if Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:534: if (last_fling_velocity_ > 0) { On 2017/05/26 01:27:57, vadimt wrote: > No need for placing 'else' contents in {}, unless you have higher reasons. I did it for clarity but I see how it isn't necessary. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:627: switch (event->type()) { On 2017/05/26 01:27:57, vadimt wrote: > Empty line before. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:629: last_fling_velocity_ = event->details().velocity_y(); On 2017/05/26 01:27:57, vadimt wrote: > break; > If this is what you wanted, this don't use the not-using-break pattern. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:96: void UpdateDimensions(); On 2017/05/26 01:27:57, vadimt wrote: > Group with the rest non-overrides; sort same way in .cc. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:152: views::Widget* fullscreen_widget_; On 2017/05/26 01:27:57, vadimt wrote: > Unowned? > And a comment. > = nullptr Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:168: int launcher_threshold_; On 2017/05/26 01:27:57, vadimt wrote: > Initialize. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/conte... ui/app_list/views/contents_view.h:205: AppListView* app_list_view_; // Owned by the views hierarchy. On 2017/05/26 01:27:57, vadimt wrote: > const > Comment in a separate line. Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:77: bool is_fullscreen_enabled_; On 2017/05/26 01:27:57, vadimt wrote: > const Done. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:173: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); On 2017/05/26 01:27:57, vadimt wrote: > Does it compile? I don't see a declaration of is_fullscreen_enabled_. Declared in header, line 127. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:57: constexpr int kCenterColumnOfStartPageAppGrid = 3; On 2017/05/26 01:27:57, vadimt wrote: > Can it be calculated from other constants? > Then you don't need a separate constant for it. I would have to divide by 2 (which I would need a constant for) and then round up a float to get 3. Keeping this constant makes more sense to me. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:267: // if (features::IsFullscreenAppListEnabled() && i == 5) { On 2017/05/26 01:27:57, vadimt wrote: > Remove. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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...)
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
fixed merge issue.
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/05/26 17:30:43, newcomer wrote: > fixed merge issue. Please don't delete patch set. Otherwise, reviewers will lose all the comments on the deleted PS.
The CQ bit was checked by newcomer@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...
Will do! I didn't know. On Fri, May 26, 2017 at 10:39 AM, <xiyuan@chromium.org> wrote: > On 2017/05/26 17:30:43, newcomer wrote: > > fixed merge issue. > > Please don't delete patch set. Otherwise, reviewers will lose all the > comments > on the deleted PS. > > https://codereview.chromium.org/2898743002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: int kShelfSize = 48; Nah, you can just leave this. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:173: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); const then https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:139: // behavior. Nit: for OnXXX methods, you don't need to specify why they are called. Only when. And rename param to something like is_active. Note: not isActive. https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.h:63: // Called when the AppList is active in order to restrict background behavior. Nit same. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:71: const float kLauncherOpacity = 0.8; Comment!!!!! https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:516: int new_y_position = location.y() - initial_drag_separation_ + Nit: this can be const. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:120: void HandleDrag(gfx::Point location, ui::EventType type); const gfx::Point& https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:176: ScopedObserver<display::Screen, display::DisplayObserver> display_observer_; Comments for all new fields (in .h files). Everywhere, and forever. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:170: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); const https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:207: : std::min<int>(kNumStartPageTiles, display_results.size())); It really doesn't work w/o <int>? https://codereview.chromium.org/2898743002/diff/150001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/150001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:382: search_box_view_->SetAppListView(this); Constructor parameter, const field.
The CQ bit was checked by newcomer@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: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2898743002/diff/70024/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:136: if (app_list::features::IsFullscreenAppListEnabled()) { Can you replicate the logic in AppListDelegateImpl::OnAppListVisibilityChanged as well so that the feature works for mash? You can try mash by adding "--mash" to chrome's command line. Ideally we should have a single code path but we are not there yet. :( https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:140: void OnAppListIsActive(bool hideBackground); hideBackground -> hide_background It is strange that the name looks irrelevant to applist active state. Should it be called something like |active|? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... File ui/app_list/app_list_constants.cc (left): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... ui/app_list/app_list_constants.cc:6: Please restore. There should be an empty line between include sections. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... File ui/app_list/app_list_constants.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... ui/app_list/app_list_constants.cc:7: #include "ui/app_list/app_list_features.h" unused? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:9: #include "ui/app_list/app_list_features.h" Are these two used in this file? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/all_a... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/all_a... ui/app_list/views/all_apps_tile_item_view.h:10: #include "ui/app_list/views/app_list_view.h" nit: move this to cc file and use forward decl here. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.h:15: #include "ui/app_list/views/app_list_view.h" ditto, using forward decl https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: const int kShelfSize = 48; const -> constexpr for all https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:206: display_observer_.Remove(display::Screen::GetScreen()); nit: Get rid of this since ScopedObsever would do this automatically. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:501: void AppListView::HandleDrag(gfx::Point location, ui::EventType type) { nit: Break this into StartDrag, UpdateDrag and EndDrag. We can keep HandleDrag to call them from the switch statement. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:670: UpdateDimensions(); Why this is needed? Shouldn't UpdateDimensions() only be called when display/workarea changes? Think this and ToFullscreen() calls etc belongs to OnDisplayMetricsChanged. Resizing hosting widget from inside Layout is not good since widget resizing might trigger Layout again. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:677: fullscreen_widget_bounds_ = recomputed_fullscreen_widget_bounds; |fullscreen_widget_bounds_| is used to track the bounds for dragging. Why we need to update it here? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:17: #include "ui/display/screen.h" Can this be moved to cc? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:120: void HandleDrag(gfx::Point location, ui::EventType type); gfx::Point -> const gfx::Point& https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:132: // Overridden from ui::EventHandler nit: end with ":" like other overridden comments https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/conte... ui/app_list/views/contents_view.h:19: #include "ui/app_list/views/app_list_view.h" ditto, forward decl AppListView https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:72: : kBackgroundBorderCornerRadius, Can we use a const member var for border radius and initilze it in ctor ? https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.h:13: #include "ui/app_list/views/app_list_view.h" ditto https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... ui/app_list/views/start_page_view.h:15: #include "ui/app_list/views/app_list_view.h" ditto
The CQ bit was checked by newcomer@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:230001) has been deleted
The CQ bit was checked by newcomer@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...
Addressed Comments! https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... File ui/app_list/app_list_constants.cc (left): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/app_list_consta... ui/app_list/app_list_constants.cc:6: On 2017/05/24 18:28:17, xiyuan wrote: > nit: please restore this. Prevailing code style is to have an empty line between > different seconds of includes. Done. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:70: features::IsFullscreenAppListEnabled() On 2017/05/26 01:27:56, vadimt wrote: > And make sure that a SearchBoxBackground instance is never shared by 2 > consequential parameterized tests (with different values of > IsFullscreenAppListEnabled()). They aren't. SearchBoxBackground is created when a SearchBoxView is created. https://codereview.chromium.org/2898743002/diff/1/ui/app_list/views/search_bo... ui/app_list/views/search_box_view.cc:134: app_list_view_(), On 2017/05/26 01:27:56, vadimt wrote: > Just old code. nullptr is a new way, and is my preference. It is now initialized as a constant pointer to the AppListView. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: int kShelfSize = 48; On 2017/05/26 17:58:55, vadimt wrote: > Nah, you can just leave this. Acknowledged. https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/80001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:173: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); On 2017/05/26 17:58:55, vadimt wrote: > const then Done. https://codereview.chromium.org/2898743002/diff/70024/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:136: if (app_list::features::IsFullscreenAppListEnabled()) { On 2017/05/26 18:27:31, xiyuan wrote: > Can you replicate the logic in AppListDelegateImpl::OnAppListVisibilityChanged > as well so that the feature works for mash? > > You can try mash by adding "--mash" to chrome's command line. Ideally we should > have a single code path but we are not there yet. :( Resolved offline, created Mustash bug. https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:139: // behavior. On 2017/05/26 17:58:55, vadimt wrote: > Nit: for OnXXX methods, you don't need to specify why they are called. Only > when. > And rename param to something like is_active. Note: not isActive. Done. https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf.h#newco... ash/shelf/shelf.h:140: void OnAppListIsActive(bool hideBackground); On 2017/05/26 18:27:31, xiyuan wrote: > hideBackground -> hide_background > > It is strange that the name looks irrelevant to applist active state. Should it > be called something like |active|? Renamed to is_active . https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf_layout_... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/70024/ash/shelf/shelf_layout_... ash/shelf/shelf_layout_manager.h:63: // Called when the AppList is active in order to restrict background behavior. On 2017/05/26 17:58:55, vadimt wrote: > Nit same. Done. Also renamed to |is_active| for consistency. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... File ui/app_list/app_list_constants.cc (left): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... ui/app_list/app_list_constants.cc:6: On 2017/05/26 18:27:31, xiyuan wrote: > Please restore. There should be an empty line between include sections. Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... File ui/app_list/app_list_constants.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_co... ui/app_list/app_list_constants.cc:7: #include "ui/app_list/app_list_features.h" On 2017/05/26 18:27:31, xiyuan wrote: > unused? You're correct. Thank you. Is there a tool for me to detect this? (past projects I've been on have had tools to detect useless includes.) https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:9: #include "ui/app_list/app_list_features.h" On 2017/05/26 18:27:31, xiyuan wrote: > Are these two used in this file? Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/all_a... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/all_a... ui/app_list/views/all_apps_tile_item_view.h:10: #include "ui/app_list/views/app_list_view.h" On 2017/05/26 18:27:31, xiyuan wrote: > nit: move this to cc file and use forward decl here. > > See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_main_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_main_view.h:15: #include "ui/app_list/views/app_list_view.h" On 2017/05/26 18:27:31, xiyuan wrote: > ditto, using forward decl Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:60: const int kShelfSize = 48; On 2017/05/26 18:27:31, xiyuan wrote: > const -> constexpr for all Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:71: const float kLauncherOpacity = 0.8; On 2017/05/26 17:58:55, vadimt wrote: > Comment!!!!! Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:206: display_observer_.Remove(display::Screen::GetScreen()); On 2017/05/26 18:27:31, xiyuan wrote: > nit: Get rid of this since ScopedObsever would do this automatically. Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:501: void AppListView::HandleDrag(gfx::Point location, ui::EventType type) { On 2017/05/26 18:27:31, xiyuan wrote: > nit: Break this into StartDrag, UpdateDrag and EndDrag. We can keep HandleDrag > to call them from the switch statement. Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:516: int new_y_position = location.y() - initial_drag_separation_ + On 2017/05/26 17:58:55, vadimt wrote: > Nit: this can be const. Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:670: UpdateDimensions(); On 2017/05/26 18:27:31, xiyuan wrote: > Why this is needed? Shouldn't UpdateDimensions() only be called when > display/workarea changes? Think this and ToFullscreen() calls etc belongs to > OnDisplayMetricsChanged. > > Resizing hosting widget from inside Layout is not good since widget resizing > might trigger Layout again. You're correct. Thanks for pointing that out. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:677: fullscreen_widget_bounds_ = recomputed_fullscreen_widget_bounds; On 2017/05/26 18:27:31, xiyuan wrote: > |fullscreen_widget_bounds_| is used to track the bounds for dragging. Why we > need to update it here? The bounds need to be updated when the screen rotates. This should live in OnDisplayMetricsChanged. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:17: #include "ui/display/screen.h" On 2017/05/26 18:27:32, xiyuan wrote: > Can this be moved to cc? Done. Forward declared Screen. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:120: void HandleDrag(gfx::Point location, ui::EventType type); On 2017/05/26 18:27:32, xiyuan wrote: > gfx::Point -> const gfx::Point& Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:120: void HandleDrag(gfx::Point location, ui::EventType type); On 2017/05/26 17:58:55, vadimt wrote: > const gfx::Point& Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:132: // Overridden from ui::EventHandler On 2017/05/26 18:27:32, xiyuan wrote: > nit: end with ":" like other overridden comments Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:176: ScopedObserver<display::Screen, display::DisplayObserver> display_observer_; On 2017/05/26 17:58:55, vadimt wrote: > Comments for all new fields (in .h files). Everywhere, and forever. Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/conte... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/conte... ui/app_list/views/contents_view.h:19: #include "ui/app_list/views/app_list_view.h" On 2017/05/26 18:27:32, xiyuan wrote: > ditto, forward decl AppListView Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:72: : kBackgroundBorderCornerRadius, On 2017/05/26 18:27:32, xiyuan wrote: > Can we use a const member var for border radius and initilze it in ctor ? Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:170: is_fullscreen_enabled_ = features::IsFullscreenAppListEnabled(); On 2017/05/26 17:58:55, vadimt wrote: > const Done, in the header line 127. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/searc... ui/app_list/views/search_box_view.h:13: #include "ui/app_list/views/app_list_view.h" On 2017/05/26 18:27:32, xiyuan wrote: > ditto Done. https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:207: : std::min<int>(kNumStartPageTiles, display_results.size())); On 2017/05/26 17:58:55, vadimt wrote: > It really doesn't work w/o <int>? It works without <int>, I just left it in after debugging an issue. Fixed! https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... File ui/app_list/views/start_page_view.h (right): https://codereview.chromium.org/2898743002/diff/70024/ui/app_list/views/start... ui/app_list/views/start_page_view.h:15: #include "ui/app_list/views/app_list_view.h" On 2017/05/26 18:27:32, xiyuan wrote: > ditto Done. https://codereview.chromium.org/2898743002/diff/150001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/150001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:382: search_box_view_->SetAppListView(this); On 2017/05/26 17:58:55, vadimt wrote: > Constructor parameter, const field. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:40: shelf->OnAppListIsActive(true); This repeated part can be moved to after if. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:124: void StartDrag(const gfx::Point& location); You don't need to declare StartDrag in .h file. Anonymous namespace in .cc.
https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:46: shelf->OnAppListIsActive(true); Should this be "false" ? We probably can use |visible| here. https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:135: nit: get rid of the empty line. And maybe move the comment inside "if" similar to what we have in OnDismissed(). https://codereview.chromium.org/2898743002/diff/250001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/250001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:318: bool app_list_is_active_ = false; nit: Brief comment to document the member. e.g. // Whether the app launcher is active. Use the default transparent // shelf background when the flag it set. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:813: if (launcher_background_shield_) { nit: bail out earlier i.e. if (!launcher_background_shield_) return; ... https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:830: Layout(); This is probably not needed since it should be called automatically when the widget bounds is changed. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:126: bool const is_fullscreen_enabled_; nit: bool const -> const bool https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/star... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/star... ui/app_list/views/start_page_view.cc:64: class SearchBoxSpacerView : public views::View { Where is this used?
One more thing, please format the CL description to something like this: <subject, less than 72 chars> <empty_line> <body, wrap around 80 chars> <empty_line> BUG=xxx TEST=xxx
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent black background. supports orientation changes. UI tests are not yet implemented, waiting on animation curves. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 ========== to ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 TEST=xxx ==========
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 TEST=xxx ========== to ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 ==========
Patchset #9 (id:270001) has been deleted
Patchset #9 (id:290001) has been deleted
The CQ bit was checked by newcomer@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #9 (id:310001) has been deleted
The CQ bit was checked by newcomer@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...
Patchset #9 (id:330001) has been deleted
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 newcomer@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...
Rebased and addressed comments! https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:40: shelf->OnAppListIsActive(true); On 2017/05/26 23:44:08, vadimt wrote: > This repeated part can be moved to after if. Done. https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:46: shelf->OnAppListIsActive(true); On 2017/05/30 16:32:33, xiyuan wrote: > Should this be "false" ? We probably can use |visible| here. Correct, I noticed this too and It's fixed. https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:135: On 2017/05/30 16:32:33, xiyuan wrote: > nit: get rid of the empty line. And maybe move the comment inside "if" similar > to what we have in OnDismissed(). Done. https://codereview.chromium.org/2898743002/diff/250001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/250001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:318: bool app_list_is_active_ = false; On 2017/05/30 16:32:34, xiyuan wrote: > nit: Brief comment to document the member. > > e.g. > // Whether the app launcher is active. Use the default transparent > // shelf background when the flag it set. Done. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:813: if (launcher_background_shield_) { On 2017/05/30 16:32:34, xiyuan wrote: > nit: bail out earlier > > i.e. > > if (!launcher_background_shield_) > return; > > ... Done. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:830: Layout(); On 2017/05/30 16:32:34, xiyuan wrote: > This is probably not needed since it should be called automatically when the > widget bounds is changed. Done. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:124: void StartDrag(const gfx::Point& location); Done. Also moved HandleDrag, UpdateDrag, and EndDrag. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:126: bool const is_fullscreen_enabled_; On 2017/05/30 16:32:34, xiyuan wrote: > nit: bool const -> const bool Done. https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/star... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2898743002/diff/250001/ui/app_list/views/star... ui/app_list/views/start_page_view.cc:64: class SearchBoxSpacerView : public views::View { Seems like dead code. I commented it out and ran tests, all pass.
lgtm with nits https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:43: shelf->OnAppListIsActive(visible); nit: get rid of |shelf| and use Shelf::ForWindow in-place i.e. Shelf::ForWindow(root_window)->OnAppListIsActive(visible); and if it could fit in one line, get rid of {} as well. https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:137: shelf->OnAppListIsActive(true); nit: Get rid of |shelf| and use Shelf::ForWindow in-place. https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/all_... File ui/app_list/views/all_apps_tile_item_view.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/all_... ui/app_list/views/all_apps_tile_item_view.cc:19: #include "ui/views/widget/widget.h" nit: unused ? https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/cont... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/cont... ui/app_list/views/contents_view.h:50: explicit ContentsView(AppListMainView* app_list_main_view, nit: remove "explicit". It is only needed for single arg ctor (or there are args with default value). https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/star... File ui/app_list/views/start_page_view.cc (left): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/star... ui/app_list/views/start_page_view.cc:56: nit: please restore. Prevailing style is to have an empty line after opening a namespace and before closing it.
lgtm + last comments https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf.h#newc... ash/shelf/shelf.h:141: // Called when the app list is active. Actually when the active status changes. https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:63: // Called when the AppList is active. Same https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:623: if (!launcher_background_shield_) Why we are doing this, is not clear. Neither it's clear if you read a comment for launcher_background_shield_ in the header. The comment in the header should say when it's null or not null in such a way that the reader can understand pieces like this. Please review all other added fields too (and add comments in .h files). Don't overdo, but think about a reader who finds no comments both in .h and in .cc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sky@chromium.org changed reviewers: + msw@chromium.org
+msw as he knows more about the shelf than I. I only looked at the ash side. https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:129: Shelf::ForWindow(root_window)->shelf_widget()->GetAppListButton(); I prefer minimizing direct dependencies between classes and using observers. How about adding ShellObserver::OnAppListShown()? This code would call to Shell to notify observers and shelf could observe said function. Shelf would use this function to both update the applistbutton (as done on 131) as well as the new functionality you are adding. https://codereview.chromium.org/2898743002/diff/350001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/root_window_contro... ash/root_window_controller.cc:912: CreateContainer(kShellWindowId_AppListContainer, "AppListContainer", shell_window_ids is meant to contain the relative order of the containers. Please update the file to indicate new position. https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:177: app_list_is_active_ = is_active; Is it really necessary to cache this state? Could it be looked up when necessary?
Patchset #10 (id:370001) has been deleted
Patchset #11 (id:410001) has been deleted
Patchset #10 (id:390001) has been deleted
The CQ bit was checked by newcomer@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #11 (id:450001) has been deleted
Patchset #10 (id:430001) has been deleted
The CQ bit was checked by newcomer@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:470001) has been deleted
The CQ bit was checked by newcomer@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_...)
Patchset #10 (id:490001) has been deleted
The CQ bit was checked by newcomer@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...
Patchset #10 (id:510001) has been deleted
https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:43: shelf->OnAppListIsActive(visible); On 2017/05/30 22:18:50, xiyuan wrote: > nit: get rid of |shelf| and use Shelf::ForWindow in-place > > i.e. > Shelf::ForWindow(root_window)->OnAppListIsActive(visible); > > and if it could fit in one line, get rid of {} as well. Done. https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:129: Shelf::ForWindow(root_window)->shelf_widget()->GetAppListButton(); On 2017/05/31 15:04:17, sky wrote: > I prefer minimizing direct dependencies between classes and using observers. How > about adding ShellObserver::OnAppListShown()? This code would call to Shell to > notify observers and shelf could observe said function. Shelf would use this > function to both update the applistbutton (as done on 131) as well as the new > functionality you are adding. Done. https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:137: shelf->OnAppListIsActive(true); On 2017/05/30 22:18:50, xiyuan wrote: > nit: Get rid of |shelf| and use Shelf::ForWindow in-place. Done. https://codereview.chromium.org/2898743002/diff/350001/ash/root_window_contro... File ash/root_window_controller.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/root_window_contro... ash/root_window_controller.cc:912: CreateContainer(kShellWindowId_AppListContainer, "AppListContainer", On 2017/05/31 15:04:17, sky wrote: > shell_window_ids is meant to contain the relative order of the containers. > Please update the file to indicate new position. Done. https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf.h File ash/shelf/shelf.h (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf.h#newc... ash/shelf/shelf.h:141: // Called when the app list is active. On 2017/05/30 23:52:08, vadimt wrote: > Actually when the active status changes. Done. https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:177: app_list_is_active_ = is_active; On 2017/05/31 15:04:17, sky wrote: > Is it really necessary to cache this state? Could it be looked up when > necessary? Done. https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/350001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:63: // Called when the AppList is active. On 2017/05/30 23:52:08, vadimt wrote: > Same Done. https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/all_... File ui/app_list/views/all_apps_tile_item_view.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/all_... ui/app_list/views/all_apps_tile_item_view.cc:19: #include "ui/views/widget/widget.h" On 2017/05/30 22:18:50, xiyuan wrote: > nit: unused ? Done. https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:623: if (!launcher_background_shield_) On 2017/05/30 23:52:08, vadimt wrote: > Why we are doing this, is not clear. Neither it's clear if you read a comment > for launcher_background_shield_ in the header. > > The comment in the header should say when it's null or not null in such a way > that the reader can understand pieces like this. Please review all other added > fields too (and add comments in .h files). > > Don't overdo, but think about a reader who finds no comments both in .h and in > .cc. Done. https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/cont... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/cont... ui/app_list/views/contents_view.h:50: explicit ContentsView(AppListMainView* app_list_main_view, On 2017/05/30 22:18:50, xiyuan wrote: > nit: remove "explicit". It is only needed for single arg ctor (or there are args > with default value). Done. https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/star... File ui/app_list/views/start_page_view.cc (left): https://codereview.chromium.org/2898743002/diff/350001/ui/app_list/views/star... ui/app_list/views/start_page_view.cc:56: On 2017/05/30 22:18:50, xiyuan wrote: > nit: please restore. Prevailing style is to have an empty line after opening a > namespace and before closing it. Done.
The CQ bit was checked by newcomer@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.
https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:43: shelf->OnAppListIsActive(visible); On 2017/06/01 01:42:53, newcomer wrote: > On 2017/05/30 22:18:50, xiyuan wrote: > > nit: get rid of |shelf| and use Shelf::ForWindow in-place > > > > i.e. > > Shelf::ForWindow(root_window)->OnAppListIsActive(visible); > > > > and if it could fit in one line, get rid of {} as well. > > Done. Thanks for starting down this path! What I'm really suggesting here is Shell becomes the ApplistDelegate. Shell::OnApplistVisibilityChanged() *always* calls to OnAppListIsActive, or at this point we should name it OnAppListVisibilityChanged(). Shelf code has a ShellObserver that updates both the app-list_button and whatever you need the new code to do? In other words, I'm suggesting you nuke this class entirely.
https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:125: Shell::Get()->NotifyAppListShownOrDismissed(is_visible_, root_window); Not directly related to this CL. But along with sky@'s comment, we probably can remove code here and in OnDismissed (only keep |is_visible_| update part). AppListPresenterImpl should call into AppList::OnVisibilityChanged [1]. If it does not call it for classic ash, we should fix it so that both cash and mash works on the same code path for notifying app list visibility. [1]: https://cs.chromium.org/chromium/src/ui/app_list/presenter/app_list_presenter...
I'd like to see some simplification, perhaps landed as a prerequisite for this CL. Also, are there any mocks or screen capture video/pictures of this new functionality in action? I'm not sure what 'peeking' really means. https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:41: Shelf::ForWindow(root_window)->OnAppListIsActive(); Why does this call OnAppListIsActive without arguments regardless of whether the app list is shown or hidden? There are several changes I'd like to see here: (1) At the minimum, combine Shelf::OnAppListIsActive and Shelf::OnAppListShownOrDismissed into a single method, called regardless of the fullscreen flag, it should be called something like OnAppListVisibilityChanged(bool visible) (presuming it's only called for the Shelf instance on the corresponding display). That function should update the AppListButton, that should not be done here. (2) If you're more ambitious, skip a bunch of indirection, by removing this AppListDelegateImpl class, rename the AppListDelegate interface to AppListObserver, implemented by Shelf, and let Shelf just do Shell::Get()->app_list()->AddObserver(this). https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:125: Shell::Get()->NotifyAppListShownOrDismissed(is_visible_, root_window); On 2017/06/01 15:15:13, xiyuan wrote: > Not directly related to this CL. But along with sky@'s comment, we probably can > remove code here and in OnDismissed (only keep |is_visible_| update part). > AppListPresenterImpl should call into AppList::OnVisibilityChanged [1]. If it > does not call it for classic ash, we should fix it so that both cash and mash > works on the same code path for notifying app list visibility. > > > [1]: > https://cs.chromium.org/chromium/src/ui/app_list/presenter/app_list_presenter... +1 for simplifying AppList visibility observation along these lines, and/or as I suggested in AppListDelegate.
Responded to comments, created a bug to unify the ash/mash code paths on the next CL. https://codereview.chromium.org/2898743002 https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/350001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:43: shelf->OnAppListIsActive(visible); On 2017/06/01 13:34:05, sky wrote: > On 2017/06/01 01:42:53, newcomer wrote: > > On 2017/05/30 22:18:50, xiyuan wrote: > > > nit: get rid of |shelf| and use Shelf::ForWindow in-place > > > > > > i.e. > > > Shelf::ForWindow(root_window)->OnAppListIsActive(visible); > > > > > > and if it could fit in one line, get rid of {} as well. > > > > Done. > > Thanks for starting down this path! What I'm really suggesting here is Shell > becomes the ApplistDelegate. Shell::OnApplistVisibilityChanged() *always* calls > to OnAppListIsActive, or at this point we should name it > OnAppListVisibilityChanged(). Shelf code has a ShellObserver that updates both > the app-list_button and whatever you need the new code to do? In other words, > I'm suggesting you nuke this class entirely. Discussed Offline, I've created a bug and plan merge the change into a new CL. https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:41: Shelf::ForWindow(root_window)->OnAppListIsActive(); On 2017/06/01 20:03:45, msw wrote: > Why does this call OnAppListIsActive without arguments regardless of whether the > app list is shown or hidden? There are several changes I'd like to see here: > (1) At the minimum, combine Shelf::OnAppListIsActive and > Shelf::OnAppListShownOrDismissed into a single method, called regardless of the > fullscreen flag, it should be called something like > OnAppListVisibilityChanged(bool visible) (presuming it's only called for the > Shelf instance on the corresponding display). That function should update the > AppListButton, that should not be done here. > (2) If you're more ambitious, skip a bunch of indirection, by removing this > AppListDelegateImpl class, rename the AppListDelegate interface to > AppListObserver, implemented by Shelf, and let Shelf just do > Shell::Get()->app_list()->AddObserver(this). The function has no arguments because its purpose is to force ShelfLayoutManager to run MaybeUpdateShelfBackground(), which does not require a Bool because MaybeUpdateShelfBackground() checks to see if the app list is active. (1)&(2) sky@ just recommended we get rid of AppListDelegateImpl and implement AppListDelegate in Shell. This would mean that the mash implementation would call the same OnAppListVisibilityChanged(bool visible) as Ash. I have it implemented, I'm planning on submitting it after this CL (BUG -> https://bugs.chromium.org/p/chromium/issues/detail?id=729715) https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/530001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:125: Shell::Get()->NotifyAppListShownOrDismissed(is_visible_, root_window); On 2017/06/01 15:15:13, xiyuan wrote: > Not directly related to this CL. But along with sky@'s comment, we probably can > remove code here and in OnDismissed (only keep |is_visible_| update part). > AppListPresenterImpl should call into AppList::OnVisibilityChanged [1]. If it > does not call it for classic ash, we should fix it so that both cash and mash > works on the same code path for notifying app list visibility. > > > [1]: > https://cs.chromium.org/chromium/src/ui/app_list/presenter/app_list_presenter... OK! I just created a bug for xiyuan, msw, and sky's comments. https://bugs.chromium.org/p/chromium/issues/detail?id=729715#
Patchset #11 (id:550001) has been deleted
Removed App_list_delegate_impl and refactored notifications to the AppListButton and to the ShelfLayoutManager. Unified Mash/Ash code paths.
https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:18: AppListDelegateImpl::AppListDelegateImpl() { Can we remove this code since Shell is AppListDelegate now? https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:125: Shell::Get()->NotifyAppListShownOrDismissed(is_visible_, root_window); Think we can remove this code block from Line 123-125 and also line 133-135 since AppListPresenterImpl would call Shell's app_list() instance via mojo to notify the visiblity change [1]. [1]: https://cs.chromium.org/chromium/src/ui/app_list/presenter/app_list_presenter... https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:249: OnAppListDismissed(); This is not right on multiple monitors. AppListButton is per shelf and shelf is per display. So this would cause all AppListButton on all displays to change its status, which is not right. We need to filter it by root window. https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:12: #include "ash/shelf/app_list_button.h" unused? https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:22: #include "ui/app_list/app_list_features.h" unused? https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc#newcode607 ash/shell.cc:607: app_list()->set_delegate(this); If we unify the app list visibility change notification code path, we should do this regardless of the Ash config. https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc#newcode1228 ash/shell.cc:1228: NotifyAppListShownOrDismissed(visible, NotifyAppListShownOrDismissed -> NotifyAppListVisiblityChanged and also update the ShellObserver method name to make it consistent.
Patchset #12 (id:590001) has been deleted
Responded to Xiyuan@ 's comments. https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:18: AppListDelegateImpl::AppListDelegateImpl() { On 2017/06/06 15:51:04, xiyuan wrote: > Can we remove this code since Shell is AppListDelegate now? Done. https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:125: Shell::Get()->NotifyAppListShownOrDismissed(is_visible_, root_window); On 2017/06/06 15:51:04, xiyuan wrote: > Think we can remove this code block from Line 123-125 and also line 133-135 > since AppListPresenterImpl would call Shell's app_list() instance via mojo to > notify the visiblity change [1]. > > [1]: > https://cs.chromium.org/chromium/src/ui/app_list/presenter/app_list_presenter... Done. https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:249: OnAppListDismissed(); On 2017/06/06 15:51:04, xiyuan wrote: > This is not right on multiple monitors. AppListButton is per shelf and shelf is > per display. So this would cause all AppListButton on all displays to change its > status, which is not right. > > We need to filter it by root window. Done. https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc File ash/shelf/shelf.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:12: #include "ash/shelf/app_list_button.h" On 2017/06/06 15:51:04, xiyuan wrote: > unused? Done. https://codereview.chromium.org/2898743002/diff/570001/ash/shelf/shelf.cc#new... ash/shelf/shelf.cc:22: #include "ui/app_list/app_list_features.h" On 2017/06/06 15:51:04, xiyuan wrote: > unused? Done. https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc#newcode607 ash/shell.cc:607: app_list()->set_delegate(this); On 2017/06/06 15:51:04, xiyuan wrote: > If we unify the app list visibility change notification code path, we should do > this regardless of the Ash config. Done. https://codereview.chromium.org/2898743002/diff/570001/ash/shell.cc#newcode1228 ash/shell.cc:1228: NotifyAppListShownOrDismissed(visible, On 2017/06/06 15:51:04, xiyuan wrote: > NotifyAppListShownOrDismissed -> NotifyAppListVisiblityChanged > > and also update the ShellObserver method name to make it consistent. Done.
Mostly nits. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:247: if (shelf_ == Shelf::ForWindow(root_window)) { nit: reverse the condition and bail out early. i.e. if (shelf_ != Shelf::ForWindow(root_window)) return; ... https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:435: if (shelf_widget() == Shelf::ForWindow(root_window)->shelf_widget()) { nit: reverse and bail out early if (shelf_widget() != Shelf::ForWindow(root_window)->shelf_widget()) return; https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:64: void OnAppListIsActive(); Can this be removed since we have OnAppListVisibilityChanged? https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode56 ash/shell.cc:56: #include "ash/shelf/app_list_button.h" unused? https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode139 ash/shell.cc:139: #include "ui/app_list/app_list_features.h" unused? https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode141 ash/shell.cc:141: #include "ui/app_list/presenter/app_list_delegate.h" remove since it is included in h already.
Responded to Xiyuan@'s comments. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:247: if (shelf_ == Shelf::ForWindow(root_window)) { On 2017/06/06 16:50:18, xiyuan wrote: > nit: reverse the condition and bail out early. > > i.e. > > if (shelf_ != Shelf::ForWindow(root_window)) > return; > > ... Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:435: if (shelf_widget() == Shelf::ForWindow(root_window)->shelf_widget()) { On 2017/06/06 16:50:18, xiyuan wrote: > nit: reverse and bail out early > > if (shelf_widget() != Shelf::ForWindow(root_window)->shelf_widget()) > return; Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:64: void OnAppListIsActive(); On 2017/06/06 16:50:18, xiyuan wrote: > Can this be removed since we have OnAppListVisibilityChanged? Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode56 ash/shell.cc:56: #include "ash/shelf/app_list_button.h" On 2017/06/06 16:50:19, xiyuan wrote: > unused? Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode139 ash/shell.cc:139: #include "ui/app_list/app_list_features.h" On 2017/06/06 16:50:19, xiyuan wrote: > unused? Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shell.cc#newcode141 ash/shell.cc:141: #include "ui/app_list/presenter/app_list_delegate.h" On 2017/06/06 16:50:18, xiyuan wrote: > remove since it is included in h already. Done.
lgtm++
Thanks for fixing up the delegate mess, that's much better now. https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:214: // Tests that the peeking launcher closes if the user taps outside it's nit: its (no apostrophe) https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:219: app_list_presenter_impl()->Show(GetPrimaryDisplayId()); nit: EXPECT_TRUE(app_list_presenter_impl()->GetTargetVisibility()); after this https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:224: gfx::Rect search_box_bounds = app_list_presenter_impl() optional nit: inline this in the |tap_point| assignment https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:246: // If the AppList belonged to this shelf. nit: remove this comment, it's not a complete sentence, and the code conveys the conditional well enough https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:248: if (shown) { nit: no curlies needed for the nested if/else https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:435: if (shelf_widget() == Shelf::ForWindow(root_window)->shelf_widget()) { nit: if (shelf_ == Shelf::ForWindow(root_window)) or in the new patch set: if (shelf_ != Shelf::ForWindow(root_window)) https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:322: bool app_list_is_being_shown; Add a trailing underscore and consider is_app_list_visible_ https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... File ui/app_list/app_list_constants.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... ui/app_list/app_list_constants.cc:13: const size_t kNumStartPageTiles = 5; ditto, move this assignment down to match the moved decl https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... ui/app_list/app_list_constants.h:19: namespace fullscreen_constants { nit: don't add this nested namespace, move this next to the existing app_list::kNumStartPageTiles, and rename this variant something like kNumStartPageTilesFullscreen https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/all_... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/all_... ui/app_list/views/all_apps_tile_item_view.h:20: explicit AllAppsTileItemView(ContentsView* contents_view, nit: remove explicit https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_main_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_main_view.h:39: explicit AppListMainView(AppListViewDelegate* delegate, nit: remove explicit https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_main_view_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_main_view_unittest.cc:85: // In Ash, the third argument is a container aura::Window, but it is always optional nit: remove this old irrelevant comment. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:262: if (!launcher_background_shield_) q: Why change this? it seems to make more sense to check IsFullscreenAppListEnabled, and then remove this function altogether once the feature is always enabled, right? https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:267: display_work_area_bounds_ = display::Screen::GetScreen() optional: I don't think it makes sense to cache |display_work_area_bounds_| nor |default_y_for_state_| as members of the AppListView class, they should just be calculated as needed as function-local variables. If you take that advice, you can remove UpdateDimensions, or replace it with a cc-local anon namespace function like |int GetPeekingYPosition()|. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:462: app_list_overlay_view_params.bounds = app_list_overlay_view_bounds; When you make the bounds of this widget extend beyond the display's lower edge (by using the full display height with the top y-coordinate of the peeking app list state), does that make the widget extend onto a display located below the display actually showing the app list? You should test this with a vertical multi-display setup, to ensure we don't have some weird behavior like that. I think this command line switch should help: chrome --ash-host-window-bounds=600x600,0+700-600x600 [maybe also try --ash-enable-unified-desktop?] https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:483: // created. nit: don't remove the close paren, or remove the open paren too https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:498: void AppListView::HandleDrag(const gfx::Point& location, ui::EventType type) { I suggest removing this function, and just inlining the calls to Start/Update/EndDrag in OnMouseEvent and OnGestureEvent, where you already have EventType switch statements. (remove a layer of indirection) https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:522: initial_drag_separation_ = location.y(); nit: keep a member |gfx::Point initial_drag_location_| instead, the meaning of that is more immediately obvious than a 'separation' int https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:523: fullscreen_widget_bounds_ = fullscreen_widget_->GetWindowBoundsInScreen(); I'm not sure it makes sense to cache |fullscreen_widget_bounds_| as a class member, if the value is readily accessible as fullscreen_widget_->GetWindowBoundsInScreen() https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:565: // If the user releases +/- 1/3 of launcher_threshold_ , snap to the nit: no space before comma, feel free to use vertical bars around |identifiers| in comments here and elsewhere https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:567: if (std::abs(launcher_snap_y - new_y_position) < (launcher_threshold_)) { nit: no parens needed around |launcher_threshold_| https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:567: if (std::abs(launcher_snap_y - new_y_position) < (launcher_threshold_)) { I don't think it makes sense to cache |launcher_threshold_| as a member variable, it's only really used in this conditional block of this function, and it's easy enough to calculate here. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:568: // If the drag released to the state above. Can you try clarifying this comment? It doesn't seem helpful to me. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:569: if (is_fullscreen_launcher_) { Does this actually do anything? It seems like you're instructing the app list to go to fullscreen when it already is fullscreen, ditto for peeking. I suppose this might be fitting the app list bounds to the target bounds from some intermediary drag state? If so, revise the comment to something like: "Set the app list bounds to the target bounds for the current state, to revert the incomplete drag resizing." https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:574: } else if ((launcher_snap_y + (launcher_threshold_)) < new_y_position) { nit: no parens needed around |launcher_threshold_| https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:575: // If the user released to the state below. Can you try clarifying this comment? It doesn't seem helpful to me. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:582: // if the user released to the state above, go to the state above. Can you try clarifying this comment? It doesn't seem helpful to me. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:622: if (!launcher_background_shield_) ditto: it seems to make more sense to check IsFullscreenAppListEnabled, and then remove this early return altogether once the feature is always enabled, right? https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:638: if (!launcher_background_shield_) ditto: it seems to make more sense to check IsFullscreenAppListEnabled, and then remove this early return altogether once the feature is always enabled, right? https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:692: // If the fullscreen feature has been enabled. Ditto, why comment and then check for the shield? Just check the feature flag https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:812: if (!launcher_background_shield_) ditto: check the feature flag https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:817: gfx::Rect recomputed_fullscreen_widget_bounds( nit: inline this below, there's no reason for this temporary value https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:824: if (is_fullscreen_launcher_) { nit: curlies not needed https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:72: void UpdateDimensions(); nit: UpdateSize? UpdateFullscreen? https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:102: void ToFullscreen(); nit: combine these two to void SetFullscreen(bool fullscreen); https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:169: // Owned by the app list's widget. Null if the fullscreen launcher is not nit: avoid using the term 'launcher' here and elsewhere in this CL, 'app list' is more appropriate. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:177: // based on the launcher state and screen orientation. Does this actually change based on the launcher state? It seems like it's always the same offset from the bottom of the display's work area. I think this should be renamed to something like |peeking_y_position_|, if not removed as a member variable. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/cont... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/cont... ui/app_list/views/contents_view.h:195: // Owned by the views hierarchy. nit: one space after // https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:123: app_list::AppListView* app_list_view_; // Owned by views hierarchy. nit: one space after // https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:125: const bool is_fullscreen_enabled_; nit: comment
Patchset #14 (id:650001) has been deleted
addressed msw@'s comments. PTAL! https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:214: // Tests that the peeking launcher closes if the user taps outside it's On 2017/06/06 17:56:31, msw wrote: > nit: its (no apostrophe) Done. https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:219: app_list_presenter_impl()->Show(GetPrimaryDisplayId()); On 2017/06/06 17:56:31, msw wrote: > nit: EXPECT_TRUE(app_list_presenter_impl()->GetTargetVisibility()); after this Done. https://codereview.chromium.org/2898743002/diff/610001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:224: gfx::Rect search_box_bounds = app_list_presenter_impl() On 2017/06/06 17:56:31, msw wrote: > optional nit: inline this in the |tap_point| assignment Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:246: // If the AppList belonged to this shelf. On 2017/06/06 17:56:31, msw wrote: > nit: remove this comment, it's not a complete sentence, and the code conveys the > conditional well enough Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/app_list_but... ash/shelf/app_list_button.cc:248: if (shown) { On 2017/06/06 17:56:31, msw wrote: > nit: no curlies needed for the nested if/else Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.cc:435: if (shelf_widget() == Shelf::ForWindow(root_window)->shelf_widget()) { On 2017/06/06 17:56:31, msw wrote: > nit: if (shelf_ == Shelf::ForWindow(root_window)) > or in the new patch set: if (shelf_ != Shelf::ForWindow(root_window)) Done. https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... File ash/shelf/shelf_layout_manager.h (right): https://codereview.chromium.org/2898743002/diff/610001/ash/shelf/shelf_layout... ash/shelf/shelf_layout_manager.h:322: bool app_list_is_being_shown; On 2017/06/06 17:56:31, msw wrote: > Add a trailing underscore and consider is_app_list_visible_ Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... File ui/app_list/app_list_constants.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... ui/app_list/app_list_constants.cc:13: const size_t kNumStartPageTiles = 5; On 2017/06/06 17:56:31, msw wrote: > ditto, move this assignment down to match the moved decl Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... File ui/app_list/app_list_constants.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/app_list_c... ui/app_list/app_list_constants.h:19: namespace fullscreen_constants { On 2017/06/06 17:56:31, msw wrote: > nit: don't add this nested namespace, move this next to the existing > app_list::kNumStartPageTiles, and rename this variant something like > kNumStartPageTilesFullscreen Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/all_... File ui/app_list/views/all_apps_tile_item_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/all_... ui/app_list/views/all_apps_tile_item_view.h:20: explicit AllAppsTileItemView(ContentsView* contents_view, On 2017/06/06 17:56:31, msw wrote: > nit: remove explicit Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_main_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_main_view.h:39: explicit AppListMainView(AppListViewDelegate* delegate, On 2017/06/06 17:56:31, msw wrote: > nit: remove explicit Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_main_view_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_main_view_unittest.cc:85: // In Ash, the third argument is a container aura::Window, but it is always On 2017/06/06 17:56:31, msw wrote: > optional nit: remove this old irrelevant comment. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:262: if (!launcher_background_shield_) On 2017/06/06 17:56:31, msw wrote: > q: Why change this? it seems to make more sense to check > IsFullscreenAppListEnabled, and then remove this function altogether once the > feature is always enabled, right? Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:267: display_work_area_bounds_ = display::Screen::GetScreen() On 2017/06/06 17:56:32, msw wrote: > optional: I don't think it makes sense to cache |display_work_area_bounds_| nor > |default_y_for_state_| as members of the AppListView class, they should just be > calculated as needed as function-local variables. If you take that advice, you > can remove UpdateDimensions, or replace it with a cc-local anon namespace > function like |int GetPeekingYPosition()|. Removed UpdateBounds() and created local variables that serve the same functionality without caching. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:462: app_list_overlay_view_params.bounds = app_list_overlay_view_bounds; On 2017/06/06 17:56:32, msw wrote: > When you make the bounds of this widget extend beyond the display's lower edge > (by using the full display height with the top y-coordinate of the peeking app > list state), does that make the widget extend onto a display located below the > display actually showing the app list? You should test this with a vertical > multi-display setup, to ensure we don't have some weird behavior like that. I > think this command line switch should help: chrome > --ash-host-window-bounds=600x600,0+700-600x600 [maybe also try > --ash-enable-unified-desktop?] It does not extend onto any other display regardless of position. (I just manually tested it). https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:483: // created. On 2017/06/06 17:56:32, msw wrote: > nit: don't remove the close paren, or remove the open paren too Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:498: void AppListView::HandleDrag(const gfx::Point& location, ui::EventType type) { On 2017/06/06 17:56:32, msw wrote: > I suggest removing this function, and just inlining the calls to > Start/Update/EndDrag in OnMouseEvent and OnGestureEvent, where you already have > EventType switch statements. (remove a layer of indirection) Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:522: initial_drag_separation_ = location.y(); On 2017/06/06 17:56:32, msw wrote: > nit: keep a member |gfx::Point initial_drag_location_| instead, the meaning of > that is more immediately obvious than a 'separation' int Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:523: fullscreen_widget_bounds_ = fullscreen_widget_->GetWindowBoundsInScreen(); On 2017/06/06 17:56:31, msw wrote: > I'm not sure it makes sense to cache |fullscreen_widget_bounds_| as a class > member, if the value is readily accessible as > fullscreen_widget_->GetWindowBoundsInScreen() Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:565: // If the user releases +/- 1/3 of launcher_threshold_ , snap to the On 2017/06/06 17:56:32, msw wrote: > nit: no space before comma, feel free to use vertical bars around |identifiers| > in comments here and elsewhere Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:567: if (std::abs(launcher_snap_y - new_y_position) < (launcher_threshold_)) { On 2017/06/06 17:56:31, msw wrote: > I don't think it makes sense to cache |launcher_threshold_| as a member > variable, it's only really used in this conditional block of this function, and > it's easy enough to calculate here. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:567: if (std::abs(launcher_snap_y - new_y_position) < (launcher_threshold_)) { On 2017/06/06 17:56:31, msw wrote: > I don't think it makes sense to cache |launcher_threshold_| as a member > variable, it's only really used in this conditional block of this function, and > it's easy enough to calculate here. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:568: // If the drag released to the state above. On 2017/06/06 17:56:32, msw wrote: > Can you try clarifying this comment? It doesn't seem helpful to me. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:569: if (is_fullscreen_launcher_) { On 2017/06/06 17:56:32, msw wrote: > Does this actually do anything? It seems like you're instructing the app list to > go to fullscreen when it already is fullscreen, ditto for peeking. I suppose > this might be fitting the app list bounds to the target bounds from some > intermediary drag state? If so, revise the comment to something like: "Set the > app list bounds to the target bounds for the current state, to revert the > incomplete drag resizing." Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:574: } else if ((launcher_snap_y + (launcher_threshold_)) < new_y_position) { On 2017/06/06 17:56:31, msw wrote: > nit: no parens needed around |launcher_threshold_| Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:575: // If the user released to the state below. On 2017/06/06 17:56:31, msw wrote: > Can you try clarifying this comment? It doesn't seem helpful to me. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:582: // if the user released to the state above, go to the state above. On 2017/06/06 17:56:32, msw wrote: > Can you try clarifying this comment? It doesn't seem helpful to me. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:622: if (!launcher_background_shield_) On 2017/06/06 17:56:31, msw wrote: > ditto: it seems to make more sense to check IsFullscreenAppListEnabled, and then > remove this early return altogether once the feature is always enabled, right? Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:638: if (!launcher_background_shield_) On 2017/06/06 17:56:31, msw wrote: > ditto: it seems to make more sense to check IsFullscreenAppListEnabled, and then > remove this early return altogether once the feature is always enabled, right? Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:692: // If the fullscreen feature has been enabled. On 2017/06/06 17:56:32, msw wrote: > Ditto, why comment and then check for the shield? Just check the feature flag Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:812: if (!launcher_background_shield_) On 2017/06/06 17:56:32, msw wrote: > ditto: check the feature flag Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:817: gfx::Rect recomputed_fullscreen_widget_bounds( On 2017/06/06 17:56:31, msw wrote: > nit: inline this below, there's no reason for this temporary value Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:824: if (is_fullscreen_launcher_) { On 2017/06/06 17:56:32, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:72: void UpdateDimensions(); On 2017/06/06 17:56:32, msw wrote: > nit: UpdateSize? UpdateFullscreen? Function removed as a side effect of other changes. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:102: void ToFullscreen(); On 2017/06/06 17:56:32, msw wrote: > nit: combine these two to void SetFullscreen(bool fullscreen); Settled offline, Introduced AppListState and ChangeState(AppListState new_state). https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:169: // Owned by the app list's widget. Null if the fullscreen launcher is not On 2017/06/06 17:56:32, msw wrote: > nit: avoid using the term 'launcher' here and elsewhere in this CL, 'app list' > is more appropriate. All of my changes that used 'launcher' have been changed to 'app list'/'app_list' https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:177: // based on the launcher state and screen orientation. On 2017/06/06 17:56:32, msw wrote: > Does this actually change based on the launcher state? It seems like it's always > the same offset from the bottom of the display's work area. I think this should > be renamed to something like |peeking_y_position_|, if not removed as a member > variable. Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/cont... File ui/app_list/views/contents_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/cont... ui/app_list/views/contents_view.h:195: // Owned by the views hierarchy. On 2017/06/06 17:56:32, msw wrote: > nit: one space after // Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:123: app_list::AppListView* app_list_view_; // Owned by views hierarchy. On 2017/06/06 17:56:33, msw wrote: > nit: one space after // Done. https://codereview.chromium.org/2898743002/diff/610001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:125: const bool is_fullscreen_enabled_; On 2017/06/06 17:56:33, msw wrote: > nit: comment Done.
Looking better, here's another round of comments. https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:77: // Tests that app app list hides when focus moves to a normal window. nit: "app app" https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:88: // Tests that app app list remains visible when focus is moved to a different nit: "app app" https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:146: // Tests opening the app app list on a non-primary display, then deleting the nit: "app app" https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:169: // Tests opening the app app list on a tiny display that is too small to contain nit: "app app" https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:609: void AppListView::OnMouseEvent(ui::MouseEvent* event) { Should OnMouseEvent and OnGestureEvent invoke the base class's implementation of each function in the cases where this subclass doesn't do anything? https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:724: int display_height = display::Screen::GetScreen() nit: |display_height| and |default_peeking_y| are only needed in |case PEEKING:| below, use curly braces to scope their use to that case. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:842: gfx::Rect recomputed_fullscreen_widget_bounds( Remove this, it's no longer used. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:850: display_work_area_bounds.height() + kShelfSize - kPeekingAppListHeight, nit: you should probably replace display_work_area_bounds.height() with display_work_area_bounds.bottom() here, otherwise this will break if display_work_area_bounds.y() isn't 0, right? https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:854: if (app_list_state_ == FULLSCREEN) nit: add a comment like "Update the window's bounds to accommodate the new work area." https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:44: enum AppListState { INITIAL = 0, PEEKING, FULLSCREEN }; Comment on this enum and each of its states, the concept of peeking and fullscreen app list states are new and will be unfamiliar to most readers. Also, replace INITIAL with CLOSED (or remove INITIAL and initialize |app_list_state_| to the actual peeking/fullscreen initial state when the view is constructed)? https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:70: void CloseAppList(); You could potentially replace this with SetState(CLOSED) https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:102: void ToFullscreen(); Remove these now that we have ChangeState/SetState. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:104: void ChangeState(AppListState new_state); Rename this SetState https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:106: bool IsFullscreen() const; Change this to |AppListState app_list_state() const { return app_list_state_; } You can optionally also/instead do |bool is_fullscreen() const { return app_list_state_ == FULLSCREEN; } https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:176: int default_peeking_y_ = 0; Remove this, it's no longer used. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:177: bool is_fullscreen_app_list_ = false; Remove this, it's no longer used. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:180: bool is_fullscreen_app_list_enabled_ = false; Move this to a file-local function in cc file's anon namespace, like: bool IsFullscreenAppListEnabled() { // Cache this value to avoid repeated lookup. static bool cached_value = features::IsFullscreenAppListEnabled(); return cached_value; } https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:182: gfx::Rect display_work_area_bounds_; Remove this, it's no longer used. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:185: gfx::Rect fullscreen_widget_bounds_; Remove this, it's no longer used. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:186: // The state of the app list, controlled via ChangeState(). Update ChangeState -> SetState if you accept that rename, or just remove ", controlled via ChangeState()" https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:187: AppListState app_list_state_; Initialize this value here or in the constructor. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:127: const bool is_fullscreen_enabled_; Move this to a file-local function in cc file's anon namespace, like: bool IsFullscreenAppListEnabled() { // Cache this value to avoid repeated lookup. static bool cached_value = features::IsFullscreenAppListEnabled(); return cached_value; }
Patchset #15 (id:690001) has been deleted
Responded to comments! -Alex https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:77: // Tests that app app list hides when focus moves to a normal window. On 2017/06/07 02:31:38, msw wrote: > nit: "app app" Done. https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:88: // Tests that app app list remains visible when focus is moved to a different On 2017/06/07 02:31:38, msw wrote: > nit: "app app" Done. https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:146: // Tests opening the app app list on a non-primary display, then deleting the On 2017/06/07 02:31:38, msw wrote: > nit: "app app" Done. https://codereview.chromium.org/2898743002/diff/670001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:169: // Tests opening the app app list on a tiny display that is too small to contain On 2017/06/07 02:31:38, msw wrote: > nit: "app app" Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:609: void AppListView::OnMouseEvent(ui::MouseEvent* event) { On 2017/06/07 02:31:38, msw wrote: > Should OnMouseEvent and OnGestureEvent invoke the base class's implementation of > each function in the cases where this subclass doesn't do anything? Handled offline. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:724: int display_height = display::Screen::GetScreen() On 2017/06/07 02:31:38, msw wrote: > nit: |display_height| and |default_peeking_y| are only needed in |case PEEKING:| > below, use curly braces to scope their use to that case. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:842: gfx::Rect recomputed_fullscreen_widget_bounds( On 2017/06/07 02:31:38, msw wrote: > Remove this, it's no longer used. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:850: display_work_area_bounds.height() + kShelfSize - kPeekingAppListHeight, On 2017/06/07 02:31:38, msw wrote: > nit: you should probably replace display_work_area_bounds.height() with > display_work_area_bounds.bottom() here, otherwise this will break if > display_work_area_bounds.y() isn't 0, right? Actually this value is meaningless because y gets set to the proper value in ChangeState before being drawn. I set x and y to 0 and added a comment to explain that the purpose of this line is to set the proper width and height. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:854: if (app_list_state_ == FULLSCREEN) On 2017/06/07 02:31:38, msw wrote: > nit: add a comment like "Update the window's bounds to accommodate the new work > area." Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:44: enum AppListState { INITIAL = 0, PEEKING, FULLSCREEN }; On 2017/06/07 02:31:39, msw wrote: > Comment on this enum and each of its states, the concept of peeking and > fullscreen app list states are new and will be unfamiliar to most readers. Also, > replace INITIAL with CLOSED (or remove INITIAL and initialize |app_list_state_| > to the actual peeking/fullscreen initial state when the view is constructed)? Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:70: void CloseAppList(); On 2017/06/07 02:31:38, msw wrote: > You could potentially replace this with SetState(CLOSED) Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:102: void ToFullscreen(); On 2017/06/07 02:31:39, msw wrote: > Remove these now that we have ChangeState/SetState. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:104: void ChangeState(AppListState new_state); On 2017/06/07 02:31:39, msw wrote: > Rename this SetState Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:106: bool IsFullscreen() const; On 2017/06/07 02:31:39, msw wrote: > Change this to |AppListState app_list_state() const { return app_list_state_; } > You can optionally also/instead do |bool is_fullscreen() const { return > app_list_state_ == FULLSCREEN; } I think having both is a good idea. Thanks! https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:176: int default_peeking_y_ = 0; On 2017/06/07 02:31:39, msw wrote: > Remove this, it's no longer used. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:177: bool is_fullscreen_app_list_ = false; On 2017/06/07 02:31:39, msw wrote: > Remove this, it's no longer used. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:180: bool is_fullscreen_app_list_enabled_ = false; On 2017/06/07 02:31:39, msw wrote: > Move this to a file-local function in cc file's anon namespace, like: > bool IsFullscreenAppListEnabled() { > // Cache this value to avoid repeated lookup. > static bool cached_value = features::IsFullscreenAppListEnabled(); > return cached_value; > } Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:182: gfx::Rect display_work_area_bounds_; On 2017/06/07 02:31:39, msw wrote: > Remove this, it's no longer used. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:185: gfx::Rect fullscreen_widget_bounds_; On 2017/06/07 02:31:39, msw wrote: > Remove this, it's no longer used. Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:186: // The state of the app list, controlled via ChangeState(). On 2017/06/07 02:31:39, msw wrote: > Update ChangeState -> SetState if you accept that rename, or just remove ", > controlled via ChangeState()" Done. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:187: AppListState app_list_state_; On 2017/06/07 02:31:39, msw wrote: > Initialize this value here or in the constructor. Initialized in the constructor. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/sear... File ui/app_list/views/search_box_view.h (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/sear... ui/app_list/views/search_box_view.h:127: const bool is_fullscreen_enabled_; On 2017/06/07 02:31:39, msw wrote: > Move this to a file-local function in cc file's anon namespace, like: > bool IsFullscreenAppListEnabled() { > // Cache this value to avoid repeated lookup. > static bool cached_value = features::IsFullscreenAppListEnabled(); > return cached_value; > } Done.
lgtm with minor comments. I'll look again if you post an update by EOD, but otherwise I'll be out June 8-27. Thanks for addressing all my significant feedback thus far. https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:609: void AppListView::OnMouseEvent(ui::MouseEvent* event) { On 2017/06/07 17:21:37, newcomer wrote: > On 2017/06/07 02:31:38, msw wrote: > > Should OnMouseEvent and OnGestureEvent invoke the base class's implementation > of > > each function in the cases where this subclass doesn't do anything? > Handled offline. For potentially interested readers, the offline discussion was: newcomer: The base class has no implementation of OnMouseEvent or OnGestureEvent, so I don't think it would be useful to do this in this case. msw: ok, that's fine then (it's good to document decisions relevant to the code review) https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:522: if (last_fling_velocity_ > 0) { optional nit: curlies not needed for this if https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:526: if (last_fling_velocity_ > 0) { optional nit: curlies not needed for this if/else or do: SetState(last_fling_velocity_ > 0 ? CLOSED : FULLSCREEN); https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:548: ((app_list_state_ == FULLSCREEN) ? 0 : kPeekingAppListHeight) / nit: parens not needed around |app_list_state_ == FULLSCREEN| https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:560: if (app_list_state_ == FULLSCREEN) { optional nit: curlies not needed for this if/else or do: SetState(app_list_state_ == FULLSCREEN ? PEEKING : CLOSED); https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:706: .height(); q: should this be .bottom() in case the work area has a non-zero y()? https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:719: default: Remove the default case, so adding a new enum value will yield a compile error unless the author also adds handling to this switch statement. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:821: // Set the |fullscreen_widget_| width and height to fit the new display nit: s/width and height/size/ https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:823: fullscreen_widget_->SetBounds( nit: consider using Widget::SetSize, and maybe doing this: gfx::Size size = display::Screen::GetScreen()->GetDisplayNearestView(parent_window()).work_area().size(); size.Enlarge(0, kShelfSize); fullscreen_widget_->SetSize(size); https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:828: if (app_list_state_ == FULLSCREEN) optional nit: SetState(app_list_state_ == FULLSCREEN ? FULLSCREEN: PEEKING); or just do SetState(app_list_state_) (presuming the state isn't closed, or calling SetState(CLOSED) in that case is fine) https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:49: // the shelf by KPeekingAppListHeight DIPs. Lowercase 'k' https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:53: FULLSCREEN optional nit: a trailing comma indicates that it's okay to add new states. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:112: bool IsFullscreen() const { return app_list_state_ == FULLSCREEN; } nit: |is_fullscreen| for inline simple accessors/helpers.
Patchset #16 (id:730001) has been deleted
Addressed the rest of the comments. -Alex https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/670001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:609: void AppListView::OnMouseEvent(ui::MouseEvent* event) { On 2017/06/07 17:44:05, msw wrote: > On 2017/06/07 17:21:37, newcomer wrote: > > On 2017/06/07 02:31:38, msw wrote: > > > Should OnMouseEvent and OnGestureEvent invoke the base class's > implementation > > of > > > each function in the cases where this subclass doesn't do anything? > > Handled offline. > > For potentially interested readers, the offline discussion was: > newcomer: The base class has no implementation of OnMouseEvent or > OnGestureEvent, so I don't think it would be useful to do this in this case. > msw: ok, that's fine then > (it's good to document decisions relevant to the code review) Acknowledged. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:522: if (last_fling_velocity_ > 0) { On 2017/06/07 17:44:06, msw wrote: > optional nit: curlies not needed for this if Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:526: if (last_fling_velocity_ > 0) { On 2017/06/07 17:44:05, msw wrote: > optional nit: curlies not needed for this if/else or do: > SetState(last_fling_velocity_ > 0 ? CLOSED : FULLSCREEN); Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:548: ((app_list_state_ == FULLSCREEN) ? 0 : kPeekingAppListHeight) / On 2017/06/07 17:44:05, msw wrote: > nit: parens not needed around |app_list_state_ == FULLSCREEN| Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:560: if (app_list_state_ == FULLSCREEN) { On 2017/06/07 17:44:05, msw wrote: > optional nit: curlies not needed for this if/else or do: > SetState(app_list_state_ == FULLSCREEN ? PEEKING : CLOSED); Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:706: .height(); On 2017/06/07 17:44:06, msw wrote: > q: should this be .bottom() in case the work area has a non-zero y()? Good catch! ty. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:719: default: On 2017/06/07 17:44:05, msw wrote: > Remove the default case, so adding a new enum value will yield a compile error > unless the author also adds handling to this switch statement. Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:821: // Set the |fullscreen_widget_| width and height to fit the new display On 2017/06/07 17:44:05, msw wrote: > nit: s/width and height/size/ Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:823: fullscreen_widget_->SetBounds( On 2017/06/07 17:44:06, msw wrote: > nit: consider using Widget::SetSize, and maybe doing this: > gfx::Size size = > display::Screen::GetScreen()->GetDisplayNearestView(parent_window()).work_area().size(); > size.Enlarge(0, kShelfSize); > fullscreen_widget_->SetSize(size); I wasn't aware of this. Thanks! Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:828: if (app_list_state_ == FULLSCREEN) On 2017/06/07 17:44:06, msw wrote: > optional nit: SetState(app_list_state_ == FULLSCREEN ? FULLSCREEN: PEEKING); or > just do SetState(app_list_state_) (presuming the state isn't closed, or calling > SetState(CLOSED) in that case is fine) I went with SetState(app_list_state_). https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:49: // the shelf by KPeekingAppListHeight DIPs. On 2017/06/07 17:44:06, msw wrote: > Lowercase 'k' Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:53: FULLSCREEN On 2017/06/07 17:44:06, msw wrote: > optional nit: a trailing comma indicates that it's okay to add new states. Done. https://codereview.chromium.org/2898743002/diff/710001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:112: bool IsFullscreen() const { return app_list_state_ == FULLSCREEN; } On 2017/06/07 17:44:06, msw wrote: > nit: |is_fullscreen| for inline simple accessors/helpers. Done.
https://codereview.chromium.org/2898743002/diff/750001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/750001/ash/shell.cc#newcode619 ash/shell.cc:619: app_list()->set_delegate(nullptr); Document why this this is necessary. In particular, if this is ordering dependent what it depends on. https://codereview.chromium.org/2898743002/diff/750001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2898743002/diff/750001/ash/shell.h#newcode564 ash/shell.h:564: void NotifyAppListVisibilityChanged(bool shown, aura::Window* root_window); The reason there are various NotifyFoo functions here is so other classes can call them. That isn't the case with applist. So, move the implementation of NotifyApplistVisibilityChanged to OnAppListVisibilityChanged and remove NotifyApplistVisibilityChanged.
Patchset #18 (id:790001) has been deleted
Patchset #17 (id:770001) has been deleted
Patchset #17 (id:810001) has been deleted
Patchset #17 (id:830001) has been deleted
I would appreciate any help fixing these tests after merging the mash/ash code paths. -Alex https://codereview.chromium.org/2898743002/diff/750001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2898743002/diff/750001/ash/shell.cc#newcode619 ash/shell.cc:619: app_list()->set_delegate(nullptr); On 2017/06/07 20:21:36, sky wrote: > Document why this this is necessary. In particular, if this is ordering > dependent what it depends on. Done. https://codereview.chromium.org/2898743002/diff/750001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2898743002/diff/750001/ash/shell.h#newcode564 ash/shell.h:564: void NotifyAppListVisibilityChanged(bool shown, aura::Window* root_window); On 2017/06/07 20:21:36, sky wrote: > The reason there are various NotifyFoo functions here is so other classes can > call them. That isn't the case with applist. So, move the implementation of > NotifyApplistVisibilityChanged to OnAppListVisibilityChanged and remove > NotifyApplistVisibilityChanged. Done.
The CQ bit was checked by newcomer@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #17 (id:850001) has been deleted
Patchset #18 (id:890001) has been deleted
The CQ bit was checked by newcomer@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 checked by newcomer@chromium.org to run a CQ dry run
Adressed sky@'s comments and reverted ash/mash merge until after 61. (
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2898743002/diff/930001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/930001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:25: if (app_list::features::IsFullscreenAppListEnabled()) { Please add the TODO pointer to the bug you filed about removing this class.
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 newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org, xiyuan@chromium.org, msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2898743002/#ps950001 (title: "Added todo for deletion of app_list_delegate_impl.cc/h")
Thanks all! -Alex https://codereview.chromium.org/2898743002/diff/930001/ash/app_list/app_list_... File ash/app_list/app_list_delegate_impl.cc (right): https://codereview.chromium.org/2898743002/diff/930001/ash/app_list/app_list_... ash/app_list/app_list_delegate_impl.cc:25: if (app_list::features::IsFullscreenAppListEnabled()) { On 2017/06/09 18:22:02, sky wrote: > Please add the TODO pointer to the bug you filed about removing this class. Done.
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": 950001, "attempt_start_ts": 1497034718170000,
"parent_rev": "e67d603af3065d26e557eb00f964badd71f7d1a3", "commit_rev":
"01b3ff8dd7b21723108b7802164f3a8fa64dabce"}
Message was sent while issue was closed.
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 ========== to ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 Review-Url: https://codereview.chromium.org/2898743002 Cr-Commit-Position: refs/heads/master@{#478372} Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:950001) as https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f...
Message was sent while issue was closed.
On 2017/06/09 19:37:08, commit-bot: I haz the power wrote: > Committed patchset #20 (id:950001) as > https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... It looks like this tickled something and is making a gn check fail: https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg... The following files should be included in gn files: ui/app_list/presenter/app_list_delegate.h ui/app_list/presenter/app_list_presenter_export.h Can someone make a quick change to fix that?
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:950001) has been created in https://codereview.chromium.org/2934673002/ by rdevlin.cronin@chromium.org. The reason for reverting is: Caused gn check failures: Running ['/usr/bin/python', '/b/c/b/linux/src/build/check_gn_headers.py', '--out-dir', u'/b/c/b/linux/src/out/Debug', '--whitelist', '/b/c/b/linux/src/build/check_gn_headers_whitelist.txt', '--json', '/tmp/tmpkLJpof'] in '/b/c/b/linux/src' (env: None) The following files should be included in gn files: ui/app_list/presenter/app_list_delegate.h ui/app_list/presenter/app_list_presenter_export.h e.g. https://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg....
Message was sent while issue was closed.
Patchset #21 (id:970001) has been deleted
Message was sent while issue was closed.
wychen@chromium.org changed reviewers: + wychen@chromium.org
Message was sent while issue was closed.
This CL wrongly passed the CQ because the bot skipped compilation due to wrong analysis results. https://bugs.chromium.org/p/chromium/issues/detail?id=661774
Message was sent while issue was closed.
Patchset #21 (id:990001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 Review-Url: https://codereview.chromium.org/2898743002 Cr-Commit-Position: refs/heads/master@{#478372} Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... ========== to ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 Review-Url: https://codereview.chromium.org/2898743002 Cr-Commit-Position: refs/heads/master@{#478372} Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... ==========
The CQ bit was checked by newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, vadimt@chromium.org, sky@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2898743002/#ps1010001 (title: "Removed header include in shell.h that was causing gn check failure.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Removed header include that was causing gn check failure.
CQ is committing da patch.
Bot data: {"patchset_id": 1010001, "attempt_start_ts": 1497283676789380,
"parent_rev": "2d5c7b158fb963fea6f63a568a610ddf43f72426", "commit_rev":
"2febb9832367123799f3e553274c4449f91a5634"}
Message was sent while issue was closed.
Description was changed from ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 Review-Url: https://codereview.chromium.org/2898743002 Cr-Commit-Position: refs/heads/master@{#478372} Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... ========== to ========== Draggable peeking/fullscreen launcher with transparent background. UI tests are not yet implemented, waiting on final behavior from UX. Enable the new launcher with --enable-features=EnableFullscreenAppList BUG=721781 Review-Url: https://codereview.chromium.org/2898743002 Cr-Original-Commit-Position: refs/heads/master@{#478372} Committed: https://chromium.googlesource.com/chromium/src/+/01b3ff8dd7b21723108b7802164f... Review-Url: https://codereview.chromium.org/2898743002 Cr-Commit-Position: refs/heads/master@{#478665} Committed: https://chromium.googlesource.com/chromium/src/+/2febb9832367123799f3e553274c... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:1010001) as https://chromium.googlesource.com/chromium/src/+/2febb9832367123799f3e553274c... |
