Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

Initialize the item title on creation/addition to shelf.
Watch for user window title changes in ShelfWindowWatcher.

Refine Arc app support in LauncherControllerHelper::GetAppTitle.
Remove ShelfModel::Status enum for app list item title changes on syncing.
(Change title in ChromeLauncherControllerImpl::OnAppSyncUIStatusChanged)

BUG=557406
TEST=No shelf item title/tooltip/application-menu changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2627533002
Cr-Commit-Position: refs/heads/master@{#442908}
Committed: https://chromium.googlesource.com/chromium/src/+/072dc547306ce82b35b6ef90f0917cbcaeb65534

[msw, 2017-01-10 21:45:03]

Patchset #2 (id:20001) has been deleted

[msw, 2017-01-10 21:46:54]

Description was changed from

==========
Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

BUG=557406
==========

to

==========
Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

Initialize the item title on creation/addition to shelf.
Watch for user window title changes in ShelfWindowWatcher.

Refine Arc app support in LauncherControllerHelper::GetAppTitle.
Remove ShelfModel::Status enum for app list item title changes on syncing.
(Change title in ChromeLauncherControllerImpl::OnAppSyncUIStatusChanged)

BUG=557406
TEST=No shelf item title/tooltip/application-menu changes.
R=jamescook@chromium.org
==========

[msw, 2017-01-10 21:46:54]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2017-01-10 21:47:18]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 21:48:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:33:30]

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

[commit-bot: I haz the power, 2017-01-10 22:33:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2017-01-11 00:30:33]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[msw, 2017-01-11 00:30:50]

Hey James, please take a look; thanks!

[commit-bot: I haz the power, 2017-01-11 00:31:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 01:07:00]

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

[commit-bot: I haz the power, 2017-01-11 01:07:01]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-11 01:23:38]

One question about ARC apps.

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.cc
(left):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.cc:40:
base::string16 ArcAppDeferredLauncherItemController::GetTitle() {
Q: Do ARC deferred items still have a tooltip with a name while they load?

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1284:
item.title = LauncherControllerHelper::GetAppTitle(profile(), app_id);
Is this where ARC apps get their title?

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1453: int
title_id = IDS_ASH_SHELF_APP_LIST_LAUNCHER_TITLE;
nit: maybe trigraph to avoid assigning then re-assigning?

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_controller_helper.cc (right):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_controller_helper.cc:113:
ArcAppWindowLauncherController::GetArcAppIdFromShelfAppId(app_id);
So before the |app_id| was sometimes a shelf app id and sometimes an arc id?

[msw, 2017-01-11 02:17:31]

Please take another look, thanks!
khmel@, feel free to review if you're interested.

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.cc
(left):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.cc:40:
base::string16 ArcAppDeferredLauncherItemController::GetTitle() {
On 2017/01/11 01:23:37, James Cook wrote:
> Q: Do ARC deferred items still have a tooltip with a name while they load?

I made ArcAppDeferredLauncherBrowserTest.StartAppDeferred check this (lmk if you
think I should have a separate test or set of such tests...). I can also
manually test that on a physical device soon. From the code,
ArcAppDeferredLauncherItemController is instantiated for existing items:
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/arc_app_d...
AFAIK, arc deferred ShelfItems are actually constructed in
ChromeLauncherControllerImpl::InsertAppLauncherItem (as you guessed), which
initializes the title for use while pinned, launching, and running.

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1284:
item.title = LauncherControllerHelper::GetAppTitle(profile(), app_id);
On 2017/01/11 01:23:38, James Cook wrote:
> Is this where ARC apps get their title?

Yes, afaict. I can test manually with a device soon, khmel@ might also know.
ArcAppDeferredLauncherController::RegisterDeferredLaunch calls
CreateAppLauncherItem, which calls this function.
ArcAppWindowLauncherController::RegisterApp also calls CreateAppLauncherItem via
ArcAppWindowLauncherController::AttachControllerToTask.

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1453: int
title_id = IDS_ASH_SHELF_APP_LIST_LAUNCHER_TITLE;
On 2017/01/11 01:23:38, James Cook wrote:
> nit: maybe trigraph to avoid assigning then re-assigning?

Done (and inlined).

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/launcher_controller_helper.cc (right):

https://codereview.chromium.org/2627533002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/launcher_controller_helper.cc:113:
ArcAppWindowLauncherController::GetArcAppIdFromShelfAppId(app_id);
On 2017/01/11 01:23:38, James Cook wrote:
> So before the |app_id| was sometimes a shelf app id and sometimes an arc id?

Not sure I understand your question exactly, but that function just maps the
Play Store app to the Arc platform host app, so it should be safe regardless.
The two arc item controller classes called it in their GetTitle overrides, I
believe adding it here is fixing a minor/trivial defect, but I'm not sure if
GetAppTitle was actually used for arc apps before my change.

[msw, 2017-01-11 02:17:37]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 02:18:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 02:54:18]

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

[commit-bot: I haz the power, 2017-01-11 02:54:19]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-11 15:09:38]

LGTM. Thanks for extending the test coverage.

[msw, 2017-01-11 15:10:50]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-01-11 15:11:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 15:16:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484147450650380,
"parent_rev": "87f1b6b8f0e0493ba7d1540abe795e13b313e141", "commit_rev":
"072dc547306ce82b35b6ef90f0917cbcaeb65534"}

[commit-bot: I haz the power, 2017-01-11 15:16:57]

Description was changed from

==========
Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

Initialize the item title on creation/addition to shelf.
Watch for user window title changes in ShelfWindowWatcher.

Refine Arc app support in LauncherControllerHelper::GetAppTitle.
Remove ShelfModel::Status enum for app list item title changes on syncing.
(Change title in ChromeLauncherControllerImpl::OnAppSyncUIStatusChanged)

BUG=557406
TEST=No shelf item title/tooltip/application-menu changes.
R=jamescook@chromium.org
==========

to

==========
Replace ShelfItemDelegate::GetTitle() with ShelfItem::title.

Initialize the item title on creation/addition to shelf.
Watch for user window title changes in ShelfWindowWatcher.

Refine Arc app support in LauncherControllerHelper::GetAppTitle.
Remove ShelfModel::Status enum for app list item title changes on syncing.
(Change title in ChromeLauncherControllerImpl::OnAppSyncUIStatusChanged)

BUG=557406
TEST=No shelf item title/tooltip/application-menu changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2627533002
Cr-Commit-Position: refs/heads/master@{#442908}
Committed:
https://chromium.googlesource.com/chromium/src/+/072dc547306ce82b35b6ef90f091...
==========

[commit-bot: I haz the power, 2017-01-11 15:16:58]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/072dc547306ce82b35b6ef90f091...