Combine shelf platform and windowed app types.

Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org

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

[msw, 2016-11-23 20:31:16]

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

[commit-bot: I haz the power, 2016-11-23 20:32:43]

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

[msw, 2016-11-23 20:43:29]

Description was changed from

==========
Combine shelf platform and windowed app types.

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

to

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org
==========

[msw, 2016-11-23 20:43:30]

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

[msw, 2016-11-23 20:43:56]

Hey Stefan, please take a look; thanks!

[commit-bot: I haz the power, 2016-11-23 22:23:24]

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

[commit-bot: I haz the power, 2016-11-23 22:23:25]

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

[msw, 2016-11-23 22:49:22]

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

[commit-bot: I haz the power, 2016-11-23 22:50:06]

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

[Mr4D (OOO till 08-26), 2016-11-23 23:11:51]

Please see comments!

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_types.h (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:34: TYPE_APP,
We should probably name this TYPE_WEB_APP - or TYPE_BROWSER_APP.

Otherwise that gets confusing with the ARC_APP's and what else we will add next.

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_unittes...
File ash/shelf/shelf_unittest.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_unittes...
ash/shelf/shelf_unittest.cc:49: // Add a running app.
.. running browser apps.

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:360: ShelfID AddAppNoWait() {
I suppose these names too...

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

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:594:
LauncherItemController* controller = GetLauncherItemController(id);
You might want to dcheck the controller?

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1656:
EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
Hmmm. Why did you remove this test? After pinning the item should be pinned?

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1641:
EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
Note: that you also test for not being pinned after unpinning...

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:97:
!extension_misc::IsImeMenuExtensionId(
You have effectively removed the pinning option - unless I am missing something?

[msw, 2016-11-24 00:00:55]

Thanks for the feedback, I've responded to your comments inline.

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_types.h (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:34: TYPE_APP,
On 2016/11/23 23:11:51, Mr4D wrote:
> We should probably name this TYPE_WEB_APP - or TYPE_BROWSER_APP.
> 
> Otherwise that gets confusing with the ARC_APP's and what else we will add
next.

I don't think we should add another item type for arc apps. My hope is to
simplify here first, then move things over to ShelfWindowWatcher, as I've
already done for panels. Some type differences should be expressible in other
ways, for example TYPE_APP_SHORTCUT is just TYPE_APP with STATUS_CLOSED. Context
menu items should be populated on the Extension/Arc side (which can still
discern the app type from the app id), and Chrome will have to interact with the
shelf more directly for V1 apps when they're opened as tabs, but otherwise I'd
like to minimize Chrome/Ash interaction, and knowledge of app types, etc. This
is still in the works, and I'm open to ideas, but I think reducing the set of
item types here makes more sense. WDYT?

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_unittes...
File ash/shelf/shelf_unittest.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_unittes...
ash/shelf/shelf_unittest.cc:49: // Add a running app.
On 2016/11/23 23:11:51, Mr4D wrote:
> .. running browser apps.

Ditto.

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_view_un...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/shelf/shelf_view_un...
ash/shelf/shelf_view_unittest.cc:360: ShelfID AddAppNoWait() {
On 2016/11/23 23:11:51, Mr4D wrote:
> I suppose these names too...

Ditto.

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

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:594:
LauncherItemController* controller = GetLauncherItemController(id);
On 2016/11/23 23:11:51, Mr4D wrote:
> You might want to dcheck the controller?

I don't usually DCHECK immediately before a dereference; line 597 will crash if
it's null; but I can add a DCHECK if you still prefer that.

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1656:
EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
On 2016/11/23 23:11:51, Mr4D wrote:
> Hmmm. Why did you remove this test? After pinning the item should be pinned?

This is redundant with line 1621, right?

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1641:
EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
On 2016/11/23 23:11:51, Mr4D wrote:
> Note: that you also test for not being pinned after unpinning...

Would you like me to add a comment here, like "Ensure the item is not pinned
after unpinning."? I ask because that seems too self-explanatory for a special
one-off comment, and in general these tests don't give step-by-step comments
like that.

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:97:
!extension_misc::IsImeMenuExtensionId(
On 2016/11/23 23:11:51, Mr4D wrote:
> You have effectively removed the pinning option - unless I am missing
something?

Pinning was only added here for TYPE_PLATFORM_APP, which is now handled above
for TYPE_APP. This else block no longer handles the app type.

[commit-bot: I haz the power, 2016-11-24 00:53:04]

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

[commit-bot: I haz the power, 2016-11-24 00:53:04]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[msw, 2016-11-24 01:32:00]

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

[commit-bot: I haz the power, 2016-11-24 01:33:30]

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

[Mr4D (OOO till 08-26), 2016-11-24 02:21:33]

Please see email. Beside that lgtm.

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_types.h (right):

https://codereview.chromium.org/2518423003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:34: TYPE_APP,
See longer eMail.

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

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:594:
LauncherItemController* controller = GetLauncherItemController(id);
I'd like that - Thanks!

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1656:
EXPECT_TRUE(launcher_controller_->IsAppPinned(extension1_->id()));
Yes. There it is. Never mind.

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1641:
EXPECT_FALSE(launcher_controller_->IsAppPinned(extension1_->id()));
No need, that was meant for the comment above which is resolved. Thanks!

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (right):

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:97:
!extension_misc::IsImeMenuExtensionId(
Ok. Thanks!

[commit-bot: I haz the power, 2016-11-24 03:34:57]

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

[commit-bot: I haz the power, 2016-11-24 03:34:58]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-11-28 20:11:55]

Description was changed from

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org
==========

to

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org
==========

[msw, 2016-11-28 20:11:55]

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

[msw, 2016-11-28 20:13:31]

James, please take a look for ash/OWNERS review; thanks!

Stefan, I updated the unit tests to support the new manner of distinguishing
platform apps on the chrome side. Feel free to take another look; thanks!

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

https://codereview.chromium.org/2518423003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:594:
LauncherItemController* controller = GetLauncherItemController(id);
On 2016/11/24 02:21:33, Mr4D wrote:
> I'd like that - Thanks!

Done.

[msw, 2016-11-28 20:13:40]

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

[commit-bot: I haz the power, 2016-11-28 20:16:06]

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, 2016-11-28 22:26:26]

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

[commit-bot: I haz the power, 2016-11-28 22:26:26]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-11-28 22:57:48]

LGTM with nits. Nice patch.

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_types.h (right):

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:33: // Represents an app (platform/V2 or
windowed V1 browser).
Can you clarify the various sorts of apps in this comment? Like, is "packaged"
one of them? And aren't there 3 types?

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:40: TYPE_IME_MENU,
Not for this CL: This might be dead code. I think the IME folks re-implemented
it as a system tray item, not a shelf item.

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:589: case TYPE_APP:
Thanks for reordering. These details help.

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:120: // against a
users un-pinning removal.
Did you ever figure out what "locking" is used for? Enterprise policy stuff? If
so, feel free to expand this comment slightly. If not, don't bother.

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

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1567:
EXPECT_EQ(0, launcher_controller_->GetShelfIDForAppID(extension1_->id()));
kInvalidShelfId?

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

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:54:
!extension_misc::IsImeMenuExtensionId(
Likewise, I wonder if this code is still used. Not for this CL, but azurewei@
might know if the IME menu extension code can be deleted.

[msw, 2016-11-28 23:47:41]

+azurewei@ for thoughts and/or followup on IME comments.

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_item_types.h (right):

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:33: // Represents an app (platform/V2 or
windowed V1 browser).
On 2016/11/28 22:57:47, James Cook wrote:
> Can you clarify the various sorts of apps in this comment? Like, is "packaged"
> one of them? And aren't there 3 types?

I don't feel highly experienced here, but I tried to enumerate the app types...
Really, the shelf should be agnostic to app types, so I'm not sure how useful
this list is here. Lmk what you think.

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_item_types.h:40: TYPE_IME_MENU,
On 2016/11/28 22:57:47, James Cook wrote:
> Not for this CL: This might be dead code. I think the IME folks re-implemented
> it as a system tray item, not a shelf item.

+azurewei@ for thoughts and/or followup.

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2518423003/diff/60001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_view.cc:589: case TYPE_APP:
On 2016/11/28 22:57:47, James Cook wrote:
> Thanks for reordering. These details help.

Acknowledged.

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:120: // against a
users un-pinning removal.
On 2016/11/28 22:57:47, James Cook wrote:
> Did you ever figure out what "locking" is used for? Enterprise policy stuff?
If
> so, feel free to expand this comment slightly. If not, don't bother.

The comment seems reasonable (albeit incomplete), so I expanded a *tiny* bit.
Afaict, locking is simply the way V1 apps (eg. Chrome Web Store) are added to
the shelf as running, when their 'Open as window' context menu option is checked
and they are not pinned to the shelf. It seems like 'lock/unlock' are misnomers
for 'add/remove'. (found via experimentation and this comment and code path):
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.h?l=891
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/browser_s...

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

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1567:
EXPECT_EQ(0, launcher_controller_->GetShelfIDForAppID(extension1_->id()));
On 2016/11/28 22:57:47, James Cook wrote:
> kInvalidShelfId?

Yeah... I was using 0 to follow other checks in this file and avoid line
wrapping, but I changed all appropriate occurrences.

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

https://codereview.chromium.org/2518423003/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:54:
!extension_misc::IsImeMenuExtensionId(
On 2016/11/28 22:57:48, James Cook wrote:
> Likewise, I wonder if this code is still used. Not for this CL, but azurewei@
> might know if the IME menu extension code can be deleted.

+azurewei@ for thoughts and/or followup.

[msw, 2016-11-28 23:47:54]

The CQ bit was checked by msw@chromium.org

[msw, 2016-11-28 23:47:54]

The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org,
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2518423003/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-28 23:48:59]

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

[Mr4D (OOO till 08-26), 2016-11-29 00:13:48]

Still lgtm

[msw, 2016-11-29 01:27:45]

The CQ bit was checked by msw@chromium.org

[msw, 2016-11-29 01:27:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2518423003/#ps100001
(title: "Fix kInvalidShelfID typo.")

[commit-bot: I haz the power, 2016-11-29 01:28:51]

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

[James Cook, 2016-11-29 01:32:52]

still lgtm

[commit-bot: I haz the power, 2016-11-29 03:33:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480382865231000,
"parent_rev": "f1d76e4cc4eb80f40f306b0eaf927b4b3779b036", "commit_rev":
"e8dfc8466859034d19653c4e75fdeef2f9535c6f"}

[commit-bot: I haz the power, 2016-11-29 03:34:18]

Description was changed from

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org
==========

to

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-11-29 03:34:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-29 03:39:49]

Description was changed from

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org
==========

to

==========
Combine shelf platform and windowed app types.

Combine ash::TYPE_PLATFORM_APP and TYPE_WINDOWED_APP.
(V1 and V2 apps can be distinguished by IsPlatformApp functions)

Remove ChromeLauncherController::IsWindowedAppInLauncher.
(check for item presence and pinned state by id instead)

Remove unused LauncherItemController::TYPE_WINDOWED_APP.
The only behavior change *may* be the combination of drag groups.
(but afaict, we can already mix V1 and V2 app order on the shelf...)

Update users and tests.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes for apps.
R=skuhne@chromium.org,jamescook@chromium.org

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

[commit-bot: I haz the power, 2016-11-29 03:39:53]

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