[Aura] Implement views-based applist.

[Aura] Implement views-based applist.

Implement a views-based applist behind "aura-views-applist" flag.

BUG=98308, 105913
TEST=Views-based app list should have no lag.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115515

[xiyuan, 2011-12-09 00:45:48]

Views-based app list, here's some info that might help with the review.

- app_list_result.* defines the data to be displayed in UI; It contains a
GetResults function that currently returns all extension apps in a similar
manner as in NTP4. That is, results are organized into groups and each group has
a flat list of result item.

- app_list_view.* defines AppListView that is the parent of all app list UI
views. It host ResultsView, which will display data from GetResults calls;

- results_view.* defines ResultsView. In a ResultsView, each result group is
represented by a TilesPageView and each item in result group is represented by a
TileView;


I got most NTP4 webui features except drag-n-drop changing apps order, scrolling
and context menu.

[Ben Goodger (Google), 2011-12-09 03:22:44]

Quick comment - seems like most of the ui bits should live in
ui/aura_shell, and just the profile-dependent model should live in Chrome.

-Ben

On Thu, Dec 8, 2011 at 4:45 PM, <xiyuan@chromium.org> wrote:

> Reviewers: sky,
>
> Message:
> Views-based app list, here's some info that might help with the review.
>
> - app_list_result.* defines the data to be displayed in UI; It contains a
> GetResults function that currently returns all extension apps in a similar
> manner as in NTP4. That is, results are organized into groups and each
> group has
> a flat list of result item.
>
> - app_list_view.* defines AppListView that is the parent of all app list UI
> views. It host ResultsView, which will display data from GetResults calls;
>
> - results_view.* defines ResultsView. In a ResultsView, each result group
> is
> represented by a TilesPageView and each item in result group is
> represented by a
> TileView;
>
>
> I got most NTP4 webui features except drag-n-drop changing apps order,
> scrolling
> and context menu.
>
>
> Description:
> [Aura] Implement views-based applist.
>
> Implement a views-based applist behind "aura-views-applist" flag.
>
> BUG=98308,105913
> TEST=Views-based app list should have no lag.
>
>
> Please review this at
http://codereview.chromium.**org/8890049/<http://codereview.chromium.org/8890...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  A chrome/browser/ui/views/aura/**app_list/app_list_result.h
>  A chrome/browser/ui/views/aura/**app_list/app_list_result.cc
>  A chrome/browser/ui/views/aura/**app_list/app_list_view.h
>  A chrome/browser/ui/views/aura/**app_list/app_list_view.cc
>  A chrome/browser/ui/views/aura/**app_list/extension_tile_view.h
>  A chrome/browser/ui/views/aura/**app_list/extension_tile_view.**cc
>  A chrome/browser/ui/views/aura/**app_list/results_view.h
>  A chrome/browser/ui/views/aura/**app_list/results_view.cc
>  A chrome/browser/ui/views/aura/**app_list/tile_view.h
>  A chrome/browser/ui/views/aura/**app_list/tile_view.cc
>  A chrome/browser/ui/views/aura/**app_list/tiles_page_view.h
>  A chrome/browser/ui/views/aura/**app_list/tiles_page_view.cc
>  M chrome/browser/ui/views/aura/**chrome_shell_delegate.cc
>  M chrome/chrome_browser.gypi
>  M ui/aura/aura_switches.h
>  M ui/aura/aura_switches.cc
>
>
>

[sky, 2011-12-09 04:29:32]

Look to the launcher for an example of this.

  -Scott

On Thu, Dec 8, 2011 at 7:22 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Quick comment - seems like most of the ui bits should live in ui/aura_shell,
> and just the profile-dependent model should live in Chrome.
>
> -Ben
>
>
> On Thu, Dec 8, 2011 at 4:45 PM, <xiyuan@chromium.org> wrote:
>>
>> Reviewers: sky,
>>
>> Message:
>> Views-based app list, here's some info that might help with the review.
>>
>> - app_list_result.* defines the data to be displayed in UI; It contains a
>> GetResults function that currently returns all extension apps in a similar
>> manner as in NTP4. That is, results are organized into groups and each
>> group has
>> a flat list of result item.
>>
>> - app_list_view.* defines AppListView that is the parent of all app list
>> UI
>> views. It host ResultsView, which will display data from GetResults calls;
>>
>> - results_view.* defines ResultsView. In a ResultsView, each result group
>> is
>> represented by a TilesPageView and each item in result group is
>> represented by a
>> TileView;
>>
>>
>> I got most NTP4 webui features except drag-n-drop changing apps order,
>> scrolling
>> and context menu.
>>
>>
>> Description:
>> [Aura] Implement views-based applist.
>>
>> Implement a views-based applist behind "aura-views-applist" flag.
>>
>> BUG=98308,105913
>> TEST=Views-based app list should have no lag.
>>
>>
>> Please review this at http://codereview.chromium.org/8890049/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  A chrome/browser/ui/views/aura/app_list/app_list_result.h
>>  A chrome/browser/ui/views/aura/app_list/app_list_result.cc
>>  A chrome/browser/ui/views/aura/app_list/app_list_view.h
>>  A chrome/browser/ui/views/aura/app_list/app_list_view.cc
>>  A chrome/browser/ui/views/aura/app_list/extension_tile_view.h
>>  A chrome/browser/ui/views/aura/app_list/extension_tile_view.cc
>>  A chrome/browser/ui/views/aura/app_list/results_view.h
>>  A chrome/browser/ui/views/aura/app_list/results_view.cc
>>  A chrome/browser/ui/views/aura/app_list/tile_view.h
>>  A chrome/browser/ui/views/aura/app_list/tile_view.cc
>>  A chrome/browser/ui/views/aura/app_list/tiles_page_view.h
>>  A chrome/browser/ui/views/aura/app_list/tiles_page_view.cc
>>  M chrome/browser/ui/views/aura/chrome_shell_delegate.cc
>>  M chrome/chrome_browser.gypi
>>  M ui/aura/aura_switches.h
>>  M ui/aura/aura_switches.cc
>>
>>
>

[sky, 2011-12-09 16:53:13]

As Ben suggested moving this to aura_shell will force you to think about this in
generic terms. In the end it seems like you are modeling a list of lists. The
the first list corresponds to a page, and the second list the contents of a
page. Seems like you should consider a model in these terms. The chrome side
would be responsible for populating and mutating the model, and ideally the rest
lives in aura_shell.

  -Scott

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list/app_list_result.cc (right):

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:42: Results* results,
Make results your last param since it may be modified (style guide say out
params should be last).
Document who owns returned ResultGroup*

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:76: ExtensionPrefs*
prefs;
members last

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:82:
DCHECK(result->type() == Result::EXTENSION_APP);
DCHECK_EQ

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list/app_list_result.h (right):

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:19: // Base class for
all app list result.
result -> results

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:35: Type type_;
Style guide doesn't like protected members, make it private. If you can, make it
const too.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:69: virtual
~ResultGroup();
Does this need to be virtual?

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:86: ResultList&
mutable_results() {
I don't like exposing the underlying list in a modifiable way. That breaks
encapsulation. Seems like the only reason you're exposing this is for sort.
Maybe sort should be a method on this?

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:98: class Results {
Can you think of a better name for this. Maybe ResultGroups or
ResultGroupContainer or something.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:105: // Adds a result
group.
document ownership.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:119: Results*
GetResults();
Having this be a function makes it harder for you to mock and test. Seems like
you would be better served by having a real model object that you can provide
different implementations.

[xiyuan, 2011-12-14 00:36:39]

Rebased and refactored:
- Refactored app_list_result.* into AppListModel, AppListItemGroup and
AppListItemModel;
- Moved the model base classes into aura_shell/app_list;
- Moved AppListView, TilesPageView, TileView code into aura_shell/app_list;
- Moved app_list.cc into app_list subdir;
- Added a ListModel to ui/base/models since AppListModel and AppListItemGroup
are all list-based models;
- Implemented a specific AppListModel for chrome and aura_shell_exe;

Please take another look. Thanks.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list/app_list_result.cc (right):

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:42: Results* results,
On 2011/12/09 16:53:17, sky wrote:
> Make results your last param since it may be modified (style guide say out
> params should be last).
> Document who owns returned ResultGroup*

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:76: ExtensionPrefs*
prefs;
On 2011/12/09 16:53:17, sky wrote:
> members last

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.cc:82:
DCHECK(result->type() == Result::EXTENSION_APP);
On 2011/12/09 16:53:17, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
File chrome/browser/ui/views/aura/app_list/app_list_result.h (right):

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:19: // Base class for
all app list result.
On 2011/12/09 16:53:17, sky wrote:
> result -> results

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:35: Type type_;
On 2011/12/09 16:53:17, sky wrote:
> Style guide doesn't like protected members, make it private. If you can, make
it
> const too.

Done. This should be private.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:69: virtual
~ResultGroup();
On 2011/12/09 16:53:17, sky wrote:
> Does this need to be virtual?

Removed.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:86: ResultList&
mutable_results() {
On 2011/12/09 16:53:17, sky wrote:
> I don't like exposing the underlying list in a modifiable way. That breaks
> encapsulation. Seems like the only reason you're exposing this is for sort.
> Maybe sort should be a method on this?

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:98: class Results {
On 2011/12/09 16:53:17, sky wrote:
> Can you think of a better name for this. Maybe ResultGroups or
> ResultGroupContainer or something.

Done. Refactored into AppListModel.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:105: // Adds a result
group.
On 2011/12/09 16:53:17, sky wrote:
> document ownership.

Done.

http://codereview.chromium.org/8890049/diff/1/chrome/browser/ui/views/aura/ap...
chrome/browser/ui/views/aura/app_list/app_list_result.h:119: Results*
GetResults();
On 2011/12/09 16:53:17, sky wrote:
> Having this be a function makes it harder for you to mock and test. Seems like
> you would be better served by having a real model object that you can provide
> different implementations.

Done as part of refactoring. It's now a member function of Chrome's
AppListModel.

[sky, 2011-12-14 16:51:33]

How about some tests for the models in the chrome side? I'm not too familiar
with extension code, could you get some from the extensions side to review your
the chrome model implementations?

  -Scott

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/app_list_model.cc (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:47: // Gets or creates
group for given |extension| in |model|. The created gruop
gruop -> group

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:79: return
GetExtensionLaunchOrdinal(a->extension(), prefs) <
Is this an expensive operation? Would it be better to cache the ordinal for
quicker comparison?

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:95: void
AppListModel::UpdateExtensionApps() {
This is pretty heavy for a constructor. How about renaming to Init and having
consumers invoke it themselves.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:96: Profile* profile =
ProfileManager::GetDefaultProfile();
Seems like this should be passed in the constructor.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:23: const
Extension* extension);
I don't know enough about the extension side of things. What is the life time of
an extension? Might it be deleted out from under you?

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_group_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_group_model.h:20: : public
ui::ListModel<AppListItemModel> {
Would this be better modeled by a has-a relation. That is, rather than extending
ui::ListModel make this have a ui::ListModel?

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_group_model.h:30: std::string title_;
const

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_model.h:31: virtual void Activate(int
event_flags);
This feels better served by a specific interface attached to the view.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_model.h:18: class AURA_SHELL_EXPORT AppListModel
Do you even need a new class for this? Can't this be a typedef?

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_view.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_view.h:27: AppListView(AppListModel* model,
Document ownership.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
File ui/aura_shell/app_list/results_view.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
ui/aura_shell/app_list/results_view.h:26: // ResultView displays UI for
AppListModel. Each group is displayed in a
displays the UI for an

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
ui/aura_shell/app_list/results_view.h:27: // page (TilesPageView) and each item
is displayed in a tile (TileView).
It would be nice if these names matched your model items. ResultsView ->
AppView, TilesPageView -> GroupView and TileView->ItemView . You could 'insert'
model in the names too, AppModelView, GroupModelView, ItemModelView, but that
isn't really necessary. Alternatively rename your models to match the names you
have here, but I think your model names are more consistent.

http://codereview.chromium.org/8890049/diff/10001/ui/base/models/list_model.h
File ui/base/models/list_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/base/models/list_model.h...
ui/base/models/list_model.h:17: // A list model that stores ItemType pointer.
Document ownership.

[xiyuan, 2011-12-14 18:01:25]

Evan, could you help with extension code? The logic is similar to your
AppLaunchHandler. Thanks.

[Evan Stade, 2011-12-14 22:59:20]

Is there any context for why you are adding this with Views? The explanation in
http://code.google.com/p/chromium/issues/detail?id=105913 seems to suggest
keeping a copy of the app list warm, not to re-implement it.

I ask because there seem to be a lot of observers missing from AppListModel,
such as extension added or removed or disabled or changing position or chrome's
online state changing or an app's notification changing. Reimplementing all of
the NTP app list is going to be an arduous and error-prone process; whereas the
current NTP is already well tested, new code would likely contain new bugs. If
the effort of building this native implementation were instead spent making the
NTP better (e.g., load faster or fixing bugs) then more users would benefit.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:19: class
ExtensionItemModel : public aura_shell::AppListItemModel,
I don't understand what else there are besides extension apps

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:23: const
Extension* extension);
On 2011/12/14 16:51:33, sky wrote:
> I don't know enough about the extension side of things. What is the life time
of
> an extension? Might it be deleted out from under you?

you'll get notified if it gets unloaded

[xiyuan, 2011-12-20 00:14:42]

CL updated to address comments in #2:
- Renamed views classes so that their names are more consistent and easier to
correlated them with model classes;
- Added AppListItemViewListener and AppListViewDelegate. Now, item activation is
propagate back to AppListView and then forwarded to AppListViewDeleage;
- Removed Extension* from ExtensionItem and always use extension id to get it
from ExtensionService so that unloading/disabling extension would not break app
list;
- Added unit test for Chrome's AppListModel per suggestion;

Other changes:
- Fixed a DropShadowLabel bug that shadow color gets into text when eliding fade
it used;
- Added BrowserCommandItem and added incognito and settings browser command to
app list;

Evan, as we have talked, app list looks similar to NTP apps page but it would be
a different feature. When user invokes app list, all other UI would fade out.
User would not be able to do much except picking an app or dismiss it. Things
such as installing/uninstalling/disabling app or changing app order is not
possible when app list is visible. We might need to handle app notifications but
I'll leave that until we have a clearer design.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/app_list_model.cc (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:47: // Gets or creates
group for given |extension| in |model|. The created gruop
On 2011/12/14 16:51:33, sky wrote:
> gruop -> group

Done.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:79: return
GetExtensionLaunchOrdinal(a->extension(), prefs) <
On 2011/12/14 16:51:33, sky wrote:
> Is this an expensive operation? Would it be better to cache the ordinal for
> quicker comparison?

Done. Launch ordinal is not stored in ExtensionItemModel.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:95: void
AppListModel::UpdateExtensionApps() {
On 2011/12/14 16:51:33, sky wrote:
> This is pretty heavy for a constructor. How about renaming to Init and having
> consumers invoke it themselves.

Done.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.cc:96: Profile* profile =
ProfileManager::GetDefaultProfile();
On 2011/12/14 16:51:33, sky wrote:
> Seems like this should be passed in the constructor.

Done.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:19: class
ExtensionItemModel : public aura_shell::AppListItemModel,
On 2011/12/14 22:59:20, Evan Stade wrote:
> I don't understand what else there are besides extension apps

From what I have heard from UX people, app list would also provide things such
as "Go incognito", "Settings" and current opened browser window/tabs besides
extension apps.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:23: const
Extension* extension);
On 2011/12/14 16:51:33, sky wrote:
> I don't know enough about the extension side of things. What is the life time
of
> an extension? Might it be deleted out from under you?

Changed to only keep extension_id around and check with extension service when
we need an Extension service.

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:23: const
Extension* extension);
On 2011/12/14 22:59:20, Evan Stade wrote:
> On 2011/12/14 16:51:33, sky wrote:
> > I don't know enough about the extension side of things. What is the life
time
> of
> > an extension? Might it be deleted out from under you?
> 
> you'll get notified if it gets unloaded

Not sure if we will setup notification for app list. Unlike NTP, app list is a
UI like omnibox's popup. We re-build (populate) the model when it is invoked.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_group_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_group_model.h:20: : public
ui::ListModel<AppListItemModel> {
On 2011/12/14 16:51:33, sky wrote:
> Would this be better modeled by a has-a relation. That is, rather than
extending
> ui::ListModel make this have a ui::ListModel?

Done.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_group_model.h:30: std::string title_;
On 2011/12/14 16:51:33, sky wrote:
> const

Done.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_model.h:31: virtual void Activate(int
event_flags);
On 2011/12/14 16:51:33, sky wrote:
> This feels better served by a specific interface attached to the view.

Done.

- Added AppListItemViewListener to propogate activation event back to
AppListView;
- Added an AppListViewDelegate that handles item activation for AppListView;

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_model.h:18: class AURA_SHELL_EXPORT AppListModel
On 2011/12/14 16:51:33, sky wrote:
> Do you even need a new class for this? Can't this be a typedef?

Not sure about whether we should use typedef here. If we do, then we need to
pull this header in all other headers that refers to this instead of forward
declare a class there.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_view.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_view.h:27: AppListView(AppListModel* model,
On 2011/12/14 16:51:33, sky wrote:
> Document ownership.

Done.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
File ui/aura_shell/app_list/results_view.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
ui/aura_shell/app_list/results_view.h:26: // ResultView displays UI for
AppListModel. Each group is displayed in a
On 2011/12/14 16:51:33, sky wrote:
> displays the UI for an

Done.

http://codereview.chromium.org/8890049/diff/10001/ui/aura_shell/app_list/resu...
ui/aura_shell/app_list/results_view.h:27: // page (TilesPageView) and each item
is displayed in a tile (TileView).
On 2011/12/14 16:51:33, sky wrote:
> It would be nice if these names matched your model items. ResultsView ->
> AppView, TilesPageView -> GroupView and TileView->ItemView . You could
'insert'
> model in the names too, AppModelView, GroupModelView, ItemModelView, but that
> isn't really necessary. Alternatively rename your models to match the names
you
> have here, but I think your model names are more consistent.

Done.

ResultsView -> AppListGroupsView
TilesPageView -> AppListItemGroupView
TileView -> AppListItemView

http://codereview.chromium.org/8890049/diff/10001/ui/base/models/list_model.h
File ui/base/models/list_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/ui/base/models/list_model.h...
ui/base/models/list_model.h:17: // A list model that stores ItemType pointer.
On 2011/12/14 16:51:33, sky wrote:
> Document ownership.

Done.

[Evan Stade, 2011-12-20 00:21:22]

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:19: class
ExtensionItemModel : public aura_shell::AppListItemModel,
On 2011/12/20 00:14:42, xiyuan wrote:
> On 2011/12/14 22:59:20, Evan Stade wrote:
> > I don't understand what else there are besides extension apps
> 
> From what I have heard from UX people, app list would also provide things such
> as "Go incognito", "Settings" and current opened browser window/tabs besides
> extension apps.

sounds like it should be called an app list then

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:23: const
Extension* extension);
On 2011/12/20 00:14:42, xiyuan wrote:
> On 2011/12/14 22:59:20, Evan Stade wrote:
> > On 2011/12/14 16:51:33, sky wrote:
> > > I don't know enough about the extension side of things. What is the life
> time
> > of
> > > an extension? Might it be deleted out from under you?
> > 
> > you'll get notified if it gets unloaded
> 
> Not sure if we will setup notification for app list. Unlike NTP, app list is a
> UI like omnibox's popup. We re-build (populate) the model when it is invoked.

sure, but you have to be careful because the apps list can still change out from
under you (either due to timing issues, or more likely because of sync).

[Evan Stade, 2011-12-20 00:26:35]

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item_model.h (right):

http://codereview.chromium.org/8890049/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item_model.h:19: class
ExtensionItemModel : public aura_shell::AppListItemModel,
On 2011/12/20 00:21:22, Evan Stade wrote:
> On 2011/12/20 00:14:42, xiyuan wrote:
> > On 2011/12/14 22:59:20, Evan Stade wrote:
> > > I don't understand what else there are besides extension apps
> > 
> > From what I have heard from UX people, app list would also provide things
such
> > as "Go incognito", "Settings" and current opened browser window/tabs besides
> > extension apps.
> 
> sounds like it should be called an app list then

should not*

[Ben Goodger (Google), 2011-12-20 19:07:18]

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/app_list_model.h (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.h:14: class AppListModel :
public aura_shell::AppListModel {
Is it necessary for Chrome to subclass the model here, or can the shell just
give Chrome the model and have it add to it through the public API?

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item.h (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item.h:20: class ExtensionItem :
public ChromeAppListItem,
ExtensionAppItem then?

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:75: new
aura_shell::AppListView(model.release(),
can you just return the model instance and do this init in the shell?

http://codereview.chromium.org/8890049/diff/18001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/8890049/diff/18001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_view.cc:72: void
AppListItemView::NotifyActivated(int event_flags) {
order in .cc should match .h, please check all files in this CL.

[xiyuan, 2011-12-20 21:55:50]

CL updated. Please take another look.

Could you also stamp on http://codereview.chromium.org/9010002/ so that I can
get the images in and start to run trybots.

Thanks.

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/app_list_model.h (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/app_list_model.h:14: class AppListModel :
public aura_shell::AppListModel {
On 2011/12/20 19:07:18, Ben Goodger (Google) wrote:
> Is it necessary for Chrome to subclass the model here, or can the shell just
> give Chrome the model and have it add to it through the public API?

Done. Changed this into a builder class that takes a AppListModel and populates
it instead of subclassing it.

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/app_list/extension_item.h (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/app_list/extension_item.h:20: class ExtensionItem :
public ChromeAppListItem,
On 2011/12/20 19:07:18, Ben Goodger (Google) wrote:
> ExtensionAppItem then?

Done.

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/chrome_shell_delegate.cc (right):

http://codereview.chromium.org/8890049/diff/18001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/chrome_shell_delegate.cc:75: new
aura_shell::AppListView(model.release(),
On 2011/12/20 19:07:18, Ben Goodger (Google) wrote:
> can you just return the model instance and do this init in the shell?

Done.

http://codereview.chromium.org/8890049/diff/18001/ui/aura_shell/app_list/app_...
File ui/aura_shell/app_list/app_list_item_view.cc (right):

http://codereview.chromium.org/8890049/diff/18001/ui/aura_shell/app_list/app_...
ui/aura_shell/app_list/app_list_item_view.cc:72: void
AppListItemView::NotifyActivated(int event_flags) {
On 2011/12/20 19:07:18, Ben Goodger (Google) wrote:
> order in .cc should match .h, please check all files in this CL.

Done.

[Ben Goodger (Google), 2011-12-21 21:11:02]

lgtm

[commit-bot: I haz the power, 2011-12-22 03:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8890049/34068

[commit-bot: I haz the power, 2011-12-22 11:17:04]

Change committed as 115515