Fix import legacy, pref based pins.

Fix import legacy, pref based pins.

This CL contains 2 fixes for importing legacy pref based pins.
1. Import may happen at the moment when profile is not synced yet and
in last case it does not actually contain information from previous
bulds.
2. Importing ignores apps that currently are not installed. This
leads to situation that we loose pin information for such apps
and in case apps are installed later they do not appear pinned.
Solution is to import all existings apps in prefs.

BUG=680821
TEST=Clearn app sync server data. Login on M52 and do some pinning.
     Next login on M55 and pin additional app. Next login to ToT
     and observe that pin is restored for existing apps and
     pins appear automatically once apps are installed later.

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

[khmel, 2017-01-20 21:31:34]

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

[khmel, 2017-01-20 21:31:35]

Hi Steven,

PTAL

Thanks!

[stevenjb, 2017-01-21 00:14:31]

The extra dependency between launcher code and sync code is a bit unfortunate,
but it seems unavoidable.

It would be nice if there were a reasonable automated way to test this, but I
don't see anything alarming in these changes, and it's a pretty unfortunate bug.

lgtm

https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:245: return;
{}

[khmel, 2017-01-21 01:24:29]

On 2017/01/21 00:14:31, stevenjb wrote:
> The extra dependency between launcher code and sync code is a bit unfortunate,
> but it seems unavoidable.
> 
> It would be nice if there were a reasonable automated way to test this, but I
> don't see anything alarming in these changes, and it's a pretty unfortunate
bug.
> 
> lgtm
> 
>
https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
> File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):
> 
>
https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
> chrome/browser/ui/ash/chrome_launcher_prefs.cc:245: return;
> {}

Thank you Steven for review. Anyway ImportLegacy test is failing now (which is
truly expected). I will try to update and extend it now.

[khmel, 2017-01-21 09:02:41]

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

[commit-bot: I haz the power, 2017-01-21 09:02:54]

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-21 09:34:36]

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

[commit-bot: I haz the power, 2017-01-21 09:34:37]

Dry run: This issue passed the CQ dry run.

[khmel, 2017-01-23 16:54:47]

Hi Steven,

Could you please take a look to updated ImportLegacyPins test. Now it is closer
to real cases.

PTAL

Thanks

https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right):

https://codereview.chromium.org/2646973003/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_launcher_prefs.cc:245: return;
On 2017/01/21 00:14:31, stevenjb wrote:
> {}

Done.

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

https://codereview.chromium.org/2646973003/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:359:
extension_chrome_ = Extension::Create(base::FilePath(), Manifest::UNPACKED,
It is expected that Chrome app always exists

[stevenjb, 2017-01-23 17:01:20]

Thanks for the extra tests. still lgtm

[khmel, 2017-01-23 17:01:45]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-01-23 17:02:05]

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-23 17:33:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485190905685330,
"parent_rev": "499cafbc5f0f53763c0e7a99ba097ae1a5098eff", "commit_rev":
"d8cc9d23b26dc31e050fdc1e2042f639d8a608fa"}

[commit-bot: I haz the power, 2017-01-23 17:34:54]

Description was changed from

==========
Fix import legacy, pref based pins.

This CL contains 2 fixes for importing legacy pref based pins.
1. Import may happen at the moment when profile is not synced yet and
in last case it does not actually contain information from previous
bulds.
2. Importing ignores apps that currently are not installed. This
leads to situation that we loose pin information for such apps
and in case apps are installed later they do not appear pinned.
Solution is to import all existings apps in prefs.

BUG=680821
TEST=Clearn app sync server data. Login on M52 and do some pinning.
     Next login on M55 and pin additional app. Next login to ToT
     and observe that pin is restored for existing apps and
     pins appear automatically once apps are installed later.
==========

to

==========
Fix import legacy, pref based pins.

This CL contains 2 fixes for importing legacy pref based pins.
1. Import may happen at the moment when profile is not synced yet and
in last case it does not actually contain information from previous
bulds.
2. Importing ignores apps that currently are not installed. This
leads to situation that we loose pin information for such apps
and in case apps are installed later they do not appear pinned.
Solution is to import all existings apps in prefs.

BUG=680821
TEST=Clearn app sync server data. Login on M52 and do some pinning.
     Next login on M55 and pin additional app. Next login to ToT
     and observe that pin is restored for existing apps and
     pins appear automatically once apps are installed later.

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

[commit-bot: I haz the power, 2017-01-23 17:34:55]

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