Discard pinning an app with non-empty launcher id.

Discard pinning an app with non-empty launcher id.

Currently we don't support persisting Chrome shelf pin for app with
custom launcher id. We should discard the code that stores pins for
such apps in app list sync service which is not compatible with its
design.

TEST=tests passed and manually on device various pin scenarios
BUG=689772
Review-Url: https://codereview.chromium.org/2684853002
Cr-Commit-Position: refs/heads/master@{#449126}
Committed: https://chromium.googlesource.com/chromium/src/+/613cdf8eeeaac4830b55c4b55b4377269faa2e35

[khmel, 2017-02-08 02:01:00]

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

[commit-bot: I haz the power, 2017-02-08 02:01:57]

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-02-08 02:30:57]

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

[commit-bot: I haz the power, 2017-02-08 02:30:57]

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_...)

[khmel, 2017-02-08 02:43:39]

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

[commit-bot: I haz the power, 2017-02-08 02:44:14]

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-02-08 03:42:55]

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

[commit-bot: I haz the power, 2017-02-08 03:42:56]

Dry run: This issue passed the CQ dry run.

[khmel, 2017-02-08 16:43:42]

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

[khmel, 2017-02-08 16:43:43]

Hi Steven,

As we discussed this discard saving pins with launcher id.

PTAL

https://codereview.chromium.org/2684853002/diff/1/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2684853002/diff/1/chrome/browser/ui/BUILD.gn#...
chrome/browser/ui/BUILD.gn:1287: "ash/app_launcher_id.cc",
Move it to separate class to make logic and dependencies clearer.

https://codereview.chromium.org/2684853002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.h (left):

https://codereview.chromium.org/2684853002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.h:91:
std::unique_ptr<base::DictionaryValue> CreateAppDict(
This is actually helper that can be moved to namespace {}

[stevenjb, 2017-02-08 18:07:28]

cc:rafael.antognolli@intel.com

[antognolli, 2017-02-08 18:17:27]

On 2017/02/08 18:07:28, stevenjb wrote:
> cc:rafael.antognolli@intel.com

Change looks good to me.

[stevenjb, 2017-02-08 18:19:03]

Thanks for separating out app_launcher_id.cc/h.

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

https://codereview.chromium.org/2684853002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1083: }
Long term I think we are going to need to sync launch_id one way or another. It
might be simpler to leave the code below as it was and move this test to
ash::launcher::SetPinPosition instead. (We would want to have RemovePinPosition
continue to take ApplauncherId as well). WDYT?

[khmel, 2017-02-08 20:08:43]

PTAL

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

https://codereview.chromium.org/2684853002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1083: }
On 2017/02/08 18:19:03, stevenjb wrote:
> Long term I think we are going to need to sync launch_id one way or another.
It
> might be simpler to leave the code below as it was and move this test to
> ash::launcher::SetPinPosition instead. (We would want to have
RemovePinPosition
> continue to take ApplauncherId as well). WDYT?
> 

Agree, we can leave this code for now.

[stevenjb, 2017-02-08 20:32:37]

lgtm w/ nits

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:700: VLOG(2) << "Unpinning app '"
<< app_id << "' with non-empty launch id '"
nit: Unpinning works, just syncing does not, maybe "Syncing remove pin for "

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:719: VLOG(2) << "Pinning app '"
<< app_id << "' with non-empty launch id '"
ditto

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:739: for (const auto&
app_launher_id_after : app_launcher_ids_after) {
app_launcher_id_after

[khmel, 2017-02-08 21:20:40]

Thank you for review!

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:700: VLOG(2) << "Unpinning app '"
<< app_id << "' with non-empty launch id '"
On 2017/02/08 20:32:37, stevenjb wrote:
> nit: Unpinning works, just syncing does not, maybe "Syncing remove pin for "

That is better, thanks!

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:719: VLOG(2) << "Pinning app '"
<< app_id << "' with non-empty launch id '"
On 2017/02/08 20:32:37, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/2684853002/diff/40001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:739: for (const auto&
app_launher_id_after : app_launcher_ids_after) {
On 2017/02/08 20:32:37, stevenjb wrote:
> app_launcher_id_after

Done.

[khmel, 2017-02-08 21:20:43]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-02-08 21:20:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2684853002/#ps60001
(title: "few nits")

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

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-02-08 23:08:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486588843722790,
"parent_rev": "a5c3798ee2de14fb39c3bde92015c4ee7081c2b2", "commit_rev":
"613cdf8eeeaac4830b55c4b55b4377269faa2e35"}

[commit-bot: I haz the power, 2017-02-08 23:08:39]

Description was changed from

==========
Discard pinning an app with non-empty launcher id.

Currently we don't support persisting Chrome shelf pin for app with
custom launcher id. We should discard the code that stores pins for
such apps in app list sync service which is not compatible with its
design.

TEST=tests passed and manually on device various pin scenarios
BUG=689772
==========

to

==========
Discard pinning an app with non-empty launcher id.

Currently we don't support persisting Chrome shelf pin for app with
custom launcher id. We should discard the code that stores pins for
such apps in app list sync service which is not compatible with its
design.

TEST=tests passed and manually on device various pin scenarios
BUG=689772

Review-Url: https://codereview.chromium.org/2684853002
Cr-Commit-Position: refs/heads/master@{#449126}
Committed:
https://chromium.googlesource.com/chromium/src/+/613cdf8eeeaac4830b55c4b55b43...
==========

[commit-bot: I haz the power, 2017-02-08 23:08:50]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/613cdf8eeeaac4830b55c4b55b43...

[James Cook, 2017-02-09 01:26:52]

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

[James Cook, 2017-02-09 01:26:53]

+msw FYI

https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_launcher_id.h (right):

https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_launcher_id.h:11: namespace launcher {
Is there any particular reason you put this in "namespace launcher"?  That
namespace is only used in ChromeLauncherPrefs and probably shouldn't exist at
all. Taking this out of namespace launcher would make some identifiers shorter.

[khmel, 2017-02-09 01:30:22]

https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
File chrome/browser/ui/ash/app_launcher_id.h (right):

https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
chrome/browser/ui/ash/app_launcher_id.h:11: namespace launcher {
On 2017/02/09 01:26:53, James Cook wrote:
> Is there any particular reason you put this in "namespace launcher"?  That
> namespace is only used in ChromeLauncherPrefs and probably shouldn't exist at
> all. Taking this out of namespace launcher would make some identifiers
shorter.

There is no particular reason for this (kept from previous impl). (IIUC this
change is due rebase)

[James Cook, 2017-02-09 01:38:28]

On 2017/02/09 01:30:22, khmel wrote:
>
https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
> File chrome/browser/ui/ash/app_launcher_id.h (right):
> 
>
https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
> chrome/browser/ui/ash/app_launcher_id.h:11: namespace launcher {
> On 2017/02/09 01:26:53, James Cook wrote:
> > Is there any particular reason you put this in "namespace launcher"?  That
> > namespace is only used in ChromeLauncherPrefs and probably shouldn't exist
at
> > all. Taking this out of namespace launcher would make some identifiers
> shorter.
> 
> There is no particular reason for this (kept from previous impl). (IIUC this
> change is due rebase)

Maybe either you or msw@ could take it out of the namespace?

He's got an active CL that touches a bunch of call sites that use it,
https://codereview.chromium.org/2684723003/

[khmel, 2017-02-09 01:52:22]

On 2017/02/09 01:38:28, James Cook wrote:
> On 2017/02/09 01:30:22, khmel wrote:
> >
>
https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
> > File chrome/browser/ui/ash/app_launcher_id.h (right):
> > 
> >
>
https://codereview.chromium.org/2684853002/diff/60001/chrome/browser/ui/ash/a...
> > chrome/browser/ui/ash/app_launcher_id.h:11: namespace launcher {
> > On 2017/02/09 01:26:53, James Cook wrote:
> > > Is there any particular reason you put this in "namespace launcher"?  That
> > > namespace is only used in ChromeLauncherPrefs and probably shouldn't exist
> at
> > > all. Taking this out of namespace launcher would make some identifiers
> > shorter.
> > 
> > There is no particular reason for this (kept from previous impl). (IIUC this
> > change is due rebase)
> 
> Maybe either you or msw@ could take it out of the namespace?
> 
> He's got an active CL that touches a bunch of call sites that use it,
> https://codereview.chromium.org/2684723003/

I will take care