Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939693004:
Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. (Closed)
DescriptionAdded HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH.
|HALF| - Half App List. This mode is
entered when the user types into the search box from
peeking mode.
|FULLSCREEN_ALL_APPS| - Fullscreen app list that
shows all apps. Entered by default in maximize and
side shelf modes. Also entered with an upward swipe
from |PEEKING|.
|FULLSCREEN_SEARCH| - Fullscreen app list that
shows search results. Entered from
FULLSCREEN_ALL_APPS after text is entered in the
search box, or from upward swipe from |HALF|.
Please note: all Mash support will come post 61 as
per crbug.com/726838
BUG=731892
Review-Url: https://codereview.chromium.org/2939693004
Cr-Original-Commit-Position: refs/heads/master@{#479851}
Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879bd3521ba88a7
Review-Url: https://codereview.chromium.org/2939693004
Cr-Commit-Position: refs/heads/master@{#480064}
Committed: https://chromium.googlesource.com/chromium/src/+/f486598b282e1f5399b94b04ca25e7e3bbd12526
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Addressed comments. #
Total comments: 50
Patch Set 4 : Addressed comments. #
Total comments: 3
Patch Set 5 : Rebased. #
Total comments: 2
Patch Set 6 : Addressed comments. #
Total comments: 10
Patch Set 7 : Addressed comments. #Patch Set 8 : Rebased. #Patch Set 9 : Added a return statement for the default case. Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEAR… #Patch Set 10 : Fixed missing return statement after rebase. #Patch Set 11 : rebased post revert. #Messages
Total messages: 94 (69 generated)
Description was changed from ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH app list with state transitions. ========== to ========== Introduced three new app list states: |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALl_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ==========
newcomer@chromium.org changed reviewers: + vadimt@chromium.org
Good morning Vadim, Please take a look before I submit this to sky@ and Xiyuan@ -Alex
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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:102: IsSideShelf(root_window)); Would it be too expensive to calculate root_window in IsSideShelf(), and not pass it as a param? https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:173: return maximize_mode_controller && Can it realistically be null? https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.h:58: bool IsMaximizeModeActive(); const, if possible. Same for all IsXXX methods. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.h:59: // Whether the shelf is oriented on the side or the bottom. // Whether the shelf is oriented on the side, not on the bottom. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate_unittest.cc (left): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:194: TEST_F(AppListPresenterDelegateTest, SnapToFullscreenAfterSearchboxInput) { Why does this test goes away? https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:586: default: It will perhaps be more readable if you list all remaining options instead of "default". https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:641: SetState(HALF); break; https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:809: if (new_state == PEEKING) Please avoid modifying the param. You can create a local var for this. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:45: // Closes |app_list_main_view_| and dismisses the delegate. Good :) https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:121: // |app_list_state|. the current |app_list_state| => app_list_state_ https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:131: // Called from the AppListPresenterDelegate when maximize mode starts and No from, only when. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:198: bool is_maximize_mode_; Initialize here
Patchset #2 (id:20001) has been deleted
Addressed comments! -Alex https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:102: IsSideShelf(root_window)); On 2017/06/13 18:14:10, vadimt wrote: > Would it be too expensive to calculate root_window in IsSideShelf(), and not > pass it as a param? Done. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.cc:173: return maximize_mode_controller && On 2017/06/13 18:14:10, vadimt wrote: > Can it realistically be null? I copied that function from https://cs.chromium.org/chromium/src/ash/system/power/tablet_power_button_con... It's one of the last things initialized in Shell, and it is destroyed in the middle of the destructor. So it's possible that this function is called when the shell is getting destroyed. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.h:58: bool IsMaximizeModeActive(); On 2017/06/13 18:14:10, vadimt wrote: > const, if possible. Same for all IsXXX methods. Done. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate.h:59: // Whether the shelf is oriented on the side or the bottom. On 2017/06/13 18:14:10, vadimt wrote: > // Whether the shelf is oriented on the side, not on the bottom. Done. https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... File ash/app_list/app_list_presenter_delegate_unittest.cc (left): https://codereview.chromium.org/2939693004/diff/1/ash/app_list/app_list_prese... ash/app_list/app_list_presenter_delegate_unittest.cc:194: TEST_F(AppListPresenterDelegateTest, SnapToFullscreenAfterSearchboxInput) { On 2017/06/13 18:14:10, vadimt wrote: > Why does this test goes away? Because this behavior is no longer desired. I'm working on the tests for the new UX and it will be the next CL. (This action should result in |HALF| in certain cases, and |FULLSCREEN_SEARCH| in other cases). https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:586: default: On 2017/06/13 18:14:11, vadimt wrote: > It will perhaps be more readable if you list all remaining options instead of > "default". Done. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:641: SetState(HALF); On 2017/06/13 18:14:10, vadimt wrote: > break; Thanks!! https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.cc:809: if (new_state == PEEKING) On 2017/06/13 18:14:10, vadimt wrote: > Please avoid modifying the param. You can create a local var for this. Done. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:45: // Closes |app_list_main_view_| and dismisses the delegate. On 2017/06/13 18:14:11, vadimt wrote: > Good :) Acknowledged. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:121: // |app_list_state|. On 2017/06/13 18:14:11, vadimt wrote: > the current |app_list_state| => app_list_state_ Done. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:131: // Called from the AppListPresenterDelegate when maximize mode starts and On 2017/06/13 18:14:11, vadimt wrote: > No from, only when. Done. https://codereview.chromium.org/2939693004/diff/1/ui/app_list/views/app_list_... ui/app_list/views/app_list_view.h:198: bool is_maximize_mode_; On 2017/06/13 18:14:11, vadimt wrote: > Initialize here 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...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) 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...)
https://codereview.chromium.org/2939693004/diff/40001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/40001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:807: bool state_overridden = false; You don't need this. Just initialize new_state_override with new_state.
PTAL! -Alex https://codereview.chromium.org/2939693004/diff/40001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/40001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:807: bool state_overridden = false; On 2017/06/13 19:20:31, vadimt wrote: > You don't need this. Just initialize new_state_override with new_state. Done.
lgtm
newcomer@chromium.org changed reviewers: + sky@chromium.org, xiyuan@chromium.org
newcomer@chromium.org changed reviewers: - sky@chromium.org
newcomer@chromium.org changed reviewers: + oshima@chromium.org
PTAL! -Alex
Please insert an empty line between commit message subject and body. And wrap the body lines around 75-80 chars. https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:179: RootWindowController::ForTargetRootWindow()->GetRootWindow(); This does not look right for multi-display case. Think we should get the relevant root window from the widget of |view_|. Something like: RootWindowController::ForWindow( view_->GetWidget()->GetNativeWindow())->GetRootWindow(); https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:40: public ShelfObserver { Is ShelfObserver used? https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:58: bool IsMaximizeModeActive() const; This and IsSideShelf() can go into cc file in anonymous namespace. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (left): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:528: last_fling_velocity_ = 0; Who is going to reset |last_fling_velocity_| now? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:598: SetState(app_list_state_); Why do we need to call SetState with the same state? Wounldn't this be a no-op? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:664: void AppListView::OnMaximizeModeChanged(bool started) { |started| is different from what is in the header file. |shown| was used there. Please keep them consistent. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:805: gfx::Rect new_widget_bounds = fullscreen_widget_->GetWindowBoundsInScreen(); nit: move this down to line 815, closer to where it is first used. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:815: nit: Should we bail if new_state_override == app_list_state_ ? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:825: .bottom(); nit: move this out of the switch so that it could be shared with line 843. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:825: .bottom(); Should we use height() instead of bottom() to get |display_height| ? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:838: } break; get rid of the extra "break" https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:70: bool is_mazimize_mode = false, Avoid using default arguments. Either update all call sites or set the two bools separately if they are not really used in Initialize.
Patchset #4 (id:80001) has been deleted
Responded to Xiyuan@ 's comments. -Alex https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:179: RootWindowController::ForTargetRootWindow()->GetRootWindow(); On 2017/06/13 21:38:58, xiyuan wrote: > This does not look right for multi-display case. Think we should get the > relevant root window from the widget of |view_|. > > Something like: > > RootWindowController::ForWindow( > view_->GetWidget()->GetNativeWindow())->GetRootWindow(); Done. https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:40: public ShelfObserver { On 2017/06/13 21:38:58, xiyuan wrote: > Is ShelfObserver used? no, thanks! https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:58: bool IsMaximizeModeActive() const; On 2017/06/13 21:38:58, xiyuan wrote: > This and IsSideShelf() can go into cc file in anonymous namespace. Done. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (left): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:528: last_fling_velocity_ = 0; On 2017/06/13 21:38:58, xiyuan wrote: > Who is going to reset |last_fling_velocity_| now? Assuming that a ui::ET_GESTURE_END event must always be preceded with a UI::ET_GESTURE_SCROLL_UPDATE, the |last_fling_velocity_| doesn't need to be reset because it is set in line 738. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:598: SetState(app_list_state_); On 2017/06/13 21:38:59, xiyuan wrote: > Why do we need to call SetState with the same state? Wounldn't this be a no-op? The widget gets re-positioned as the drag events are being received. If the drag action fails to transition states then the widget must be reset from it's intermediate state to the correct y for its current state. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:664: void AppListView::OnMaximizeModeChanged(bool started) { On 2017/06/13 21:38:58, xiyuan wrote: > |started| is different from what is in the header file. |shown| was used there. > Please keep them consistent. Done! https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:805: gfx::Rect new_widget_bounds = fullscreen_widget_->GetWindowBoundsInScreen(); On 2017/06/13 21:38:58, xiyuan wrote: > nit: move this down to line 815, closer to where it is first used. Done. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:815: On 2017/06/13 21:38:58, xiyuan wrote: > nit: Should we bail if new_state_override == app_list_state_ ? No, because sometimes it is useful to re-set the current state. For example, if a drag gesture completes but the touch point is not far enough to qualify for a state transition, we call SetState(app_list_state_) in order to set the widget back in the proper position. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:825: .bottom(); On 2017/06/13 21:38:58, xiyuan wrote: > Should we use height() instead of bottom() to get |display_height| ? msw@ recommended we use bottom() to avoid a case where height() could be 0. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:825: .bottom(); On 2017/06/13 21:38:59, xiyuan wrote: > nit: move this out of the switch so that it could be shared with line 843. Done. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:838: } break; On 2017/06/13 21:38:58, xiyuan wrote: > get rid of the extra "break" Done. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:70: bool is_mazimize_mode = false, On 2017/06/13 21:38:59, xiyuan wrote: > Avoid using default arguments. Either update all call sites or set the two bools > separately if they are not really used in Initialize. Done. https://codereview.chromium.org/2939693004/diff/100001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/100001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:62: bool IsSideShelf(aura::Window* root_window) { I'm passing root_window here because calling GetNativeWindow before initializing the AppListView results in a crash. https://codereview.chromium.org/2939693004/diff/100001/ui/app_list/presenter/... File ui/app_list/presenter/app_list_presenter_impl_unittest.cc (right): https://codereview.chromium.org/2939693004/diff/100001/ui/app_list/presenter/... ui/app_list/presenter/app_list_presenter_impl_unittest.cc:47: view->Initialize(container_, current_apps_page, false, false); This will be replaced with test functions for IsSideShelf and IsMaximizeMode in the next cl. https://codereview.chromium.org/2939693004/diff/100001/ui/app_list/views/app_... File ui/app_list/views/app_list_view_unittest.cc (right): https://codereview.chromium.org/2939693004/diff/100001/ui/app_list/views/app_... ui/app_list/views/app_list_view_unittest.cc:175: view_->Initialize(parent, 0, false, false); This will be replaced with test functions for IsSideShelf and IsMaximizeMode in the next CL.
Description was changed from ========== Introduced three new app list states: |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALl_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ========== to ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ==========
Description was changed from ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ========== to ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ==========
could you please add unit tests? https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:173: return maximize_mode_controller && maximize_mode_controller should never be null. This is simple enough that you can just inline this. https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:188: } don't you need return? https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:74: void OnShelfIconPositionChanged(); override? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; Where this half value came from? Won't it be too small for portrait for example? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:231: is_maximize_mode_ = is_maximize_mode; I'd recommend to just use the maximize_mode_controller to check the state instead of copying the state and maintain it. same for is_side_shelf_ https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:529: fullscreen_widget_->GetWindowBoundsInScreen().y(); is location in screen coordinates? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:623: display_height - kAppListBezelMargin) { I didn't fully understand this. why this isn't location.y() >= bezel y? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:121: // app_list_state_. document |empty| argument.
Updated CL Description.
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
PTAL! In regards to unit tests, I would like to land this CL first (it's behind a flag and won't break anything) because we have two SWE's working on specs and they need this up asap. I'm planning on submitting a CL for unit tests after this lands. -Alex https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:173: return maximize_mode_controller && On 2017/06/13 22:26:04, oshima wrote: > maximize_mode_controller should never be null. > > This is simple enough that you can just inline this. Thanks! Done. https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:188: } On 2017/06/13 22:26:04, oshima wrote: > don't you need return? The return statements are in the switch. I refactored it so it is clearer. https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.h (right): https://codereview.chromium.org/2939693004/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.h:74: void OnShelfIconPositionChanged(); On 2017/06/13 22:26:04, oshima wrote: > override? Function removed as a side effect of xiyuan@'s comment. But correct, thanks! https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; On 2017/06/13 22:26:04, oshima wrote: > Where this half value came from? Won't it be too small for portrait for example? This value came from the Spec, I just sent it to you. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:231: is_maximize_mode_ = is_maximize_mode; On 2017/06/13 22:26:04, oshima wrote: > I'd recommend to just use the maximize_mode_controller to check the state > instead of copying the state and maintain it. > > same for is_side_shelf_ Unfortunately the MaximizeModeController is in ash and I cannot include anything from ash in ui because ui is below ash in the dependency graph. Maybe this can happen after we move ui/app_list into ash/app_list in the future. Until this happens, I'm using ShellObserver overrides in the AppListPresenterDelegate to notify AppListView when the maximize mode changes. As for side shelf, changing the shelf orientation causes the AppListView to get destroyed. So it should be sufficient to only update is_side_shelf_ on initialization. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:529: fullscreen_widget_->GetWindowBoundsInScreen().y(); On 2017/06/13 22:26:04, oshima wrote: > is location in screen coordinates? yes! https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:623: display_height - kAppListBezelMargin) { On 2017/06/13 22:26:04, oshima wrote: > I didn't fully understand this. why this isn't location.y() >= bezel y? Because location.y() would be in widget coordinates, we need screen coordinates here. Should I add a comment or create a variable for |screen_coordinates| for readability? https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:121: // app_list_state_. On 2017/06/13 22:26:04, oshima wrote: > document |empty| argument. I changed the variable to |search_box_is_empty|, I thought this would be more informative over-all. Please let me know if you still think a comment would be helpful. https://codereview.chromium.org/2939693004/diff/140001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/140001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:62: bool IsSideShelf(aura::Window* root_window) { I'm passing root_window here because calling GetNativeWindow before initializing the AppListView results in a crash. https://codereview.chromium.org/2939693004/diff/140001/ui/app_list/presenter/... File ui/app_list/presenter/app_list_presenter_impl_unittest.cc (right): https://codereview.chromium.org/2939693004/diff/140001/ui/app_list/presenter/... ui/app_list/presenter/app_list_presenter_impl_unittest.cc:47: view->Initialize(container_, current_apps_page, false, false); This will be replaced with test functions for IsSideShelf and IsMaximizeMode in the next cl. https://codereview.chromium.org/2939693004/diff/140001/ui/app_list/views/app_... File ui/app_list/views/app_list_view_unittest.cc (right): https://codereview.chromium.org/2939693004/diff/140001/ui/app_list/views/app_... ui/app_list/views/app_list_view_unittest.cc:175: view_->Initialize(parent, 0, false, false); This will be replaced with test functions for IsSideShelf and IsMaximizeMode in the next CL.
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-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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...)
https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; On 2017/06/13 23:00:39, newcomer wrote: > On 2017/06/13 22:26:04, oshima wrote: > > Where this half value came from? Won't it be too small for portrait for > example? > > This value came from the Spec, I just sent it to you. It looks to me that this height includes the shelf side, but the code below excludes it, when the shelf is on the bottom side. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:231: is_maximize_mode_ = is_maximize_mode; On 2017/06/13 23:00:39, newcomer wrote: > On 2017/06/13 22:26:04, oshima wrote: > > I'd recommend to just use the maximize_mode_controller to check the state > > instead of copying the state and maintain it. > > > > same for is_side_shelf_ > > Unfortunately the MaximizeModeController is in ash and I cannot include anything > from ash in ui because ui is below ash in the dependency graph. Maybe this can > happen after we move ui/app_list into ash/app_list in the future. > > Until this happens, I'm using ShellObserver overrides in the > AppListPresenterDelegate to notify AppListView when the maximize mode changes. > > As for side shelf, changing the shelf orientation causes the AppListView to get > destroyed. So it should be sufficient to only update is_side_shelf_ on > initialization. > Ack. I believe the app list has been moved to ui/app_list because this was used on other platforms as well. Since they're gone, we can move back to ash/. Can you file a bug to move it? I think it makes sense now. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:529: fullscreen_widget_->GetWindowBoundsInScreen().y(); On 2017/06/13 22:26:04, oshima wrote: > is location in screen coordinates? Looks like this is from event, which means it's ot screen coordinates. This works because you're using just diff from initial point. This also explains my question below. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:623: display_height - kAppListBezelMargin) { On 2017/06/13 23:00:39, newcomer wrote: > On 2017/06/13 22:26:04, oshima wrote: > > I didn't fully understand this. why this isn't location.y() >= bezel y? > > Because location.y() would be in widget coordinates, we need screen coordinates > here. Should I add a comment or create a variable for |screen_coordinates| for > readability? Yes, please create one and use wm::ConvertPointToScreen to convert.
lgtm with a style nit https://codereview.chromium.org/2939693004/diff/160001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/160001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:833: } break; This style is wrong. Prevailing style is either put it inside {}, e.g. ... break; } or outside but align with closing } e.g. ... } break;
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
Thanks and PTAL! -Alex https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; On 2017/06/14 00:37:34, oshima wrote: > On 2017/06/13 23:00:39, newcomer wrote: > > On 2017/06/13 22:26:04, oshima wrote: > > > Where this half value came from? Won't it be too small for portrait for > > example? > > > > This value came from the Spec, I just sent it to you. > > It looks to me that this height includes the shelf side, but the code below > excludes it, when the shelf is on the bottom side. Fixed! thanks. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:231: is_maximize_mode_ = is_maximize_mode; On 2017/06/14 00:37:33, oshima wrote: > On 2017/06/13 23:00:39, newcomer wrote: > > On 2017/06/13 22:26:04, oshima wrote: > > > I'd recommend to just use the maximize_mode_controller to check the state > > > instead of copying the state and maintain it. > > > > > > same for is_side_shelf_ > > > > Unfortunately the MaximizeModeController is in ash and I cannot include > anything > > from ash in ui because ui is below ash in the dependency graph. Maybe this can > > happen after we move ui/app_list into ash/app_list in the future. > > > > Until this happens, I'm using ShellObserver overrides in the > > AppListPresenterDelegate to notify AppListView when the maximize mode changes. > > > > > As for side shelf, changing the shelf orientation causes the AppListView to > get > > destroyed. So it should be sufficient to only update is_side_shelf_ on > > initialization. > > > > Ack. I believe the app list has been moved to ui/app_list because this was used > on other platforms as well. Since they're gone, we can move back to ash/. > > Can you file a bug to move it? I think it makes sense now. Done! crbug.com/733662 https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:529: fullscreen_widget_->GetWindowBoundsInScreen().y(); On 2017/06/14 00:37:34, oshima wrote: > On 2017/06/13 22:26:04, oshima wrote: > > is location in screen coordinates? > > Looks like this is from event, which means it's ot screen coordinates. This > works because you're using just diff from initial point. > > This also explains my question below. Ah, you're correct. I misinterpreted your question. location is from event and is therefore not in screen coordinates, new_y_position is in screen coordinates. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:623: display_height - kAppListBezelMargin) { On 2017/06/14 00:37:34, oshima wrote: > On 2017/06/13 23:00:39, newcomer wrote: > > On 2017/06/13 22:26:04, oshima wrote: > > > I didn't fully understand this. why this isn't location.y() >= bezel y? > > > > Because location.y() would be in widget coordinates, we need screen > coordinates > > here. Should I add a comment or create a variable for |screen_coordinates| for > > readability? > > Yes, please create one and use wm::ConvertPointToScreen to convert. > > Done. https://codereview.chromium.org/2939693004/diff/160001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/160001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:833: } break; On 2017/06/14 17:02:58, xiyuan wrote: > This style is wrong. Prevailing style is > > either put it inside {}, > e.g. > ... > break; > } > > or outside but align with closing } > e.g. > ... > } > break; git cl format kept resetting the break to the incorrect style, I think because I had the break statement outside of the brackets. I just moved the break inside the brackets.
In general, you should add a test when you add new behavior, otherwise someone else may break it. Please add test as soon as you landed this if you still want to land separately. https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; On 2017/06/15 16:55:33, newcomer wrote: > On 2017/06/14 00:37:34, oshima wrote: > > On 2017/06/13 23:00:39, newcomer wrote: > > > On 2017/06/13 22:26:04, oshima wrote: > > > > Where this half value came from? Won't it be too small for portrait for > > > example? > > > > > > This value came from the Spec, I just sent it to you. > > > > It looks to me that this height includes the shelf side, but the code below > > excludes it, when the shelf is on the bottom side. > > Fixed! thanks. Looks like this isn't fixed yet? Please see my comment below. https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:68: is_side_shelf = false; nit: just return false, true in swich. no need to define is_side_shelf. https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:75: return is_side_shelf; then return false; https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:64: // The height of the half app list. .. from the bottom of the screen, not workarea. https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:564: .height(); This is workarea height, which excludes the shelf. What you need is size().height(); https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:67: // used for both the bubble and fullscreen initialization. document new arguments
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Oshima, Will do! -Alex https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:65: constexpr int kHalfAppListHeight = 561; On 2017/06/15 17:51:44, oshima wrote: > On 2017/06/15 16:55:33, newcomer wrote: > > On 2017/06/14 00:37:34, oshima wrote: > > > On 2017/06/13 23:00:39, newcomer wrote: > > > > On 2017/06/13 22:26:04, oshima wrote: > > > > > Where this half value came from? Won't it be too small for portrait for > > > > example? > > > > > > > > This value came from the Spec, I just sent it to you. > > > > > > It looks to me that this height includes the shelf side, but the code below > > > excludes it, when the shelf is on the bottom side. > > > > Fixed! thanks. > > Looks like this isn't fixed yet? Please see my comment below. Done. https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:68: is_side_shelf = false; On 2017/06/15 17:51:45, oshima wrote: > nit: just return false, true in swich. no need to define is_side_shelf. Done. https://codereview.chromium.org/2939693004/diff/240001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate.cc:75: return is_side_shelf; On 2017/06/15 17:51:45, oshima wrote: > then return false; Moved all return statements into the switch statement. https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:64: // The height of the half app list. On 2017/06/15 17:51:45, oshima wrote: > .. from the bottom of the screen, not workarea. Done. https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:564: .height(); On 2017/06/15 17:51:45, oshima wrote: > This is workarea height, which excludes the shelf. What you need is > size().height(); > That makes more sense. Thank you! https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2939693004/diff/240001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:67: // used for both the bubble and fullscreen initialization. On 2017/06/15 17:51:45, oshima wrote: > document new arguments Done.
Patchset #8 (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...
lgtm
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 Link to the patchset: https://codereview.chromium.org/2939693004/#ps330001 (title: "Rebased.")
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": 330001, "attempt_start_ts": 1497562892091310,
"parent_rev": "9a68476194ad9536b44b42d3cdd6fce7ee8581da", "commit_rev":
"a1201ceb12bfedf51cb1c0e01879bd3521ba88a7"}
Message was sent while issue was closed.
Description was changed from ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 ========== to ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 Review-Url: https://codereview.chromium.org/2939693004 Cr-Commit-Position: refs/heads/master@{#479851} Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:330001) as https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:330001) has been created in https://codereview.chromium.org/2938203004/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 479851 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
Description was changed from ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 Review-Url: https://codereview.chromium.org/2939693004 Cr-Commit-Position: refs/heads/master@{#479851} Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879... ========== to ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 Review-Url: https://codereview.chromium.org/2939693004 Cr-Commit-Position: refs/heads/master@{#479851} Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879... ==========
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, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2939693004/#ps350001 (title: "Added a return statement for the default case. Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEAR…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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...
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-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...)
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...)
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-...)
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.
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, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2939693004/#ps390001 (title: "rebased post revert.")
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": 390001, "attempt_start_ts": 1497629686301880,
"parent_rev": "76c5c65fb7cf6495ab6c391d0e9b452fc4222b07", "commit_rev":
"f486598b282e1f5399b94b04ca25e7e3bbd12526"}
Message was sent while issue was closed.
Description was changed from ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 Review-Url: https://codereview.chromium.org/2939693004 Cr-Commit-Position: refs/heads/master@{#479851} Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879... ========== to ========== Added HALF, FULLSCREEN_ALL_APPS, and FULLSCREEN_SEARCH. |HALF| - Half App List. This mode is entered when the user types into the search box from peeking mode. |FULLSCREEN_ALL_APPS| - Fullscreen app list that shows all apps. Entered by default in maximize and side shelf modes. Also entered with an upward swipe from |PEEKING|. |FULLSCREEN_SEARCH| - Fullscreen app list that shows search results. Entered from FULLSCREEN_ALL_APPS after text is entered in the search box, or from upward swipe from |HALF|. Please note: all Mash support will come post 61 as per crbug.com/726838 BUG=731892 Review-Url: https://codereview.chromium.org/2939693004 Cr-Original-Commit-Position: refs/heads/master@{#479851} Committed: https://chromium.googlesource.com/chromium/src/+/a1201ceb12bfedf51cb1c0e01879... Review-Url: https://codereview.chromium.org/2939693004 Cr-Commit-Position: refs/heads/master@{#480064} Committed: https://chromium.googlesource.com/chromium/src/+/f486598b282e1f5399b94b04ca25... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:390001) as https://chromium.googlesource.com/chromium/src/+/f486598b282e1f5399b94b04ca25... |
