Support uninstalling ARC app from Chrome launcher

Support uninstalling ARC app from Chrome launcher

This changes renders an entry to allow uninstalling an app from Chrome
launcher.  Apps that are installed by the user should be uninstallable,
while pre-installed apps are not.  Currently, the menu entry is not
rendered for pre-installed apps.  In the future, we should also respect
device policy.

BUG=b/24569986
TEST=Installed an app and uninstalled from Chrome launcher.
TEST=Didn't see "uninstall" entry in the menu for pre-installed apps
TEST=Forcely calls UninstallApp mojo against pre-installed packages
     failed naturally.  However, if the package is overridden by later
     version, uninstall will success (presumably rollback to factory
     version).

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

[victorhsieh0, 2016-03-04 18:30:20]

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

[victorhsieh0, 2016-03-04 18:30:21]

dcheng@ PTAL at the mojo change.
others: PTAL

[khmel, 2016-03-04 18:35:53]

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:31:
menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
Did you pass unit_tests? They should be updated as well

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:34:
IDS_EXTENSION_EXTERNAL_INSTALL_PROMPT_ABORT_BUTTON);
Probably we need dedicated message for this, not extension based

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:58: ArcAppListPrefs*
arc_prefs = ArcAppListPrefs::Get(profile());
Put this log to arc_app_utils

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:67: return app_info &&
app_info->uninstallable;
I think app mast be ready too to be able to unistall.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.h (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.h:37: bool isUninstallable()
const;
 I sUninstallable?

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:116: if
(bridge_service->app_version() >= 2) {
If you have UninstallApp available you already have version >= 2. Luis could you
please confirm?

[Luis Héctor Chávez, 2016-03-04 21:29:37]

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:48: if (command_id ==
LAUNCH_NEW) {
nit: Probably a switch is better now.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:116: if
(bridge_service->app_version() >= 2) {
On 2016/03/04 18:35:53, khmel wrote:
> If you have UninstallApp available you already have version >= 2. Luis could
you
> please confirm?

You will always have the method available (compile-time), but may only use it
the other side has version >= 2 (runtime). The explicit check is needed.

nit:

if (bridge_service->app_version() < 2) {
  VLOG(2) << "Request to uninstall app when version " <<
bridge_service->app_version()
          << " does not support it.";
  return false;
}

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:95: [MinVersion=2] UninstallApp(string package);
Can you instead do

[MinVersion=2] UninstallApp(string package) => (string error_message);

? That is probably going to simplify some stuff.

[khmel, 2016-03-04 22:02:40]

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:95: [MinVersion=2] UninstallApp(string package);
On 2016/03/04 21:29:37, Luis Héctor Chávez wrote:
> Can you instead do
> 
> [MinVersion=2] UninstallApp(string package) => (string error_message);
> 
> ? That is probably going to simplify some stuff.

Do we really want sync method? This is UI thread. IMHO we should do async here

[Luis Héctor Chávez, 2016-03-04 22:05:15]

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:95: [MinVersion=2] UninstallApp(string package);
On 2016/03/04 22:02:40, khmel wrote:
> On 2016/03/04 21:29:37, Luis Héctor Chávez wrote:
> > Can you instead do
> > 
> > [MinVersion=2] UninstallApp(string package) => (string error_message);
> > 
> > ? That is probably going to simplify some stuff.
> 
> Do we really want sync method? This is UI thread. IMHO we should do async here

Mojo has no support for sync IPC. This is async :) A second parameter to
UninstallApp() when invoked from C++ will be a
base::Callback<void(mojo::String)>.

[victorhsieh0, 2016-03-07 21:00:16]

PTAL

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:31:
menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
On 2016/03/04 18:35:53, khmel wrote:
> Did you pass unit_tests? They should be updated as well

Done.  Test added.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:34:
IDS_EXTENSION_EXTERNAL_INSTALL_PROMPT_ABORT_BUTTON);
On 2016/03/04 18:35:53, khmel wrote:
> Probably we need dedicated message for this, not extension based

IDS_APP_LIST_UNINSTALL_ITEM looks like the one for Chrome Apps.  I switched to
that one.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:48: if (command_id ==
LAUNCH_NEW) {
On 2016/03/04 21:29:37, Luis Héctor Chávez wrote:
> nit: Probably a switch is better now.

Done.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:58: ArcAppListPrefs*
arc_prefs = ArcAppListPrefs::Get(profile());
On 2016/03/04 18:35:53, khmel wrote:
> Put this log to arc_app_utils

Done.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:67: return app_info &&
app_info->uninstallable;
On 2016/03/04 18:35:53, khmel wrote:
> I think app mast be ready too to be able to unistall.

Done.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.h (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_context_menu.h:37: bool isUninstallable()
const;
On 2016/03/04 18:35:53, khmel wrote:
>  I sUninstallable?

Done.

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/1756193008/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:116: if
(bridge_service->app_version() >= 2) {
On 2016/03/04 21:29:37, Luis Héctor Chávez wrote:
> On 2016/03/04 18:35:53, khmel wrote:
> > If you have UninstallApp available you already have version >= 2. Luis could
> you
> > please confirm?
> 
> You will always have the method available (compile-time), but may only use it
> the other side has version >= 2 (runtime). The explicit check is needed.
> 
> nit:
> 
> if (bridge_service->app_version() < 2) {
>   VLOG(2) << "Request to uninstall app when version " <<
> bridge_service->app_version()
>           << " does not support it.";
>   return false;
> }

Done.

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1756193008/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:95: [MinVersion=2] UninstallApp(string package);
On 2016/03/04 22:05:15, Luis Héctor Chávez wrote:
> On 2016/03/04 22:02:40, khmel wrote:
> > On 2016/03/04 21:29:37, Luis Héctor Chávez wrote:
> > > Can you instead do
> > > 
> > > [MinVersion=2] UninstallApp(string package) => (string error_message);
> > > 
> > > ? That is probably going to simplify some stuff.
> > 
> > Do we really want sync method? This is UI thread. IMHO we should do async
here
> 
> Mojo has no support for sync IPC. This is async :) A second parameter to
> UninstallApp() when invoked from C++ will be a
> base::Callback<void(mojo::String)>.

Done.

[victorhsieh0, 2016-03-07 21:01:21]

btw, I renamed uninstallable to sticky for better readability...

[khmel, 2016-03-07 21:11:52]

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

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_context_menu_unittest.cc:409: ASSERT_EQ(2,
menu->GetItemCount());
I think pinnable item is missing. I would prefer to keep this test close to real
life, were arc app is pinnable. Please add controller()->SetAppPinnable(app_id,
AppListControllerDelegate::PIN_EDITABLE);

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

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.h:41: static void
OnUninstallAppResponse(const mojo::String& error_message);
I don't think ArcAppContextMenu is good place to handle OnUninstallAppResponse

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:30: arc::AppInfo store_app;
nit: Can we do it more gracefully? For example  get first app and make it
sticky?

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run("success");
Would be nice to test failure case too.

[victorhsieh0, 2016-03-07 23:54:43]

PTAL

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

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_context_menu_unittest.cc:409: ASSERT_EQ(2,
menu->GetItemCount());
On 2016/03/07 21:11:52, khmel wrote:
> I think pinnable item is missing. I would prefer to keep this test close to
real
> life, were arc app is pinnable. Please add
controller()->SetAppPinnable(app_id,
> AppListControllerDelegate::PIN_EDITABLE);

Done.

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

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.h:41: static void
OnUninstallAppResponse(const mojo::String& error_message);
On 2016/03/07 21:11:52, khmel wrote:
> I don't think ArcAppContextMenu is good place to handle OnUninstallAppResponse

Moved to utils per discussion.

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/1756193008/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_test.cc:30: arc::AppInfo store_app;
On 2016/03/07 21:11:52, khmel wrote:
> nit: Can we do it more gracefully? For example  get first app and make it
> sticky?

Done.  We don't need that package_name.

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run("success");
On 2016/03/07 21:11:52, khmel wrote:
> Would be nice to test failure case too.

I only added this to make it compile.  arc_app_utils.cc is the only caller.  Now
I looked at it, functions there all use arc::ArcBridgeService singleton and thus
is hard to test without more changes.  I can refactor a bit if you want, but
maybe in a separate CL?

[khmel, 2016-03-08 00:09:54]

lgtm with nits

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/20001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run("success");
On 2016/03/07 23:54:43, victorhsieh0 wrote:
> On 2016/03/07 21:11:52, khmel wrote:
> > Would be nice to test failure case too.
> 
> I only added this to make it compile.  arc_app_utils.cc is the only caller. 
Now
> I looked at it, functions there all use arc::ArcBridgeService singleton and
thus
> is hard to test without more changes.  I can refactor a bit if you want, but
> maybe in a separate CL?

Sure,

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_context_menu_unittest.cc (right):

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_context_menu_unittest.cc:412:
EXPECT_EQ(app_list::AppContextMenu::LAUNCH_NEW, menu->GetCommandIdAt(0));
nit: I know you followed code style in this file, but EXPECT_TRUE/FALSE is
preferable for bool check.

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

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:61:
arc::UninstallApp(app_info->package_name,
nit: DCHECK(app_info.get()).

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:61: bool sticky;
nit: I would put sticky above ready. Ready is dynamic property and it is not
expected to be changed.

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

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:130: LOG(ERROR) << "Failed to
uninstall package: " << error_message.get();
nit: if we don't have notification from Android side, please add TODO -> Show
notification or similar case.

[victorhsieh0, 2016-03-08 00:33:08]

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_context_menu_unittest.cc (right):

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_context_menu_unittest.cc:412:
EXPECT_EQ(app_list::AppContextMenu::LAUNCH_NEW, menu->GetCommandIdAt(0));
On 2016/03/08 00:09:54, khmel wrote:
> nit: I know you followed code style in this file, but EXPECT_TRUE/FALSE is
> preferable for bool check.

Done.

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

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:61:
arc::UninstallApp(app_info->package_name,
On 2016/03/08 00:09:54, khmel wrote:
> nit: DCHECK(app_info.get()).

Done with error check.

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:61: bool sticky;
On 2016/03/08 00:09:54, khmel wrote:
> nit: I would put sticky above ready. Ready is dynamic property and it is not
> expected to be changed.

Done.

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

https://codereview.chromium.org/1756193008/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:130: LOG(ERROR) << "Failed to
uninstall package: " << error_message.get();
On 2016/03/08 00:09:54, khmel wrote:
> nit: if we don't have notification from Android side, please add TODO -> Show
> notification or similar case.

Done with TODO.  We should think about a generic way (can just be notification)
to deliver ARC error to the user.

[dcheng, 2016-03-08 00:47:23]

https://codereview.chromium.org/1756193008/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.cc (right):

https://codereview.chromium.org/1756193008/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:31:
menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
Is it guaranteed that there will always be default items?

https://codereview.chromium.org/1756193008/diff/60001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/60001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run("success");
Hmm... won't OnUninstallAppResponse treat this as a failure since it checks if
the string is non-null?

[victorhsieh0, 2016-03-08 01:05:29]

PTAL

https://codereview.chromium.org/1756193008/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_context_menu.cc (right):

https://codereview.chromium.org/1756193008/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_context_menu.cc:31:
menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
On 2016/03/08 00:47:23, dcheng wrote:
> Is it guaranteed that there will always be default items?

If you mean is there always items between this separator and the previous, no. 
BuildMenu can render some or none, and it can even render a separator first,
depends on the setting.

But luckily(?), AddSeparator dedup separator itself.  So the code here
essentially says we really want a line here.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/models/sim...

https://codereview.chromium.org/1756193008/diff/60001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/60001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run("success");
On 2016/03/08 00:47:23, dcheng wrote:
> Hmm... won't OnUninstallAppResponse treat this as a failure since it checks if
> the string is non-null?

Not sure why my change is gone :(  This isn't called today, but for correctness
it's calling with nullptr now.

[dcheng, 2016-03-08 02:18:46]

lgtm

https://codereview.chromium.org/1756193008/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run(nullptr);
It feels a little funky that you can construct mojo::String from nullptr. Meh.

[xiyuan, 2016-03-08 16:40:11]

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:238: bool sticky;
Set a default value, in case kSticky does not exist and app->GetBoolean() fails.

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:26: namespace arc {
nit: Can we combine this with the one started at line 21?

[victorhsieh0, 2016-03-08 18:02:52]

PTAL

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:238: bool sticky;
On 2016/03/08 16:40:11, xiyuan wrote:
> Set a default value, in case kSticky does not exist and app->GetBoolean()
fails.

Done.

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_utils.h (right):

https://codereview.chromium.org/1756193008/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_utils.h:26: namespace arc {
On 2016/03/08 16:40:11, xiyuan wrote:
> nit: Can we combine this with the one started at line 21?

Done.

https://codereview.chromium.org/1756193008/diff/80001/components/arc/test/fak...
File components/arc/test/fake_app_instance.cc (right):

https://codereview.chromium.org/1756193008/diff/80001/components/arc/test/fak...
components/arc/test/fake_app_instance.cc:138: callback.Run(nullptr);
On 2016/03/08 02:18:46, dcheng wrote:
> It feels a little funky that you can construct mojo::String from nullptr. Meh.

Yeah, mojo::String can be passed through IPC, so it kind of makes sense to
accept nullptr.  I now made an explicit conversion.

[xiyuan, 2016-03-08 18:03:49]

lgtm

[Luis Héctor Chávez, 2016-03-14 16:21:10]

https://codereview.chromium.org/1756193008/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/1756193008/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:102: bool UninstallApp(const
std::string& package_name,
Once you implement OnUninstallAppResponse correctly, all synchronous failures
(the bridge being unavilable, or not having the correct version) won't be
handled by the same path.

So, how about instead of making UninstallApp() bool, you make it void and
instead of logging errors here, you relay them to OnUninstallAppResponse? You'll
need to i18n the messages, but you can mark that as TODO. Alternatively if there
is already a pre-i18n'd message, you can relay that one and keep the verbose
logging here.

[victorhsieh0, 2016-03-15 17:26:33]

https://codereview.chromium.org/1756193008/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right):

https://codereview.chromium.org/1756193008/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_utils.cc:102: bool UninstallApp(const
std::string& package_name,
Per discussion, we won't report the error (which should really be internal error
since we should not add uninstall to menu at the first place) like chrome does
in many cases.

[victorhsieh0, 2016-03-15 17:28:19]

PTAL

https://codereview.chromium.org/1756193008/diff/120001/components/arc/common/...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/1756193008/diff/120001/components/arc/common/...
components/arc/common/app.mojom:74: [MinVersion=1] ScreenRect?
dimension_on_screen);
Sync this to Android's version to follow the comment's naming.

https://codereview.chromium.org/1756193008/diff/120001/components/arc/common/...
components/arc/common/app.mojom:95: [MinVersion=2] UninstallPackage(string
package_name);
Sync this to Android's version to avoid using |package|.

[Luis Héctor Chávez, 2016-03-15 17:28:28]

lgtm

[victorhsieh0, 2016-03-15 17:29:22]

The CQ bit was checked by victorhsieh@google.com

[victorhsieh0, 2016-03-15 17:29:23]

The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org,
dcheng@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1756193008/#ps120001
(title: "rename")

[commit-bot: I haz the power, 2016-03-15 17:29:39]

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

[commit-bot: I haz the power, 2016-03-15 17:31:57]

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

[commit-bot: I haz the power, 2016-03-15 17:31:58]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[victorhsieh0, 2016-03-15 19:10:49]

The CQ bit was checked by victorhsieh@google.com

[victorhsieh0, 2016-03-15 19:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
lhchavez@chromium.org, khmel@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1756193008/#ps160001
(title: "fix")

[commit-bot: I haz the power, 2016-03-15 19:11:12]

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

[commit-bot: I haz the power, 2016-03-15 19:16:48]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-15 19:18:06]

Description was changed from

==========
Support uninstalling ARC app from Chrome launcher

This changes renders an entry to allow uninstalling an app from Chrome
launcher.  Apps that are installed by the user should be uninstallable,
while pre-installed apps are not.  Currently, the menu entry is not
rendered for pre-installed apps.  In the future, we should also respect
device policy.

BUG=b/24569986
TEST=Installed an app and uninstalled from Chrome launcher.
TEST=Didn't see "uninstall" entry in the menu for pre-installed apps
TEST=Forcely calls UninstallApp mojo against pre-installed packages
     failed naturally.  However, if the package is overridden by later
     version, uninstall will success (presumably rollback to factory
     version).
==========

to

==========
Support uninstalling ARC app from Chrome launcher

This changes renders an entry to allow uninstalling an app from Chrome
launcher.  Apps that are installed by the user should be uninstallable,
while pre-installed apps are not.  Currently, the menu entry is not
rendered for pre-installed apps.  In the future, we should also respect
device policy.

BUG=b/24569986
TEST=Installed an app and uninstalled from Chrome launcher.
TEST=Didn't see "uninstall" entry in the menu for pre-installed apps
TEST=Forcely calls UninstallApp mojo against pre-installed packages
     failed naturally.  However, if the package is overridden by later
     version, uninstall will success (presumably rollback to factory
     version).

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

[commit-bot: I haz the power, 2016-03-15 19:18:08]

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

[victorhsieh0, 2016-04-08 18:33:38]

Patchset #10 (id:180001) has been deleted