arc: Support UNINSTALL_SHORTCUT

arc: Support UNINSTALL_SHORTCUT

This adds handling UNINSTALL_SHORTCUT intent.

BUG=717661
BUG=b/34749139
TEST=Manually with test app. Shortcuts added and removed properly.
     Extended unit_tests

Review-Url: https://codereview.chromium.org/2858703002
Cr-Commit-Position: refs/heads/master@{#469063}
Committed: https://chromium.googlesource.com/chromium/src/+/cf0b40a80365c1c0de0d70e785ae91a26e547f74

[khmel, 2017-05-02 19:06:07]

khmel@chromium.org changed reviewers:
+ lhchavez@chromium.org, xiyuan@chromium.org

[khmel, 2017-05-02 19:06:08]

Hi Luis and Xiyuan,

PTAL

Thanks!

[xiyuan, 2017-05-02 19:29:31]

lgtm

[Luis Héctor Chávez, 2017-05-02 19:48:17]

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

https://codereview.chromium.org/2858703002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1097: std::string
test_package_name;
Maybe call these two |installed_package_name| and |installed_intent_uri|?

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

https://codereview.chromium.org/2858703002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:589: // Adding package is
requred for safe call SendPackageUninstalled.
nit: s/for safe call/to safely call/

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

https://codereview.chromium.org/2858703002/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:153: // defined by it's |intent_uri| and
|package_name|.
nit: s/it's/its/.

Also, is it possible to use the ShortcutInfo struct here too, for consistency?

[khmel, 2017-05-02 20:09:44]

Thanks for taking a look.

PTAL updated version.

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

https://codereview.chromium.org/2858703002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1097: std::string
test_package_name;
On 2017/05/02 19:48:16, Luis Héctor Chávez wrote:
> Maybe call these two |installed_package_name| and |installed_intent_uri|?

That is nicer.

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

https://codereview.chromium.org/2858703002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:589: // Adding package is
requred for safe call SendPackageUninstalled.
On 2017/05/02 19:48:16, Luis Héctor Chávez wrote:
> nit: s/for safe call/to safely call/

Done.

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

https://codereview.chromium.org/2858703002/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:153: // defined by it's |intent_uri| and
|package_name|.
On 2017/05/02 19:48:17, Luis Héctor Chávez wrote:
> nit: s/it's/its/.
> 
> Also, is it possible to use the ShortcutInfo struct here too, for consistency?

Renamed. I tried to use ShortcutInfo in my first impl. However it failed to
serialize ShortcutInfo because this structure contains non-null label  and
icon_resource_id. Of cause it possible for uninstall to pass "" for label and
icon_resource_id but this looks like a hack. Also it is possible to mark them as
nullable in mojo but after this we have to add additional check for
onInstallShortcut. 
Counting that and fact that half of fields is not required I decided to pass
package and intent directly.

[Luis Héctor Chávez, 2017-05-02 20:23:49]

lgtm

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

https://codereview.chromium.org/2858703002/diff/1/components/arc/common/app.m...
components/arc/common/app.mojom:153: // defined by it's |intent_uri| and
|package_name|.
On 2017/05/02 20:09:43, khmel wrote:
> On 2017/05/02 19:48:17, Luis Héctor Chávez wrote:
> > nit: s/it's/its/.
> > 
> > Also, is it possible to use the ShortcutInfo struct here too, for
consistency?
> 
> Renamed. I tried to use ShortcutInfo in my first impl. However it failed to
> serialize ShortcutInfo because this structure contains non-null label  and
> icon_resource_id. Of cause it possible for uninstall to pass "" for label and
> icon_resource_id but this looks like a hack. Also it is possible to mark them
as
> nullable in mojo but after this we have to add additional check for
> onInstallShortcut. 
> Counting that and fact that half of fields is not required I decided to pass
> package and intent directly.

Ah that is very unfortunate :/ All right, seems reasonable.

[khmel, 2017-05-02 20:31:16]

khmel@chromium.org changed reviewers:
+ nasko@chromium.org

[khmel, 2017-05-02 20:31:16]

Thanks Luis,

Adding nasko@ for components/arc/common/app.mojom

PTAL

[nasko, 2017-05-03 18:34:45]

components/arc/common/app.mojom LGTM

https://codereview.chromium.org/2858703002/diff/20001/components/arc/common/a...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/2858703002/diff/20001/components/arc/common/a...
components/arc/common/app.mojom:153: // defined by it's |intent_uri| and
|package_name|.
nit: s/it's/its/

[khmel, 2017-05-03 18:37:58]

Thank you for review!

[khmel, 2017-05-03 18:38:04]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-05-03 18:38:04]

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

[commit-bot: I haz the power, 2017-05-03 18:39:09]

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-05-03 19:10:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493836684005590,
"parent_rev": "9f89092a8f533da1537b0a441b1e137b3d3b2728", "commit_rev":
"cf0b40a80365c1c0de0d70e785ae91a26e547f74"}

[commit-bot: I haz the power, 2017-05-03 19:12:00]

Description was changed from

==========
arc: Support UNINSTALL_SHORTCUT

This adds handling UNINSTALL_SHORTCUT intent.

BUG=717661
BUG=b/34749139
TEST=Manually with test app. Shortcuts added and removed properly.
     Extended unit_tests
==========

to

==========
arc: Support UNINSTALL_SHORTCUT

This adds handling UNINSTALL_SHORTCUT intent.

BUG=717661
BUG=b/34749139
TEST=Manually with test app. Shortcuts added and removed properly.
     Extended unit_tests

Review-Url: https://codereview.chromium.org/2858703002
Cr-Commit-Position: refs/heads/master@{#469063}
Committed:
https://chromium.googlesource.com/chromium/src/+/cf0b40a80365c1c0de0d70e785ae...
==========

[commit-bot: I haz the power, 2017-05-03 19:12:07]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cf0b40a80365c1c0de0d70e785ae...