Simplifies the structure of app_list search a bit.

Simplifies the structure of app_list search a bit.

This is the first CL to simplify app list search. The final goal
is making SearchResult copyable and remove SearchResultObserver
but it would need the several steps. As the first step, it
simply keeps the copying method Duplicate but removes the
unnecessary class hierarchy of ChromeSearchResult.

BUG=415500
R=oshima@chromium.org, tapted@chromium.org, xiyuan@chromium.org
TEST=build passes, no functional changes

Committed: https://crrev.com/d29ce06a1354e06e41596cf575af98aed9f6b8a7
Cr-Commit-Position: refs/heads/master@{#298154}

[tapted, 2014-10-02 07:19:21]

seems like a reasonable change, but wait for xiyuan - just a couple of questions
and some nits

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
athena/home/app_list_view_delegate.cc:57: model_->results()->DeleteAll();
Does this cause flickering of the search results in some cases? I think mixer.cc
has special code to prevent this, but it might not affect Athena

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
athena/home/app_list_view_delegate.cc:59:
model_->results()->Add(results[i]->Duplicate().release());
(no action required) would be nice if this .release() could be a .Pass() but I
guess ListModel doesn't support that :/

Moving the SearchResults typedef and having
`SearchResult::CopyToList(SearchResults* result_list);` might be another option
(and here: `results[i]->CopyToList(&model_->results())`), but it's a bit
backward.

A third option: AppListModel::AddSearchResult?

But what's here is fine.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/app_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/app_result.h:52: // SearchResult overides:
nit (while you're here) overides -> overrides. Or even just `SearchResult:` is
becoming popular, but it wouldn't be consistent with the other app_list code
yet.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/mixer_unittest.cc (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/mixer_unittest.cc:31: // SearchResult
overides:
nit: overide -> overrides

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/people/people_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/people/people_result.h:26: // SearchResult
overides:
nit overides -> overrides

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/search_webstore_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/search_webstore_result.h:24: // SearchResult
overides:
nit: overides -> overrides

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/webstore/webstore_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/webstore/webstore_result.h:47: // SearchResult
overides:
nit: you know the drill :)

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h#...
ui/app_list/search_result.h:139: virtual scoped_ptr<SearchResult> Duplicate();
Can these be pure virtual? I think I'd prefer the tests versions the only places
that do NOTREACHED.

[xiyuan, 2014-10-02 20:47:08]

LGTM % tapted's nits

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h#...
ui/app_list/search_result.h:27: enum SearchResultType {
This is chrome specific and probably should not live under ui. However, it seems
to be only used for UMA at the moment and we might be able to remove it by
tracking which provider generates the result. So, maybe add a TODO to get rid of
it?

[Jun Mukai, 2014-10-03 01:24:24]

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
File athena/home/app_list_view_delegate.cc (right):

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
athena/home/app_list_view_delegate.cc:57: model_->results()->DeleteAll();
On 2014/10/02 07:19:21, tapted wrote:
> Does this cause flickering of the search results in some cases? I think
mixer.cc
> has special code to prevent this, but it might not affect Athena

Right, this actually causes flickering unfortunately.
I don't care much about this because the current code is hacky anyways. I will
move mixer ui/app_list and use it from here later.

https://codereview.chromium.org/621823004/diff/1/athena/home/app_list_view_de...
athena/home/app_list_view_delegate.cc:59:
model_->results()->Add(results[i]->Duplicate().release());
On 2014/10/02 07:19:21, tapted wrote:
> (no action required) would be nice if this .release() could be a .Pass() but I
> guess ListModel doesn't support that :/
> 
> Moving the SearchResults typedef and having
> `SearchResult::CopyToList(SearchResults* result_list);` might be another
option
> (and here: `results[i]->CopyToList(&model_->results())`), but it's a bit
> backward.
> 
> A third option: AppListModel::AddSearchResult?
> 
> But what's here is fine.

Agreed. I think ideally ListModel should support it...

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/app_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/app_result.h:52: // SearchResult overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit (while you're here) overides -> overrides. Or even just `SearchResult:` is
> becoming popular, but it wouldn't be consistent with the other app_list code
> yet.

Done.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/mixer_unittest.cc (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/mixer_unittest.cc:31: // SearchResult
overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit: overide -> overrides

Done.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/mixer_unittest.cc:31: // SearchResult
overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit: overide -> overrides

Done.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/people/people_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/people/people_result.h:26: // SearchResult
overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit overides -> overrides

Done.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/search_webstore_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/search_webstore_result.h:24: // SearchResult
overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit: overides -> overrides

Done.

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/webstore/webstore_result.h (right):

https://codereview.chromium.org/621823004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/webstore/webstore_result.h:47: // SearchResult
overides:
On 2014/10/02 07:19:21, tapted wrote:
> nit: you know the drill :)

Done.

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h#...
ui/app_list/search_result.h:27: enum SearchResultType {
On 2014/10/02 20:47:08, xiyuan wrote:
> This is chrome specific and probably should not live under ui. However, it
seems
> to be only used for UMA at the moment and we might be able to remove it by
> tracking which provider generates the result. So, maybe add a TODO to get rid
of
> it?

Thinking about this, I think it's better not to do this, and your approach
(recording histogram on each of search result object) would be preferable.  This
approach, I will have to edit all of testing / mocky search result subclasses
and once when this enum moves back, I will have to remove them, which doesn't
look great.

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h#...
ui/app_list/search_result.h:139: virtual scoped_ptr<SearchResult> Duplicate();
On 2014/10/02 07:19:21, tapted wrote:
> Can these be pure virtual? I think I'd prefer the tests versions the only
places
> that do NOTREACHED.

Done.

[Jun Mukai, 2014-10-03 01:24:43]

mukai@chromium.org changed reviewers:
+ oshima@chromium.org

[Jun Mukai, 2014-10-03 01:24:56]

+oshima for ash / athena?

[oshima, 2014-10-03 01:57:49]

ash/athena lgtm

[tapted, 2014-10-03 02:59:18]

lgtm

(and I think there might be a CQ step that tweaks the include headers - not sure
if it's in place post-git - but it can make a mess of things once the commit
doesn't match rietveld)

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h
File ui/app_list/search_result.h (right):

https://codereview.chromium.org/621823004/diff/1/ui/app_list/search_result.h#...
ui/app_list/search_result.h:27: enum SearchResultType {
On 2014/10/03 01:24:23, Jun Mukai wrote:
> On 2014/10/02 20:47:08, xiyuan wrote:
> > This is chrome specific and probably should not live under ui. However, it
> seems
> > to be only used for UMA at the moment and we might be able to remove it by
> > tracking which provider generates the result. So, maybe add a TODO to get
rid
> of
> > it?
> 
> Thinking about this, I think it's better not to do this, and your approach
> (recording histogram on each of search result object) would be preferable. 
This
> approach, I will have to edit all of testing / mocky search result subclasses
> and once when this enum moves back, I will have to remove them, which doesn't
> look great.

looks nice :)

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_util.cc (right):

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_util.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_util.h (right):

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_util.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
File ui/app_list/test/test_search_result.cc (right):

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
ui/app_list/test/test_search_result.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
File ui/app_list/test/test_search_result.h (right):

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
ui/app_list/test/test_search_result.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: no (c)

[Jun Mukai, 2014-10-03 20:23:33]

The CQ bit was checked by mukai@chromium.org

[Jun Mukai, 2014-10-03 20:23:36]

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_util.cc (right):

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_util.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/10/03 02:59:17, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/search_util.h (right):

https://codereview.chromium.org/621823004/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/search_util.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/10/03 02:59:17, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
File ui/app_list/test/test_search_result.cc (right):

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
ui/app_list/test/test_search_result.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/10/03 02:59:17, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
File ui/app_list/test/test_search_result.h (right):

https://codereview.chromium.org/621823004/diff/20001/ui/app_list/test/test_se...
ui/app_list/test/test_search_result.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/10/03 02:59:17, tapted wrote:
> nit: no (c)

Done.

[commit-bot: I haz the power, 2014-10-03 20:23:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621823004/40001

[commit-bot: I haz the power, 2014-10-03 20:27:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-03 20:27:49]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/64184)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/69006)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jun Mukai, 2014-10-03 20:44:11]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-03 20:45:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621823004/60001

[commit-bot: I haz the power, 2014-10-03 22:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-03 22:50:25]

Exceeded time limit waiting for builds to trigger.

[Jun Mukai, 2014-10-04 00:31:34]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-10-04 00:33:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621823004/60001

[commit-bot: I haz the power, 2014-10-04 00:51:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-04 00:51:32]

Failed to apply patch for chrome/browser/ui/app_list/search/app_result.h:
While running git apply --index -3 -p1;
  error: patch failed: chrome/browser/ui/app_list/search/app_result.h:49
  Falling back to three-way merge...
  Applied patch to 'chrome/browser/ui/app_list/search/app_result.h' with
conflicts.
  U chrome/browser/ui/app_list/search/app_result.h

Patch:       chrome/browser/ui/app_list/search/app_result.h
Index: chrome/browser/ui/app_list/search/app_result.h
diff --git a/chrome/browser/ui/app_list/search/app_result.h
b/chrome/browser/ui/app_list/search/app_result.h
index
aebf662f31f3598b1b5ea4d58dfd47405f7bcb0d..41dcd15a95fb36cd368369a457fd240b3ee76c72
100644
--- a/chrome/browser/ui/app_list/search/app_result.h
+++ b/chrome/browser/ui/app_list/search/app_result.h
@@ -9,10 +9,10 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/app_list/app_context_menu_delegate.h"
-#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow_delegate.h"
 #include "extensions/browser/extension_icon_image.h"
 #include "extensions/browser/extension_registry_observer.h"
+#include "ui/app_list/search_result.h"
 
 class AppListControllerDelegate;
 class ExtensionEnableFlow;
@@ -32,7 +32,7 @@ class AppContextMenu;
 class TokenizedString;
 class TokenizedStringMatch;
 
-class AppResult : public ChromeSearchResult,
+class AppResult : public SearchResult,
                   public extensions::IconImage::Observer,
                   public AppContextMenuDelegate,
                   public ExtensionEnableFlowDelegate,
@@ -49,12 +49,10 @@ class AppResult : public ChromeSearchResult,
   void UpdateFromLastLaunched(const base::Time& current_time,
                               const base::Time& last_launched);
 
-  // ChromeSearchResult overides:
+  // SearchResult overrides:
   virtual void Open(int event_flags) OVERRIDE;
-  virtual void InvokeAction(int action_index, int event_flags) OVERRIDE;
-  virtual scoped_ptr<ChromeSearchResult> Duplicate() OVERRIDE;
+  virtual scoped_ptr<SearchResult> Duplicate() OVERRIDE;
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
-  virtual ChromeSearchResultType GetType() OVERRIDE;
 
  private:
   void StartObservingExtensionRegistry();

[commit-bot: I haz the power, 2014-10-04 00:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-04 01:28:03]

Patchset 6 (id:??) landed as
https://crrev.com/d29ce06a1354e06e41596cf575af98aed9f6b8a7
Cr-Commit-Position: refs/heads/master@{#298154}

[Jun Mukai, 2014-10-04 01:28:03]

Committed patchset #6 (id:100001) manually as
d29ce06a1354e06e41596cf575af98aed9f6b8a7 (presubmit successful).