Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2802903003: Implementation of a full screen app list and re-alphabetized switches (Closed)

Created:
7 months, 2 weeks ago by newcomer
Modified:
7 months ago
Reviewers:
vadimt, sky, newcomer
CC:
chromium-reviews, kalyank, sadrul, Matt Giuca, tfarina
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+148 lines, -62 lines) Patch
M ash/app_list/app_list_presenter_delegate.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -5 lines 0 comments Download
M ash/app_list/app_list_presenter_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +26 lines, -4 lines 0 comments Download
M chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc View 1 2 3 4 5 6 1 chunk +3 lines, -2 lines 0 comments Download
M testing/buildbot/filters/ash_mus_unittests.filter View 1 2 3 4 5 6 1 chunk +11 lines, -0 lines 0 comments Download
M ui/app_list/app_list_switches.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M ui/app_list/app_list_switches.cc View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -0 lines 0 comments Download
M ui/app_list/demo/app_list_demo_views.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M ui/app_list/presenter/app_list_presenter_impl_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M ui/app_list/views/app_list_view.h View 1 2 3 4 5 6 7 4 chunks +15 lines, -6 lines 0 comments Download
M ui/app_list/views/app_list_view.cc View 1 2 3 4 5 6 7 8 9 10 11 11 chunks +69 lines, -39 lines 0 comments Download
M ui/app_list/views/app_list_view_unittest.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M ui/aura/window.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 134 (95 generated)
newcomer
Please review my changes! I implemented a full screen app list(behind a switch) for the ...
7 months, 2 weeks ago (2017-04-06 22:46:41 UTC) #4
sky
You should also add test coverage of this. I suggest changing making the relevant tests ...
7 months, 2 weeks ago (2017-04-07 03:28:19 UTC) #5
newcomer
Thanks for your initial comments! I did some refactoring as asked although you may have ...
7 months, 2 weeks ago (2017-04-10 16:34:53 UTC) #6
newcomer
Published a few comments. -Alex https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_presenter_delegate.cc File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_presenter_delegate.cc#newcode105 ash/app_list/app_list_presenter_delegate.cc:105: GetMinimumBoundsHeightForAppList(view)), Moved view->SetAnchorPoint into ...
7 months, 2 weeks ago (2017-04-10 16:43:22 UTC) #7
vadimt
https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_presenter_delegate.cc File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ash/app_list/app_list_presenter_delegate.cc#newcode102 ash/app_list/app_list_presenter_delegate.cc:102: view->InitializeWindow( Please talk to Now people (I'll provide contacts) ...
7 months, 2 weeks ago (2017-04-10 19:40:11 UTC) #8
sky
https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_switches.cc File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/60001/ui/app_list/app_list_switches.cc#newcode26 ui/app_list/app_list_switches.cc:26: extern const char kEnableFullscreenAppList[] = "ash-enable-fullscreen-app-list"; On 2017/04/10 19:40:10, ...
7 months, 2 weeks ago (2017-04-10 20:44:29 UTC) #9
vadimt
Please create a bug and refer it from the CL. Thanks!
7 months, 2 weeks ago (2017-04-12 17:44:59 UTC) #13
newcomer1
On 2017/04/12 17:44:59, vadimt wrote: > Please create a bug and refer it from the ...
7 months, 1 week ago (2017-04-13 15:44:13 UTC) #15
newcomer1
On 2017/04/13 15:44:13, newcomer1 wrote: > On 2017/04/12 17:44:59, vadimt wrote: > > Please create ...
7 months, 1 week ago (2017-04-13 17:13:48 UTC) #22
newcomer
Addressed the comments! I am waiting for Shiba to get back to me with the ...
7 months, 1 week ago (2017-04-13 20:25:48 UTC) #23
newcomer
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_presenter_delegate.cc ...
7 months, 1 week ago (2017-04-13 21:14:00 UTC) #26
vadimt
What about tests sky@ requested? https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_presenter_delegate.cc File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_presenter_delegate.cc#newcode106 ash/app_list/app_list_presenter_delegate.cc:106: ash::ScreenUtil::GetDisplayWorkAreaBoundsInParent(container)); Please fix indentation ...
7 months, 1 week ago (2017-04-13 21:48:34 UTC) #29
newcomer
https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_presenter_delegate.cc File ash/app_list/app_list_presenter_delegate.cc (right): https://codereview.chromium.org/2802903003/diff/80001/ash/app_list/app_list_presenter_delegate.cc#newcode106 ash/app_list/app_list_presenter_delegate.cc:106: ash::ScreenUtil::GetDisplayWorkAreaBoundsInParent(container)); On 2017/04/13 21:48:34, vadimt wrote: > Please fix ...
7 months, 1 week ago (2017-04-18 21:12:34 UTC) #31
newcomer
The CL is now ready for review! -Alex
7 months, 1 week ago (2017-04-18 23:21:45 UTC) #39
vadimt
Some comments; more may follow in future iterations. 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#newcode32 ash/app_list/app_list_presenter_delegate_unittest.cc:32: void ...
7 months, 1 week ago (2017-04-19 00:57:16 UTC) #40
sky
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 ...
7 months, 1 week ago (2017-04-19 15:18:51 UTC) #41
newcomer
Thanks for the fast feedback! -Alex On Wed, Apr 19, 2017 at 8:18 AM, <sky@chromium.org> ...
7 months, 1 week ago (2017-04-19 16:36:18 UTC) #42
newcomer
Thanks, Alex 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#newcode32 ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { On 2017/04/19 00:57:15, vadimt ...
7 months ago (2017-04-20 19:59:02 UTC) #58
vadimt
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#newcode32 ash/app_list/app_list_presenter_delegate_unittest.cc:32: void SetFullscreenAppListSwitch() { On 2017/04/20 19:59:01, newcomer wrote: > ...
7 months ago (2017-04-20 20:21:35 UTC) #63
sky
https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_list_view.cc File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2802903003/diff/280001/ui/app_list/views/app_list_view.cc#newcode206 ui/app_list/views/app_list_view.cc:206: display_work_area_bounds); On 2017/04/20 20:21:35, vadimt wrote: > indentation.... I ...
7 months ago (2017-04-20 21:08:45 UTC) #64
newcomer
Addressed comments, found a typo, git cl format-ed, and finally reduced to one Initialize function! ...
7 months ago (2017-04-21 19:51:22 UTC) #78
sky
https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc#newcode93 ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { Is there a reason you have ...
7 months ago (2017-04-21 21:02:39 UTC) #81
newcomer
addressed comments! https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc#newcode93 ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { On 2017/04/21 21:02:38, sky ...
7 months ago (2017-04-21 22:44:15 UTC) #86
sky
https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc#newcode93 ui/app_list/app_list_switches.cc:93: void SetFullscreenAppListSwitch() { On 2017/04/21 22:44:15, newcomer wrote: > ...
7 months ago (2017-04-21 23:05:40 UTC) #88
vadimt
lgtm; please continue with sky@
7 months ago (2017-04-21 23:15:27 UTC) #89
newcomer
Responded to comments and made changes! -Alex
7 months ago (2017-04-24 21:05:52 UTC) #92
sky
On 2017/04/21 23:05:40, sky wrote: > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc > File ui/app_list/app_list_switches.cc (right): > > https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc#newcode93 > ...
7 months ago (2017-04-24 23:33:15 UTC) #93
newcomer
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_switches.cc ...
7 months ago (2017-04-25 00:15:08 UTC) #94
newcomer
All changes are now in the patchset. -Alex https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc File ui/app_list/app_list_switches.cc (right): https://codereview.chromium.org/2802903003/diff/360001/ui/app_list/app_list_switches.cc#newcode93 ui/app_list/app_list_switches.cc:93: void ...
7 months ago (2017-04-25 00:16:00 UTC) #95
sky
LGTM
7 months ago (2017-04-25 17:10:38 UTC) #96
newcomer
I forgot to change the namespace on the switch that I changed. I changed it, ...
7 months ago (2017-04-25 19:35:08 UTC) #105
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2802903003/440001
7 months ago (2017-04-25 20:02:56 UTC) #108
commit-bot: I haz the power
Committed patchset #11 (id:440001) as https://chromium.googlesource.com/chromium/src/+/23bdfdaf36a0e72e57e6bcf909d87fd4f5d5c663
7 months ago (2017-04-25 20:11:54 UTC) #111
findit-for-me
Findit(https://goo.gl/kROfz5) identified this CL at revision 467088 as the culprit for failures in the build ...
7 months ago (2017-04-26 00:28:56 UTC) #112
vadimt
A revert of this CL (patchset #11 id:440001) has been created in https://codereview.chromium.org/2840993002/ by vadimt@chromium.org. ...
7 months ago (2017-04-26 00:36:09 UTC) #113
newcomer
Fixed the memory error that broke the build. (dangling reference to Screen).
7 months ago (2017-04-26 22:37:58 UTC) #127
vadimt
lgtm; Please land this, I don't think we need to wait for sky@'s CR
7 months ago (2017-04-26 22:39:57 UTC) #128
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2802903003/460001
7 months ago (2017-04-26 22:43:38 UTC) #131
commit-bot: I haz the power
7 months ago (2017-04-26 22:58:17 UTC) #134
Message was sent while issue was closed.
Committed patchset #12 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/145b24f9355dc8a7c59bfb93f363...

Powered by Google App Engine
This is Rietveld efc10ee0f