Adds an integration test for uninstalling app list search results.

Adds an integration test for uninstalling app list search results.

This involves exposing the app list model to a BrowserTest, and ensuring
that changes in the model are properly observed by the UI.

BUG=277897, 169114
TEST=AppListControllerSearchResultsBrowserTest.UninstallSearchResult

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221197

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222107

[tapted, 2013-08-23 07:07:04]

(attaching comment)

https://codereview.chromium.org/23072036/diff/4001/chrome/browser/ui/app_list...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/23072036/diff/4001/chrome/browser/ui/app_list...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:54:
observed_result_->RemoveObserver(this);
Note that without this line, there are actually two OnItemUninstalled
notifications and the check on line 94 will fail.

[tapted, 2013-08-27 13:19:54]

koz, care to take an initial look? This is a regression test for
http://crbug.com/277897 and steps towards some cross-platform
integration/end-to-end tests for the app launcher.

[koz (OOO until 15th September), 2013-08-28 00:37:43]

https://chromiumcodereview.appspot.com/23072036/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://chromiumcodereview.appspot.com/23072036/diff/27001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:211:
observed_results_list_ = model->results();
All the assigning to protected variables seems a little tricky. What if instead
you had a function like this:

WatchResultsLookingForItem(model->results(), extension_name);

Which watches results() for changes that include extension_name (and then sets a
listener on it).

https://chromiumcodereview.appspot.com/23072036/diff/27001/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/23072036/diff/27001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:939: return
shower_->GetView()->model();
Could you change this to shower_->GetCurrentModel(), which returns NULL in the
case where view_ isn't defined?

[tapted, 2013-08-28 03:09:48]

+xiyuan for review. I think I'll need a src/ash OWNER as well, but wanted to run
this by you first. In particular what you think about exposing the model to
facilitate integration testing in a browser_test.


Also added a few additional checks.. and fixed a Mac unit test.

https://codereview.chromium.org/23072036/diff/27001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/23072036/diff/27001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:211:
observed_results_list_ = model->results();
On 2013/08/28 00:37:43, koz wrote:
> All the assigning to protected variables seems a little tricky. What if
instead
> you had a function like this:
> 
> WatchResultsLookingForItem(model->results(), extension_name);
> 
> Which watches results() for changes that include extension_name (and then sets
a
> listener on it).

Done.

https://codereview.chromium.org/23072036/diff/27001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/23072036/diff/27001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:939: return
shower_->GetView()->model();
On 2013/08/28 00:37:43, koz wrote:
> Could you change this to shower_->GetCurrentModel(), which returns NULL in the
> case where view_ isn't defined?

Done.

[koz (OOO until 15th September), 2013-08-28 03:18:03]

lgtm

[xiyuan, 2013-08-28 03:36:44]

LGTM

It's perfectly okay to expose the model. Out of the scope of this CL, I have
some doubt of where the model should live now. I put it into AppListView but now
felt that it might better live inside AppListViewDelegate.

And yes, you need ash owner's stamp. I am not in the list. :p

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h#newcode235
ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
nit: Can we instead exposing AppListController? In the future, we might want to
consolidate all app list methods to go through AppListController interface so
that we have less methods in Shell.

https://codereview.chromium.org/23072036/diff/49001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/23072036/diff/49001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_view.h:42: AppListModel* model() { return
model_.get(); }
nit: move accessor to the end of methods.

[tapted, 2013-08-28 07:23:13]

+sky for src/ash OWNERS

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h#newcode235
ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
On 2013/08/28 03:36:45, xiyuan wrote:
> nit: Can we instead exposing AppListController? In the future, we might want
to
> consolidate all app list methods to go through AppListController interface so
> that we have less methods in Shell.

I actually tried this in an earlier attempt ;) The problem is that
ash::internal::AppListController isn't exported out with ASH_EXPORT, so there
are link errors trying to use that interface in chrome/browser.

So this might be better in a follow-up. I think we'd either need to move it out
of ash::internal and export it all, or export a new abstract interface that it
implements.

https://codereview.chromium.org/23072036/diff/49001/ui/app_list/views/app_lis...
File ui/app_list/views/app_list_view.h (right):

https://codereview.chromium.org/23072036/diff/49001/ui/app_list/views/app_lis...
ui/app_list/views/app_list_view.h:42: AppListModel* model() { return
model_.get(); }
On 2013/08/28 03:36:45, xiyuan wrote:
> nit: move accessor to the end of methods.

Done.

https://codereview.chromium.org/23072036/diff/65001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/23072036/diff/65001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:155: void
StopWatchingResults() {
note: moved this from the destructor and it's called explicitly since, on
Windows (but not Mac), the AppListModel gets destroyed before test TearDown; in
InProcessBrowserTest::QuitBrowsers() via
views::Widget::OnNativeWidgetDestroyed()

[sky, 2013-08-28 13:57:00]

https://codereview.chromium.org/23072036/diff/65001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/23072036/diff/65001/ash/shell.h#newcode235
ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
Put this in ShellTestAPI and do something similar for AppListController.

[xiyuan, 2013-08-28 16:33:45]

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/23072036/diff/49001/ash/shell.h#newcode235
ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
On 2013/08/28 07:23:14, tapted wrote:
> On 2013/08/28 03:36:45, xiyuan wrote:
> > nit: Can we instead exposing AppListController? In the future, we might want
> to
> > consolidate all app list methods to go through AppListController interface
so
> > that we have less methods in Shell.
> 
> I actually tried this in an earlier attempt ;) The problem is that
> ash::internal::AppListController isn't exported out with ASH_EXPORT, so there
> are link errors trying to use that interface in chrome/browser.
> 
> So this might be better in a follow-up. I think we'd either need to move it
out
> of ash::internal and export it all, or export a new abstract interface that it
> implements.

Exporting AppListController should be good enough as long as we only use it in
test or internally in ash. Think we already export some internal classes for
test.

[tapted, 2013-08-29 12:41:51]

koz - please take another look too.

At this stage, ash and mac AppListService interfaces also have a shiny new test
API (otherwise the ash service would have no way to access the ash controller
test api). These have an abstract base, so that things do not get messy to
support win+ash.

Then, non-ash Windows is half-way there. There are more encapsulation layers in
app_list_controller_win.cc. Accessing the model is tricky without that accessor
`mixed in`, so exploring this further might be better in a follow-up. Happy to
explore it here too - this change isn't urgent, but it has become quite large.

On 2013/08/28 16:33:45, xiyuan wrote:
> https://codereview.chromium.org/23072036/diff/49001/ash/shell.h
> File ash/shell.h (right):
> 
> https://codereview.chromium.org/23072036/diff/49001/ash/shell.h#newcode235
> ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
> On 2013/08/28 07:23:14, tapted wrote:
> > On 2013/08/28 03:36:45, xiyuan wrote:
> > > nit: Can we instead exposing AppListController? In the future, we might
want
> > to
> > > consolidate all app list methods to go through AppListController interface
> so
> > > that we have less methods in Shell.
> > 
> > I actually tried this in an earlier attempt ;) The problem is that
> > ash::internal::AppListController isn't exported out with ASH_EXPORT, so
there
> > are link errors trying to use that interface in chrome/browser.
> > 
> > So this might be better in a follow-up. I think we'd either need to move it
> out
> > of ash::internal and export it all, or export a new abstract interface that
it
> > implements.
> 
> Exporting AppListController should be good enough as long as we only use it in
> test or internally in ash. Think we already export some internal classes for
> test.

The ash AppListController is now exposed (but not exported) via the ShellTestApi
instead of the AppListModel. And since the AppListController now has its own
TestApi, that turned out to be the only interface I needed to export for this
change.

https://codereview.chromium.org/23072036/diff/65001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/23072036/diff/65001/ash/shell.h#newcode235
ash/shell.h:235: app_list::AppListView* GetAppListViewForTesting();
On 2013/08/28 13:57:00, sky wrote:
> Put this in ShellTestAPI and do something similar for AppListController.

Done. There is a big impact due to the multi-platform nature of the
AppListService (7 new files), but I think it's a really good direction and will
help testability a lot. (Pretty sure I got the design right!)

[koz (OOO until 15th September), 2013-08-30 00:12:42]

slgtm

https://codereview.chromium.org/23072036/diff/119001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/23072036/diff/119001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:193: virtual void
ListItemsAdded(size_t start, size_t count) OVERRIDE { Update(); }
nit: Update() -> AttemptToLocateItem()

[tapted, 2013-08-30 00:54:39]

xiyuan - WDYT? I guess a sub-goal of this is to lower the barrier to more
integration tests for the app launcher. And there exists another testing
function on the service interface that is a clear candidate to be moved to these
new testing APIs in a follow-up. But if it is too scary, I think an alternative
is to go back to your earlier comment:

On 2013/08/28 03:36:44, xiyuan wrote:
> Out of the scope of this CL, I have
> some doubt of where the model should live now. I put it into AppListView but
now
> felt that it might better live inside AppListViewDelegate.

If I explore this first, it will influence what "needs" to be done in this CL.
(but maybe we should have a testing API anyway).

https://codereview.chromium.org/23072036/diff/119001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/23072036/diff/119001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:193: virtual void
ListItemsAdded(size_t start, size_t count) OVERRIDE { Update(); }
On 2013/08/30 00:12:42, koz wrote:
> nit: Update() -> AttemptToLocateItem()

Done.

[xiyuan, 2013-08-30 01:07:48]

SLGTM

On 2013/08/30 00:54:39, tapted wrote:
> xiyuan - WDYT? I guess a sub-goal of this is to lower the barrier to more
> integration tests for the app launcher. And there exists another testing
> function on the service interface that is a clear candidate to be moved to
these
> new testing APIs in a follow-up. But if it is too scary, I think an
alternative
> is to go back to your earlier comment:
> 

Moving test methods into a test api is the way to go. This CL is moving in the
right direction and great as a base for more app launcher integration tests.

> On 2013/08/28 03:36:44, xiyuan wrote:
> > Out of the scope of this CL, I have
> > some doubt of where the model should live now. I put it into AppListView but
> now
> > felt that it might better live inside AppListViewDelegate.
> 
> If I explore this first, it will influence what "needs" to be done in this CL.
> (but maybe we should have a testing API anyway).
> 

No need to do it in this CL. The thought comes to me while looking calamity@'s
work of profile switching. It makes more sense if AppListViewDelegate has access
to the model and can modify it directly.

[tapted, 2013-08-30 23:14:41]

sky: for src/ash - PTAL

[sky, 2013-09-03 15:53:31]

LGTM

[commit-bot: I haz the power, 2013-09-04 01:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/23072036/134001

[commit-bot: I haz the power, 2013-09-04 16:40:16]

Change committed as 221197

[Vitaly Buka (NO REVIEWS), 2013-09-04 17:03:31]

Reverted because of 

app_list_controller_test_api.cc(14) : error C2220: warning treated as error - no
'object' file generated
app_list_controller_test_api.cc(14) : warning C4273:
'ash::test::AppListControllerTestApi::AppListControllerTestApi' : inconsistent
dll linkage
app_list_controller_test_api.h(26) : see previous definition of '{ctor}'
app_list_controller_test_api.cc(16) : warning C4273:
'ash::test::AppListControllerTestApi::view' : inconsistent dll linkage
app_list_controller_test_api.h(28) : see previous definition of 'view'

[tapted, 2013-09-09 17:52:45]

Relanding - forgot that *test_support targets are always statically linked, so
no need for ASH_EXPORT in the test api. Double checked building aura_builder in
a component build on Windows - should be good.

Also the TODO at
https://codereview.chromium.org/23072036/diff2/161001:164001/chrome/browser/u...
is TODONE as of r221329, so it's now EXPECT_EQ(1..) instead of 2..);

[commit-bot: I haz the power, 2013-09-09 17:53:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/23072036/164001

[commit-bot: I haz the power, 2013-09-09 17:53:34]

Failed to apply patch for chrome/chrome_tests.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_tests.gypi
  Hunk #1 FAILED at 1576.
  1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej

Patch:       chrome/chrome_tests.gypi
Index: chrome/chrome_tests.gypi
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index
7276fb607d2e8e0505bc15eb86dc65984be6f651..fdb45aefb2543850ca2e5f1f4ba9763263599a37
100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1576,6 +1576,11 @@
         'browser/ui/app_list/app_list_controller_browsertest.cc',
         'browser/ui/app_list/app_list_service_mac_browsertest.mm',
         'browser/ui/app_list/search/webstore_provider_browsertest.cc',
+        'browser/ui/app_list/test/app_list_service_test_api.h',
+        'browser/ui/app_list/test/app_list_service_test_api_ash.cc',
+        'browser/ui/app_list/test/app_list_service_test_api_ash.h',
+        'browser/ui/app_list/test/app_list_service_test_api_mac.mm',
+        'browser/ui/app_list/test/app_list_service_test_api_win.cc',
         'browser/ui/ash/caps_lock_delegate_chromeos_browsertest.cc',
         'browser/ui/ash/chrome_shell_delegate_browsertest.cc',
         'browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc',

[commit-bot: I haz the power, 2013-09-09 19:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/23072036/179001

[commit-bot: I haz the power, 2013-09-09 22:18:37]

Change committed as 222107