Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1483593002:
arc: Hide app list view on ARC app launch. (Closed)
DescriptionHide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
Committed: https://crrev.com/29e5cbe882726cb60020d123909ad926e6d44b3f
Cr-Commit-Position: refs/heads/master@{#364645}
Patch Set 1 #Patch Set 2 : Prepare for review #Patch Set 3 : Enable unit_tests #
Total comments: 2
Patch Set 4 : Update #
Total comments: 7
Patch Set 5 : Rebased #
Total comments: 1
Patch Set 6 : nits addressed #
Messages
Total messages: 17 (7 generated)
Description was changed from
==========
arc: Hide app list view on ARC app launch.
BUG=b/25813997
==========
to
==========
Hide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
==========
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
PTAL https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:36: AppListControllerDelegate* controller, ExtensionAppItem also uses AppListControllerDelegate in several places. It has own function ExtensionAppItem::GetController that resolve controller from AppListService. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... At my first step, I implemented ArcAppItem similar but got a problem with tests, please see below. I don't know why ExtensionAppItem is implemented this way but from my point of view, it has several drawbacks. ExtensionAppModelBuilder has controller already that is taken from AppListService. From this point of view, this is duplicated. This is also bad for unit_tests, where AppListService does not work. Instead, in tests we use proxy test::TestAppListControllerDelegate. However, with current implementation ExtensionAppItem will try to use real AppListService and will crash (no such tests currently, but anyway). So I see a good refactoring candidate. As I understand: 1. ExtensionAppItem has to use the same controller as in ExtensionAppModelBuilder in tests and in real live. 2. controller to ExtensionAppItem is passed from ExtensionAppModelBuilder where it is created. 3. There is a temptation to go next and put this controller to the base class for ExtensionAppItem and ArcAppItem - app_list::AppListItem. But here we have to announce AppListControllerDelegate at app_list_item.h. AppListControllerDelegate is declared at chrome/browser/ui but AppListItem in ui/. As I see from the implementation, there is impossible to use chrome/browser/ui from ui. We can use forward declaration, but I feel this is unacceptable. 4. Potentially I can implement intermediate base class in chrome/browser/ui/app_list WDYT? If we need refactoring do you want to do it first?
https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:36: AppListControllerDelegate* controller, On 2015/12/08 08:30:03, khmel wrote: > ExtensionAppItem also uses AppListControllerDelegate in several places. It has > own function ExtensionAppItem::GetController that resolve controller from > AppListService. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > At my first step, I implemented ArcAppItem similar but got a problem with tests, > please see below. > > I don't know why ExtensionAppItem is implemented this way but from my point of > view, it has several drawbacks. ExtensionAppModelBuilder has controller already > that is taken from AppListService. From this point of view, this is duplicated. > This is also bad for unit_tests, where AppListService does not work. Instead, in > tests we use proxy test::TestAppListControllerDelegate. However, with current > implementation ExtensionAppItem will try to use real AppListService and will > crash (no such tests currently, but anyway). > > So I see a good refactoring candidate. As I understand: > 1. ExtensionAppItem has to use the same controller as in > ExtensionAppModelBuilder in tests and in real live. > 2. controller to ExtensionAppItem is passed from ExtensionAppModelBuilder where > it is created. > 3. There is a temptation to go next and put this controller to the base class > for ExtensionAppItem and ArcAppItem - app_list::AppListItem. But here we have to > announce AppListControllerDelegate at app_list_item.h. AppListControllerDelegate > is declared at chrome/browser/ui but AppListItem in ui/. As I see from the > implementation, there is impossible to use chrome/browser/ui from ui. We can use > forward declaration, but I feel this is unacceptable. > 4. Potentially I can implement intermediate base class in > chrome/browser/ui/app_list > > WDYT? > > If we need refactoring do you want to do it first? The code is the way it is because of historical reasons. There are two desktop types on Win8, HOST_DESKTOP_TYPE_ASH in metro mode and HOST_DESKTOP_TYPE_NATIVE for desktop mode. We actually have two controller types because of this. However, we only have one chrome instance, one profile thus one AppListSyncableService and one model builder. The controller in the model builder (and syncable service) is not necessarily the correct one to use. See comments in AppListSyncableService::BuildModel [1]. Back to the issue this CL tries to resolve, app list should automatically dismiss itself after losing focus [2]. So usually we don't need to explicitly call DismissView. So my questions are: - Does arc app have an aura window to contain its content? - If it has, does the hosting window get activated properly? [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [2] AppListController::OnWindowFocused https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/app_list_co...
On 2015/12/08 17:22:56, xiyuan wrote: > https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... > File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): > > https://codereview.chromium.org/1483593002/diff/40001/chrome/browser/ui/app_l... > chrome/browser/ui/app_list/arc/arc_app_item.cc:36: AppListControllerDelegate* > controller, > On 2015/12/08 08:30:03, khmel wrote: > > ExtensionAppItem also uses AppListControllerDelegate in several places. It has > > own function ExtensionAppItem::GetController that resolve controller from > > AppListService. > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > At my first step, I implemented ArcAppItem similar but got a problem with > tests, > > please see below. > > > > I don't know why ExtensionAppItem is implemented this way but from my point of > > view, it has several drawbacks. ExtensionAppModelBuilder has controller > already > > that is taken from AppListService. From this point of view, this is > duplicated. > > This is also bad for unit_tests, where AppListService does not work. Instead, > in > > tests we use proxy test::TestAppListControllerDelegate. However, with current > > implementation ExtensionAppItem will try to use real AppListService and will > > crash (no such tests currently, but anyway). > > > > So I see a good refactoring candidate. As I understand: > > 1. ExtensionAppItem has to use the same controller as in > > ExtensionAppModelBuilder in tests and in real live. > > 2. controller to ExtensionAppItem is passed from ExtensionAppModelBuilder > where > > it is created. > > 3. There is a temptation to go next and put this controller to the base class > > for ExtensionAppItem and ArcAppItem - app_list::AppListItem. But here we have > to > > announce AppListControllerDelegate at app_list_item.h. > AppListControllerDelegate > > is declared at chrome/browser/ui but AppListItem in ui/. As I see from the > > implementation, there is impossible to use chrome/browser/ui from ui. We can > use > > forward declaration, but I feel this is unacceptable. > > 4. Potentially I can implement intermediate base class in > > chrome/browser/ui/app_list > > > > WDYT? > > > > If we need refactoring do you want to do it first? > > The code is the way it is because of historical reasons. There are two desktop > types on Win8, HOST_DESKTOP_TYPE_ASH in metro mode and HOST_DESKTOP_TYPE_NATIVE > for desktop mode. We actually have two controller types because of this. > However, we only have one chrome instance, one profile thus one > AppListSyncableService and one model builder. The controller in the model > builder (and syncable service) is not necessarily the correct one to use. See > comments in AppListSyncableService::BuildModel [1]. Thanks for explanations! This clarifies a lot. > > Back to the issue this CL tries to resolve, app list should automatically > dismiss itself after losing focus [2]. So usually we don't need to explicitly > call DismissView. So my questions are: > - Does arc app have an aura window to contain its content? > - If it has, does the hosting window get activated properly? ARC does not use a window to host an app. Instead windows are used in Android internally. In Chrome we display only a surface with Android content. And when we start the app, no Chrome window is allocated. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > [2] AppListController::OnWindowFocused > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/app_list_co...
khmel@google.com changed reviewers: + khmel@google.com
PTAL https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:139: return AppListService::Get(chrome::GetActiveDesktop())-> I did similar to ExtensionsAppItem. However I don't think this is final code. Functionality is duplicated but I am hesitating where to put it. Base class belongs to ui/ package and I cannot move chrome/browser/ui dependencies there. WDYT?
https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:38: void ArcAppItem::DisableContollerDelegateForTesting() { I prefer to have something like void OverrideAppListControllerDelegateForTesting(AppListControllerDelegate* controller); And GetController() returns the overridden instance if it is set. https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:139: return AppListService::Get(chrome::GetActiveDesktop())-> On 2015/12/09 13:42:08, khmel1 wrote: > I did similar to ExtensionsAppItem. However I don't think this is final code. > Functionality is duplicated but I am hesitating where to put it. Base class > belongs to ui/ package and I cannot move chrome/browser/ui dependencies there. > WDYT? We cannot put the AppListService etc into ui/ because it is a chrome concept and does not belong there. We used to have a ChromeAppListItem (Removed in https://codereview.chromium.org/25859005 by pushing down Activate() call into ui/) that serves as the base class for app list items in chrome. We cannot do similar unless we want to also move AppListControllerDelegate delegate etc into ui/. So if we want to share this kind of code, we may need to resurrect it. https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.h (right): https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.h:52: nit: nuke one empty line
PTAL https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:38: void ArcAppItem::DisableContollerDelegateForTesting() { On 2015/12/09 17:20:29, xiyuan wrote: > I prefer to have something like > > void OverrideAppListControllerDelegateForTesting(AppListControllerDelegate* > controller); > > And GetController() returns the overridden instance if it is set. That is nice. Thanks for sharing best practices. I did it static as well. https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.cc:139: return AppListService::Get(chrome::GetActiveDesktop())-> On 2015/12/09 17:20:29, xiyuan wrote: > On 2015/12/09 13:42:08, khmel1 wrote: > > I did similar to ExtensionsAppItem. However I don't think this is final code. > > Functionality is duplicated but I am hesitating where to put it. Base class > > belongs to ui/ package and I cannot move chrome/browser/ui dependencies there. > > > WDYT? > > We cannot put the AppListService etc into ui/ because it is a chrome concept and > does not belong there. We used to have a ChromeAppListItem (Removed in > https://codereview.chromium.org/25859005 by pushing down Activate() call into > ui/) that serves as the base class for app list items in chrome. We cannot do > similar unless we want to also move AppListControllerDelegate delegate etc into > ui/. So if we want to share this kind of code, we may need to resurrect it. > Thanks for confirming that adding base class is acceptable way to handle it. I did it in separate CL https://codereview.chromium.org/1507873006/ PS. This will also help me in my next steps for better ARC app integration into app_list and shelf. https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_item.h (right): https://codereview.chromium.org/1483593002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_item.h:52: On 2015/12/09 17:20:30, xiyuan wrote: > nit: nuke one empty line Done.
lgtm https://codereview.chromium.org/1483593002/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right): https://codereview.chromium.org/1483593002/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_unittest.cc:275: // Disable attemts to dissmis app launcher view. nit: attemts -> attempts, dissmis -> dismiss
The CQ bit was checked by khmel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1483593002/#ps100001 (title: "nits addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1483593002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1483593002/100001
Message was sent while issue was closed.
Description was changed from
==========
Hide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
==========
to
==========
Hide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Hide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
==========
to
==========
Hide app list view on ARC app launch from app-launcher view.
BUG=567520
TEST=unit_tests passed
TEST=Start Chrome, manually click on ready ARC app item in
app-launcher. ARC app is started and app list view is
automatically closed.
Committed: https://crrev.com/29e5cbe882726cb60020d123909ad926e6d44b3f
Cr-Commit-Position: refs/heads/master@{#364645}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/29e5cbe882726cb60020d123909ad926e6d44b3f Cr-Commit-Position: refs/heads/master@{#364645} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
