|
|
DescriptionImplementation of a full screen app list behind a switch (as opposed to a feature flag).
Also re-alphabetized switches in ash_switches.cc
BUG=711349
Review-Url: https://codereview.chromium.org/2802903003
Cr-Original-Commit-Position: refs/heads/master@{#467088}
Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d87fd4f5d5c663
Review-Url: https://codereview.chromium.org/2802903003
Cr-Commit-Position: refs/heads/master@{#467496}
Committed: https://chromium.googlesource.com/chromium/src/+/145b24f9355dc8a7c59bfb93f3634eef1d555784
Patch Set 1 #Patch Set 2 : Cleaned up some old variables and spacing changes #
Total comments: 19
Patch Set 3 : Refactored InitAsBubble, InitAsFrameless into InitializeWindow and moved the switch to be under the… #Patch Set 4 : fixed some formatting/whitespace errors. #
Total comments: 17
Patch Set 5 : Addressed the issues that were brought up. #
Total comments: 10
Patch Set 6 : addressed comments, refactored Initialize, modified tests to add test coverage for the fullscreen l… #
Total comments: 22
Patch Set 7 : Addressed comments, added my tests to the filter, and refactored! #
Total comments: 9
Patch Set 8 : Un-parameterized a bubble test, addressed comments, fixed merge conflicts, and fixed a typo! #
Total comments: 16
Patch Set 9 : Addressed comments. #Patch Set 10 : Fixed missing refactoring of the Set function. #Patch Set 11 : Noob mistake, forgot to change the namespace of the switch after I moved it #Patch Set 12 : Fixed the mishandled reference that was breaking the build #Messages
Total messages: 134 (95 generated)
Description was changed from ========== reverted accidentally modified file got rid of the feature flag added switches to ash/common to avoid the dependency issue put the fullscreen launcher behind a flag. added a line to make sure the layer is not opaque moved overlay_view_ = new ... and AddChildView to below the widget initializations to attempt to fix the rendering issues accessed the widget after the view is assigned to it instead of before to avoid nullptr! fullscreen app list commit 1 BUG= ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). BUG= ==========
newcomer@chromium.org changed reviewers: + sky@chromium.org, vadimt@chromium.org
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). BUG= ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG= ==========
Please review my changes! I implemented a full screen app list(behind a switch) for the upcoming ux changes, I also alphabetized the switches in ash_switches.h (it was a TODO for sky).
You should also add test coverage of this. I suggest changing making the relevant tests peramterized so you run the tests against both cases. https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:103: if (CheckFullscreenAppListEnabled()) { IsFullscreenAppListEnabled()? Or perhaps ShouldCreateFullScreenWindow(). https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:189: return base::CommandLine::ForCurrentProcess()->HasSwitch( This doesn't use any state from AppListPresenterDelegate. Prefer a standalone non-member function in this case (in the anonymous namespace at the top of the file). https://codereview.chromium.org/2802903003/diff/20001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ash/common/ash_switches... ash/common/ash_switches.cc:51: const char kAshEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; Reorder all these as well (style guide says declaration and definition orders should match). https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (left): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:87: layer()->SetOpacity(0.0f); Why are you removing this? https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:184: blurred(false) { Does this compile? https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:197: void AppListView::InitAsFramelessWindow(gfx::NativeView parent, Please refactor this and InitAsBubble to share code. In particular I'm assuming the accelerator, targeter and other functionality should be shared. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:202: Only one newline is fine. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:218: void AppListView::InitAsBubble(gfx::NativeView parent, int initial_apps_page) { newline between 217/218. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:37: const char* GetClassName() const override; Group overrides of the same type together (in other words move this to around line 78ish). Also, style guide says constructor/destructor is before other functions. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:49: gfx::Rect bounds); const gfx::Rect& bounds
Thanks for your initial comments! I did some refactoring as asked although you may have a better idea of how to handle the function that is calling InitAsBubble in another part of chromeOS. Thanks, Alex https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:103: if (CheckFullscreenAppListEnabled()) { On 2017/04/07 03:28:18, sky wrote: > IsFullscreenAppListEnabled()? Or perhaps ShouldCreateFullScreenWindow(). Done. https://codereview.chromium.org/2802903003/diff/20001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:189: return base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/04/07 03:28:18, sky wrote: > This doesn't use any state from AppListPresenterDelegate. Prefer a standalone > non-member function in this case (in the anonymous namespace at the top of the > file). Done. https://codereview.chromium.org/2802903003/diff/20001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ash/common/ash_switches... ash/common/ash_switches.cc:51: const char kAshEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; On 2017/04/07 03:28:18, sky wrote: > Reorder all these as well (style guide says declaration and definition orders > should match). Done. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (left): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:87: layer()->SetOpacity(0.0f); On 2017/04/07 03:28:18, sky wrote: > Why are you removing this? That was a mistake! Fixed. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:184: blurred(false) { On 2017/04/07 03:28:19, sky wrote: > Does this compile? It didn't compile. I added that when doing research on the codebase and accidentally put it in the CL. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:197: void AppListView::InitAsFramelessWindow(gfx::NativeView parent, On 2017/04/07 03:28:18, sky wrote: > Please refactor this and InitAsBubble to share code. In particular I'm assuming > the accelerator, targeter and other functionality should be shared. I refactored it, then noticed there was a function calling InitAsBubble ( see the most recent patch please for full explanation ) https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:202: On 2017/04/07 03:28:18, sky wrote: > Only one newline is fine. Acknowledged. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:37: const char* GetClassName() const override; On 2017/04/07 03:28:19, sky wrote: > Group overrides of the same type together (in other words move this to around > line 78ish). Also, style guide says constructor/destructor is before other > functions. Done. https://codereview.chromium.org/2802903003/diff/20001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:49: gfx::Rect bounds); On 2017/04/07 03:28:19, sky wrote: > const gfx::Rect& bounds Done.
Published a few comments. -Alex https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:105: GetMinimumBoundsHeightForAppList(view)), Moved view->SetAnchorPoint into the Initialize Window function ( we only do that if it's a bubble). https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:131: } Moved to anonymous namespace
https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:102: view->InitializeWindow( Please talk to Now people (I'll provide contacts) on how to test your change with Google Now in Launcher. https://codereview.chromium.org/2802903003/diff/60001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/common/ash_switches... ash/common/ash_switches.cc:55: const char kAshEnableMirroredScreen[] = "ash-enable-mirrored-screen"; Good Samaritan! :) https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:26: extern const char kEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; Please talk to UX/PM whether this is a self-contained change, if so, you don't need the flag. NOT having a flag is in a way better because the change starts testing on dogfooders immediately. But please, in advance, check that sky@ won't have problems with NOT having a flag. https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:220: widget->GetLayer()->SetBackgroundBlur(20); Please check this value with UX. https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:41: // Initialized the widget with a frame or a frameless view depending on Initializes https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:43: void InitializeWindow(gfx::NativeView parent, int initial_apps_page, Just "Initialize"? https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:47: // Initializes the widget as a bubble. Integrated into InitializeWindow but kept around for it's Too long line? Please check with the style guide. it's => its
https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:26: extern const char kEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; On 2017/04/10 19:40:10, vadimt wrote: > Please talk to UX/PM whether this is a self-contained change, if so, you don't > need the flag. > NOT having a flag is in a way better because the change starts testing on > dogfooders immediately. > But please, in advance, check that sky@ won't have problems with NOT having a > flag. +1 to no flag.
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 newcomer@chromium.org
Please create a bug and refer it from the CL. Thanks!
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG= ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=711323 ==========
On 2017/04/12 17:44:59, vadimt wrote: > Please create a bug and refer it from the CL. Thanks! Added a bug! https://bugs.chromium.org/p/chromium/issues/detail?id=711323
The CQ bit was checked by newcomer@google.com 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_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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_...) 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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=711323 ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=711349 ==========
newcomer@google.com changed reviewers: + newcomer@chromium.org - newcomer@google.com
On 2017/04/13 15:44:13, newcomer1 wrote: > On 2017/04/12 17:44:59, vadimt wrote: > > Please create a bug and refer it from the CL. Thanks! > > Added a bug! > > https://bugs.chromium.org/p/chromium/issues/detail?id=711323 Added a bug that is more typical for features. https://bugs.chromium.org/p/chromium/issues/detail?id=711349
Addressed the comments! I am waiting for Shiba to get back to me with the specific blur setting.
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...
Updated with replies to comments! Not sure how they got left off last time. https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:102: view->InitializeWindow( On 2017/04/10 19:40:10, vadimt wrote: > Please talk to Now people (I'll provide contacts) on how to test your change > with Google Now in Launcher. We are waiting to hear back from travis. https://codereview.chromium.org/2802903003/diff/60001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/common/ash_switches... ash/common/ash_switches.cc:55: const char kAshEnableMirroredScreen[] = "ash-enable-mirrored-screen"; On 2017/04/10 19:40:10, vadimt wrote: > Good Samaritan! :) Thanks! https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:26: extern const char kEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; On 2017/04/10 20:44:29, sky (OOO) wrote: > On 2017/04/10 19:40:10, vadimt wrote: > > Please talk to UX/PM whether this is a self-contained change, if so, you don't > > need the flag. > > NOT having a flag is in a way better because the change starts testing on > > dogfooders immediately. > > But please, in advance, check that sky@ won't have problems with NOT having a > > flag. > > +1 to no flag. Vadimt and I talked offline about this and decided that the change is too extreme to introduce without a flag (for now). https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:220: widget->GetLayer()->SetBackgroundBlur(20); On 2017/04/10 19:40:10, vadimt wrote: > Please check this value with UX. Waiting on our ux people for this. https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:41: // Initialized the widget with a frame or a frameless view depending on On 2017/04/10 19:40:10, vadimt wrote: > Initializes fixed https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:43: void InitializeWindow(gfx::NativeView parent, int initial_apps_page, On 2017/04/10 19:40:10, vadimt wrote: > Just "Initialize"? Done. https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:47: // Initializes the widget as a bubble. Integrated into InitializeWindow but kept around for it's On 2017/04/10 19:40:10, vadimt wrote: > Too long line? Please check with the style guide. > > it's => its It now comply's with the 80 character per line limit.
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_...)
What about tests sky@ requested? https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:106: ash::ScreenUtil::GetDisplayWorkAreaBoundsInParent(container)); Please fix indentation https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:25: // Enables the fullscreen app list add '.' https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:127: // Checks to see if the fullscreen app list has been enabled via cmd line switch add '.' https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:224: AddChildView(overlay_view_); Would it be possible to move this out of 'if'? https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:44: const gfx::Point ¢er_of_display_window, Please fix indentation... actually, here and everywhere else... (in your changes :) )
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate.cc:106: ash::ScreenUtil::GetDisplayWorkAreaBoundsInParent(container)); On 2017/04/13 21:48:34, vadimt wrote: > Please fix indentation Done. https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/app_list_sw... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/app_list_sw... ui/app_list/app_list_switches.cc:25: // Enables the fullscreen app list On 2017/04/13 21:48:34, vadimt wrote: > add '.' Done. https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:127: // Checks to see if the fullscreen app list has been enabled via cmd line switch On 2017/04/13 21:48:34, vadimt wrote: > add '.' Done. https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.cc:224: AddChildView(overlay_view_); On 2017/04/13 21:48:34, vadimt wrote: > Would it be possible to move this out of 'if'? I refactored this, and we talked about duplicated code offline. https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/80001/ui/app_list/views/app_l... ui/app_list/views/app_list_view.h:44: const gfx::Point ¢er_of_display_window, On 2017/04/13 21:48:34, vadimt wrote: > Please fix indentation... actually, here and everywhere else... (in your changes > :) ) Done. Thanks for showing me git cl format !!
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) 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 #6 (id:200001) has been deleted
The CL is now ready for review! -Alex
Some comments; more may follow in future iterations. https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { You can define it in app_list_switches.h and use everywhere. https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:41: public ::testing::WithParamInterface<bool> { Do we need that '::' before 'testing'? https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:127: // Checks if the fullscreen app list has been enabled via cmd line switch. Returns whether ... https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:310: void AppListView::SetAnchorPoint(const gfx::Point& anchor_point) { MaybeSetAnchorPoint https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:41: // Helper function for Initialize, used when there is a flag. We use 'helper' in class names, not method names. Please rename, and change comments from "where it's used from" to "what it does".
https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:70: bool testWithFullscreen = GetParam(); test_with_fullscreen That said, why don't you configre this in SetUp() so you don't need it in each test? https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:224: InitContents(parent, initial_apps_page); There is a lot of similar code in the two functions. Please refactor better to share code. For example, calling ViewInititialized, adding the overlay_view_, InitChildWidgets... https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:42: void InitializeFullscreenHelper(gfx::NativeView parent, Document what the parameters meant. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:44: const gfx::Rect display_work_area_bounds); const gfx::Rect& here and 53. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:47: void InitializeBubbleHelper(gfx::NativeView parent, int initial_apps_page); Why do you need the InitializeHelper functions to be public? Shouldn't callers only call Initialize? Also, why do you need InitAsBubble? Can there be *one* public init function? It can take an enum indicating the mode.
Thanks for the fast feedback! -Alex On Wed, Apr 19, 2017 at 8:18 AM, <sky@chromium.org> wrote: > > https://codereview.chromium.org/2802903003/diff/220001/ > ash/app_list/app_list_presenter_delegate_unittest.cc > File ash/app_list/app_list_presenter_delegate_unittest.cc (right): > > https://codereview.chromium.org/2802903003/diff/220001/ > ash/app_list/app_list_presenter_delegate_unittest.cc#newcode70 > ash/app_list/app_list_presenter_delegate_unittest.cc:70: bool > testWithFullscreen = GetParam(); > test_with_fullscreen That said, why don't you configre this in SetUp() > so you don't need it in each test? > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.cc > File ui/app_list/views/app_list_view.cc (right): > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.cc#newcode224 > ui/app_list/views/app_list_view.cc:224: InitContents(parent, > initial_apps_page); > There is a lot of similar code in the two functions. Please refactor > better to share code. For example, calling ViewInititialized, adding the > overlay_view_, InitChildWidgets... > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.h > File ui/app_list/views/app_list_view.h (right): > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.h#newcode42 > ui/app_list/views/app_list_view.h:42: void > InitializeFullscreenHelper(gfx::NativeView parent, > Document what the parameters meant. > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.h#newcode44 > ui/app_list/views/app_list_view.h:44: const gfx::Rect > display_work_area_bounds); > const gfx::Rect& here and 53. > > https://codereview.chromium.org/2802903003/diff/220001/ui/ > app_list/views/app_list_view.h#newcode47 > ui/app_list/views/app_list_view.h:47: void > InitializeBubbleHelper(gfx::NativeView parent, int initial_apps_page); > Why do you need the InitializeHelper functions to be public? Shouldn't > callers only call Initialize? Also, why do you need InitAsBubble? Can > there be *one* public init function? It can take an enum indicating the > mode. > > https://codereview.chromium.org/2802903003/ > -- 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 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_...)
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=711349 ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 ==========
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 #8 (id:260001) 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 #7 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, Alex https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { On 2017/04/19 00:57:15, vadimt wrote: > You can define it in app_list_switches.h and use everywhere. Done. https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:41: public ::testing::WithParamInterface<bool> { On 2017/04/19 00:57:15, vadimt wrote: > Do we need that '::' before 'testing'? Done. https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:70: bool testWithFullscreen = GetParam(); On 2017/04/19 15:18:51, sky wrote: > test_with_fullscreen That said, why don't you configre this in SetUp() so you > don't need it in each test? Done! Check SetUp(). https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:127: // Checks if the fullscreen app list has been enabled via cmd line switch. On 2017/04/19 00:57:15, vadimt wrote: > Returns whether ... Done. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:224: InitContents(parent, initial_apps_page); On 2017/04/19 15:18:51, sky wrote: > There is a lot of similar code in the two functions. Please refactor better to > share code. For example, calling ViewInititialized, adding the overlay_view_, > InitChildWidgets... I've refactored the repeated code. Do you have an idea on how to handle the way that I have overridden Initialize(the old InitAsBubble) when it is called with only two parameters? I couldn't default a const reference(as per the style guide) so I decided on overriding. This means I had to repeat some code. Definitely interested on your thoughts here. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:310: void AppListView::SetAnchorPoint(const gfx::Point& anchor_point) { On 2017/04/19 00:57:15, vadimt wrote: > MaybeSetAnchorPoint Done. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:41: // Helper function for Initialize, used when there is a flag. On 2017/04/19 00:57:15, vadimt wrote: > We use 'helper' in class names, not method names. > Please rename, and change comments from "where it's used from" to "what it > does". Done. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:42: void InitializeFullscreenHelper(gfx::NativeView parent, On 2017/04/19 15:18:51, sky wrote: > Document what the parameters meant. Done. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:44: const gfx::Rect display_work_area_bounds); On 2017/04/19 15:18:51, sky wrote: > const gfx::Rect& here and 53. Done. https://codereview.chromium.org/2802903003/diff/220001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:47: void InitializeBubbleHelper(gfx::NativeView parent, int initial_apps_page); On 2017/04/19 15:18:51, sky wrote: > Why do you need the InitializeHelper functions to be public? Shouldn't callers > only call Initialize? Also, why do you need InitAsBubble? Can there be *one* > public init function? It can take an enum indicating the mode. Thanks, I made the helper functions (now no longer renamed *Helper after vadimt@ 's note) private. I also refactored InitAsBubble and changed all uses of it to Initialize. I decided to overload Initialize to accommodate its use by app_list_presenter_delegate_mus.cc and a few other files. I couldn't default the const gfx::Rect& parameter, and I didn't want to create a fake value for gfx::Rect&. I'm not sure my way is the best way to handle this.
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/2802903003/diff/220001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { On 2017/04/20 19:59:01, newcomer wrote: > On 2017/04/19 00:57:15, vadimt wrote: > > You can define it in app_list_switches.h and use everywhere. > > Done. Thanks, but I made a mistake :(, I meant the method that *checks* the flag... https://codereview.chromium.org/2802903003/diff/280001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:61: bool test_with_fullscreen; add "_" https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:206: display_work_area_bounds); indentation.... https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:105: int initial_apps_page, Indent doesn't look right...
https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:206: display_work_area_bounds); On 2017/04/20 20:21:35, vadimt wrote: > indentation.... I think if you run git cl format it'll fix all the spacing issues (hopefully). https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:42: void Initialize(gfx::NativeView parent, Again, can there be just one Initialize() function? Can't this class lookup the work area bounds using Screen? e.g. display::Screen::GetScreen()->GetDisplayNearestView(parent).work_area()?
Patchset #8 (id:300001) 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 #8 (id:320001) 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 #8 (id:340001) 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, found a typo, git cl format-ed, and finally reduced to one Initialize function! -Alex https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/220001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { On 2017/04/20 20:21:34, vadimt wrote: > On 2017/04/20 19:59:01, newcomer wrote: > > On 2017/04/19 00:57:15, vadimt wrote: > > > You can define it in app_list_switches.h and use everywhere. > > > > Done. > > Thanks, but I made a mistake :(, I meant the method that *checks* the flag... Roger and Done! I moved both functions into app_list_switches.cc. I'll talk with you to confirm if you want both in app_list_switches or just the function that checks the flag. https://codereview.chromium.org/2802903003/diff/280001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:61: bool test_with_fullscreen; On 2017/04/20 20:21:34, vadimt wrote: > add "_" Done. https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:206: display_work_area_bounds); On 2017/04/20 21:08:45, sky wrote: > On 2017/04/20 20:21:35, vadimt wrote: > > indentation.... > > I think if you run git cl format it'll fix all the spacing issues (hopefully). I gave it a shot! https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.h (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:42: void Initialize(gfx::NativeView parent, On 2017/04/20 21:08:45, sky wrote: > Again, can there be just one Initialize() function? Can't this class lookup the > work area bounds using Screen? e.g. > display::Screen::GetScreen()->GetDisplayNearestView(parent).work_area()? Done! https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_... ui/app_list/views/app_list_view.h:105: int initial_apps_page, On 2017/04/20 20:21:35, vadimt wrote: > Indent doesn't look right... git cl format, done!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { Is there a reason you have this here rather than in the test, which is the only place that uses it? https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:198: if (switches::IsFullscreenAppListEnabled()) { no {} https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:220: if (!switches::IsFullscreenAppListEnabled()) { no {} https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:241: if (!switches::IsFullscreenAppListEnabled()) { no {} https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:412: views::Widget::InitParams::TYPE_WINDOW_FRAMELESS); Set app_list_overlay_view_params.opacity = Widget::InitParams::TRANSLUCENT_WINDOW; and then remove 419. This will work better in mus. https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:414: app_list_overlay_view_params.parent = parent; The bubble code path parents the widget to |parent|, are you sure you don't want that here? https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:418: widget->SetBounds(display_work_area_bounds); Rather than this, specify the bounds in the InitParams.
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:380001) has been deleted
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
addressed comments! https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { On 2017/04/21 21:02:38, sky wrote: > Is there a reason you have this here rather than in the test, which is the only > place that uses it? My reason is that usually setters/getters are near each other. However I see why it could also live in the unit test. Which one do you prefer? This function won't be around for very long regardless. https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:198: if (switches::IsFullscreenAppListEnabled()) { On 2017/04/21 21:02:38, sky wrote: > no {} roger https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:220: if (!switches::IsFullscreenAppListEnabled()) { On 2017/04/21 21:02:38, sky wrote: > no {} roger https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:241: if (!switches::IsFullscreenAppListEnabled()) { On 2017/04/21 21:02:38, sky wrote: > no {} Acknowledged. https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:412: views::Widget::InitParams::TYPE_WINDOW_FRAMELESS); On 2017/04/21 21:02:38, sky wrote: > Set app_list_overlay_view_params.opacity = > Widget::InitParams::TRANSLUCENT_WINDOW; and then remove 419. This will work > better in mus. Done. https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:414: app_list_overlay_view_params.parent = parent; On 2017/04/21 21:02:39, sky wrote: > The bubble code path parents the widget to |parent|, are you sure you don't want > that here? Done! And moved the shared code to Initialize. https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:418: widget->SetBounds(display_work_area_bounds); On 2017/04/21 21:02:39, sky wrote: > Rather than this, specify the bounds in the InitParams. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { On 2017/04/21 22:44:15, newcomer wrote: > On 2017/04/21 21:02:38, sky wrote: > > Is there a reason you have this here rather than in the test, which is the > only > > place that uses it? > > My reason is that usually setters/getters are near each other. However I see why > it could also live in the unit test. Which one do you prefer? This function > won't be around for very long regardless. As this code should only be used in tests, not production, putting it in the test is the way to go.
lgtm; please continue with sky@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Responded to comments and made changes! -Alex
On 2017/04/21 23:05:40, sky wrote: > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... > File ui/app_list/app_list_switches.cc (right): > > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... > ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { > On 2017/04/21 22:44:15, newcomer wrote: > > On 2017/04/21 21:02:38, sky wrote: > > > Is there a reason you have this here rather than in the test, which is the > > only > > > place that uses it? > > > > My reason is that usually setters/getters are near each other. However I see > why > > it could also live in the unit test. Which one do you prefer? This function > > won't be around for very long regardless. > > As this code should only be used in tests, not production, putting it in the > test is the way to go. Were you going to move the setting to the test only?
On 2017/04/24 23:33:15, sky wrote: > On 2017/04/21 23:05:40, sky wrote: > > > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... > > File ui/app_list/app_list_switches.cc (right): > > > > > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... > > ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { > > On 2017/04/21 22:44:15, newcomer wrote: > > > On 2017/04/21 21:02:38, sky wrote: > > > > Is there a reason you have this here rather than in the test, which is the > > > only > > > > place that uses it? > > > > > > My reason is that usually setters/getters are near each other. However I see > > why > > > it could also live in the unit test. Which one do you prefer? This function > > > won't be around for very long regardless. > > > > As this code should only be used in tests, not production, putting it in the > > test is the way to go. > > Were you going to move the setting to the test only? Sorry about that, I just didn't commit the change. It's up now!
All changes are now in the patchset. -Alex https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_s... ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { On 2017/04/21 23:05:40, sky wrote: > On 2017/04/21 22:44:15, newcomer wrote: > > On 2017/04/21 21:02:38, sky wrote: > > > Is there a reason you have this here rather than in the test, which is the > > only > > > place that uses it? > > > > My reason is that usually setters/getters are near each other. However I see > why > > it could also live in the unit test. Which one do you prefer? This function > > won't be around for very long regardless. > > As this code should only be used in tests, not production, putting it in the > test is the way to go. Done.
LGTM
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_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.
I forgot to change the namespace on the switch that I changed. I changed it, and all tests are now passing.
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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2802903003/#ps440001 (title: "Noob mistake, forgot to change the namespace of the switch after I moved it")
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": 440001, "attempt_start_ts": 1493150546973290, "parent_rev": "613ba696d0a30183fa0a5e2e084a44aca2f6bd1e", "commit_rev": "23bdfdaf36a0e72e57e6bcf909d87fd4f5d5c663"}
Message was sent while issue was closed.
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 Review-Url: https://codereview.chromium.org/2802903003 Cr-Commit-Position: refs/heads/master@{#467088} Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:440001) as https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8...
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) identified this CL at revision 467088 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.
A revert of this CL (patchset #11 id:440001) has been created in https://codereview.chromium.org/2840993002/ by vadimt@chromium.org. The reason for reverting is: Suspecting that this breaks ash_unittests.
Message was sent while issue was closed.
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 Review-Url: https://codereview.chromium.org/2802903003 Cr-Commit-Position: refs/heads/master@{#467088} Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8... ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 Review-Url: https://codereview.chromium.org/2802903003 Cr-Commit-Position: refs/heads/master@{#467088} Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8... ==========
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 newcomer@chromium.org
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
The CQ bit was unchecked by newcomer@chromium.org
The CQ bit was checked by newcomer@chromium.org to run a CQ dry run
The CQ bit was unchecked by newcomer@chromium.org
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 unchecked by newcomer@chromium.org
Fixed the memory error that broke the build. (dangling reference to Screen).
lgtm; Please land this, I don't think we need to wait for sky@'s CR
The CQ bit was checked by newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2802903003/#ps460001 (title: "Fixed the mishandled reference that was breaking the build")
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": 460001, "attempt_start_ts": 1493246575730170, "parent_rev": "1b67ae3a76893ee92586a28fd4b11640e28e12de", "commit_rev": "145b24f9355dc8a7c59bfb93f3634eef1d555784"}
Message was sent while issue was closed.
Description was changed from ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 Review-Url: https://codereview.chromium.org/2802903003 Cr-Commit-Position: refs/heads/master@{#467088} Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8... ========== to ========== Implementation of a full screen app list behind a switch (as opposed to a feature flag). Also re-alphabetized switches in ash_switches.cc BUG=711349 Review-Url: https://codereview.chromium.org/2802903003 Cr-Original-Commit-Position: refs/heads/master@{#467088} Committed: https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d8... Review-Url: https://codereview.chromium.org/2802903003 Cr-Commit-Position: refs/heads/master@{#467496} Committed: https://chromium.googlesource.com/chromium/src/+/145b24f9355dc8a7c59bfb93f363... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:460001) as https://chromium.googlesource.com/chromium/src/+/145b24f9355dc8a7c59bfb93f363... |