Implement base class for app_list items.

Implement base class for app_list items.

Currently we have more than one type of app_list items. New base class
with common functionality is added and existing app_list items are
derived from it.

BUG=568397
TEST=Manually run Chrome with app_list on different platforms. No
     problem is detected.
TEST=unit_tests are passed

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

[khmel1, 2015-12-10 03:29:35]

khmel@google.com changed reviewers:
+ khmel@google.com, xiyuan@chromium.org

[khmel1, 2015-12-10 03:29:35]

Hi Xiyuan,

As we discussed here: https://codereview.chromium.org/1483593002/, It is
possible to implement base class for app_list items.

PTAL

[xiyuan, 2015-12-10 05:21:13]

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_item.h (right):

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_item.h:15: class
AppListControllerDelegate;
nit: seems not needed.

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/chrome_app_list_item.cc (right):

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.cc:44:
GetAppSorting()->EnsureValidOrdinals(id(), syncer::StringOrdinal());
EnsureValidOrdinals() has side effects of updating ExtensionPrefs, which is not
right when ChromeAppListItem is an ArcAppItem.

[khmel1, 2015-12-10 09:07:31]

Thanks for prompt feedback.

I reverted UpdateFromOrdering logic.

PTAL

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_item.h (right):

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_item.h:15: class
AppListControllerDelegate;
On 2015/12/10 05:21:13, xiyuan wrote:
> nit: seems not needed.

Yes, no longer used here

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/chrome_app_list_item.cc (right):

https://codereview.chromium.org/1507873006/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.cc:44:
GetAppSorting()->EnsureValidOrdinals(id(), syncer::StringOrdinal());
On 2015/12/10 05:21:13, xiyuan wrote:
> EnsureValidOrdinals() has side effects of updating ExtensionPrefs, which is
not
> right when ChromeAppListItem is an ArcAppItem.

Yes, I see that is important. Reverted to original behavior.

[xiyuan, 2015-12-10 16:57:48]

lgtm

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_item.cc (right):

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_item.cc:98: // Page is is the earliest
non-full page.
nit: remove one "is".

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/chrome_app_list_item.h (right):

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.h:22: } // namespace gfx
nit: two spaces between "}" and "// namespace gfx"

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.h:45:
DISALLOW_COPY_AND_ASSIGN(ChromeAppListItem);
nit: #include "base/macros.h"

[khmel1, 2015-12-11 06:05:48]

Thank you for review! nits were addressed

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_item.cc (right):

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_item.cc:98: // Page is is the earliest
non-full page.
On 2015/12/10 16:57:47, xiyuan wrote:
> nit: remove one "is".

Done.

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/chrome_app_list_item.h (right):

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.h:22: } // namespace gfx
On 2015/12/10 16:57:47, xiyuan wrote:
> nit: two spaces between "}" and "// namespace gfx"

Done.

https://codereview.chromium.org/1507873006/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/chrome_app_list_item.h:45:
DISALLOW_COPY_AND_ASSIGN(ChromeAppListItem);
On 2015/12/10 16:57:47, xiyuan wrote:
> nit: #include "base/macros.h"

Done.

[khmel1, 2015-12-11 06:05:56]

The CQ bit was checked by khmel@google.com

[khmel1, 2015-12-11 06:05:56]

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/1507873006/#ps60001
(title: "nits addressed")

[commit-bot: I haz the power, 2015-12-11 06:07:35]

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

[commit-bot: I haz the power, 2015-12-11 06:56:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-11 06:57:17]

Description was changed from

==========
Implement base class for app_list items.

Currently we have more than one type of app_list items. New base class
with common functionality is added and existing app_list items are
derived from it.

BUG=568397
TEST=Manually run Chrome with app_list on different platforms. No
     problem is detected.
TEST=unit_tests are passed
==========

to

==========
Implement base class for app_list items.

Currently we have more than one type of app_list items. New base class
with common functionality is added and existing app_list items are
derived from it.

BUG=568397
TEST=Manually run Chrome with app_list on different platforms. No
     problem is detected.
TEST=unit_tests are passed

Committed: https://crrev.com/e14c43f74d4efd9fc2298273b937f9416e75bd05
Cr-Commit-Position: refs/heads/master@{#364639}
==========

[commit-bot: I haz the power, 2015-12-11 06:57:19]

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