Display an app's short name in the app launcher

Display an app's short name in the app launcher

The app launcher will now display an app's short name (the new
short_name property in the manifest file). If an app's short name
and full name differ, the tooltip will show the full name. If an
app's short name and full name are identical, a tooltip will only
be shown if the title is truncated.

BUG=226848
TEST=If an app has a short name and it differs from its full name,
a tooltip should always be shown, displaying the full name. If an
app does not have a short name override, there should be no change
in behavior, i.e. a tooltip will only be shown if the title is
truncated.

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

[tmdiep, 2013-08-29 07:57:47]

App launcher now displays the new "short_name" manifest property.

[benwells, 2013-08-29 09:14:15]

I started some try jobs for you, there may be some compile issues on some
platforms. Mac tooltip will need to be updated as well, but that can happen in a
separate patch (from tapted@ or jackhou@ might be easiest).

https://codereview.chromium.org/23709003/diff/1/chrome/browser/ui/app_list/ex...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/23709003/diff/1/chrome/browser/ui/app_list/ex...
chrome/browser/ui/app_list/extension_app_item.cc:105: SetTitle(extension_name_);
This will be the full name. Are you planning on fixing this separately?

https://codereview.chromium.org/23709003/diff/1/ui/app_list/test/app_list_tes...
File ui/app_list/test/app_list_test_model.h (right):

https://codereview.chromium.org/23709003/diff/1/ui/app_list/test/app_list_tes...
ui/app_list/test/app_list_test_model.h:34: void AddItem(const std::string&
title);
It isn't clear what this will use for full_name. Please move the version with
full_name to be first, then add a comment that this is a convenience version
which uses title as the full_name.

Or (better) give them names that are different and make it obvious.

https://codereview.chromium.org/23709003/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/23709003/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:135: string16() :
UTF8ToUTF16(model_->full_name()));
I think the correct indenting here is to go in another 4 spaces.

[tapted, 2013-08-30 01:51:29]

On 2013/08/29 09:14:15, benwells wrote:
> I started some try jobs for you, there may be some compile issues on some
> platforms. Mac tooltip will need to be updated as well, but that can happen in
a
> separate patch (from tapted@ or jackhou@ might be easiest).

Yeah - there's currently no tooltip on Mac (there should be! This is
http://crbug.com/271803 ). I'll wait for this to land, and add that fix ..
should be straightforward, but it might need something tricky to show the
tooltip properly it the small area above each icon.

Let me know if you need any help getting it compiling on mac. Although if you do
the comment below, you might end up not needing any mac changes. (Otherwise
you'll need to add a stub method to ItemModelObserverBridge in
apps_grid_view_item.mm)

https://codereview.chromium.org/23709003/diff/8001/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/23709003/diff/8001/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.h:34: void SetTitle(const std::string& title);
What do people think about changing this to

void SetTitle(const std::string& title, const std::string& full_name); ?

(or SetName(..))

I think they are always set together, and this would eliminate some inconsistent
states. E.g. Currently SetTitle is calling UpdateTooltip, but accessing the
full_name, which won't have been set yet. You then also won't need to update the
observer interface.

[tmdiep, 2013-08-30 02:48:13]

https://codereview.chromium.org/23709003/diff/1/chrome/browser/ui/app_list/ex...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/23709003/diff/1/chrome/browser/ui/app_list/ex...
chrome/browser/ui/app_list/extension_app_item.cc:105: SetTitle(extension_name_);
On 2013/08/29 09:14:15, benwells wrote:
> This will be the full name. Are you planning on fixing this separately?

Does this need to be handled? I see that when the extension/app is being
installed, the label is hidden and replaced with a progress bar. The name is not
displayed at all. At least on Windows anyway, I haven't tested on Mac yet.

https://codereview.chromium.org/23709003/diff/1/ui/app_list/test/app_list_tes...
File ui/app_list/test/app_list_test_model.h (right):

https://codereview.chromium.org/23709003/diff/1/ui/app_list/test/app_list_tes...
ui/app_list/test/app_list_test_model.h:34: void AddItem(const std::string&
title);
On 2013/08/29 09:14:15, benwells wrote:
> It isn't clear what this will use for full_name. Please move the version with
> full_name to be first, then add a comment that this is a convenience version
> which uses title as the full_name.
> 
> Or (better) give them names that are different and make it obvious.

Done.

https://codereview.chromium.org/23709003/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/23709003/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:135: string16() :
UTF8ToUTF16(model_->full_name()));
On 2013/08/29 09:14:15, benwells wrote:
> I think the correct indenting here is to go in another 4 spaces.

Done.

https://codereview.chromium.org/23709003/diff/8001/ui/app_list/app_list_item_...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/23709003/diff/8001/ui/app_list/app_list_item_...
ui/app_list/app_list_item_model.h:34: void SetTitle(const std::string& title);
On 2013/08/30 01:51:29, tapted wrote:
> What do people think about changing this to
> 
> void SetTitle(const std::string& title, const std::string& full_name); ?
> 
> (or SetName(..))
> 
> I think they are always set together, and this would eliminate some
inconsistent
> states. E.g. Currently SetTitle is calling UpdateTooltip, but accessing the
> full_name, which won't have been set yet. You then also won't need to update
the
> observer interface.

Agreed, I think this is a good idea.

[tmdiep, 2013-08-30 06:15:48]

Checked in a patch which renames AppListItemModel::SetTitle() to SetName() and
sets both the title and full name simultaneously.

AppListItemModelObserver::ItemFullNameChanged() was removed.

[benwells, 2013-08-30 06:25:07]

I started more bots for you....

https://codereview.chromium.org/23709003/diff/13001/ui/app_list/app_list_item...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/23709003/diff/13001/ui/app_list/app_list_item...
ui/app_list/app_list_item_model.h:34: void SetName(const std::string& title,
const std::string& full_name);
Nit: I'd prefer SetTitleAndFullName.

[tmdiep, 2013-08-30 07:29:51]

Addressed nit and also fixed compilation errors on mac.  Can someone please
start some bots for me? :) Thanks.

https://codereview.chromium.org/23709003/diff/13001/ui/app_list/app_list_item...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/23709003/diff/13001/ui/app_list/app_list_item...
ui/app_list/app_list_item_model.h:34: void SetName(const std::string& title,
const std::string& full_name);
On 2013/08/30 06:25:07, benwells wrote:
> Nit: I'd prefer SetTitleAndFullName.

Done.

[tapted, 2013-08-30 07:49:48]

On 2013/08/30 07:29:51, tmdiep wrote:
> Addressed nit and also fixed compilation errors on mac.  Can someone please
> start some bots for me? :) Thanks.

I'll start some now that run the app list unittests too ... tapted needs to add
these to the default jobs but I get petrified whenever I look at the buildbot
config files....

[benwells, 2013-08-30 08:17:39]

lgtm

[tmdiep, 2013-09-01 02:08:55]

On 2013/08/30 07:49:48, tapted wrote:
> On 2013/08/30 07:29:51, tmdiep wrote:
> > Addressed nit and also fixed compilation errors on mac.  Can someone please
> > start some bots for me? :) Thanks.
> 
> I'll start some now that run the app list unittests too ... tapted needs to
add
> these to the default jobs but I get petrified whenever I look at the buildbot
> config files....

Thanks tapted!

The AppsGridViewTest.MouseDrag app list unit test has been failing before this
patch. It seg faults at 
AppsGridView::StartSettingUpSynchronousDrag() because delegate_
(AppsGridViewDelegate) is null in the unit tests. I think the fix is simply to
check for null before calling delegate_->GetShortcutPathForApp(). Happy to check
in this fix in another CL.

[commit-bot: I haz the power, 2013-09-01 23:37:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/23709003/26001

[commit-bot: I haz the power, 2013-09-01 23:47:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tmdiep, 2013-09-01 23:50:51]

Adding sky as reviewer.

sky - Can you please review the changes in ash/shell/app_list.cc. Thanks.

[sky, 2013-09-03 16:07:30]

LGTM

[commit-bot: I haz the power, 2013-09-03 23:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/23709003/26001

[commit-bot: I haz the power, 2013-09-04 02:58:14]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-04 04:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/23709003/26001

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

Change committed as 221205