arc: Initial implemetation of Chrome sync for Arc packages.

arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/

Committed: https://crrev.com/68bc3e7017f97a20f7deaf3ae5601f2f68c62e16
Cr-Commit-Position: refs/heads/master@{#409437}

[lgcheng, 2016-06-29 17:12:28]

Patchset #2 (id:20001) has been deleted

[lgcheng, 2016-06-29 17:30:57]

Patchset #2 (id:40001) has been deleted

[lgcheng, 2016-06-29 17:38:42]

Patchset #2 (id:60001) has been deleted

[lgcheng, 2016-06-29 17:45:05]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos introduced in 2092893002.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

[lgcheng, 2016-06-29 17:45:54]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos introduced in 2092893002.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in: https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

[lgcheng, 2016-06-29 17:46:23]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in: https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

[lgcheng, 2016-06-29 17:46:59]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
TEST=Sync integration test WIP.
==========

[lgcheng, 2016-06-29 18:18:59]

Patchset #2 (id:80001) has been deleted

[lgcheng, 2016-06-29 21:38:44]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test waiting for server side update.
TEST=Sync integration test WIP.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test with simulation Android package manager.
Waiting for Chrome sync server side update to test.
TEST=Sync integration test WIP.
==========

[lgcheng, 2016-06-29 21:44:55]

Patchset #3 (id:120001) has been deleted

[lgcheng, 2016-06-29 21:54:26]

Patchset #2 (id:100001) has been deleted

[lgcheng, 2016-06-29 22:09:49]

lgcheng@google.com changed reviewers:
+ elijahtaylor@chromium.org, stevenjb@chromium.org

[lgcheng, 2016-06-29 22:09:49]

Hi Steven,

PTAL at this patch.

I am implementing Chrome sync for Arc packages. The chrome sync server side is
still not updated for the new sync data type yet, I can't observe desired result
and I am keeping all the test log so far. 

If you have a moment, can you take a look and give some, maybe high level
advises?

cc Elijah.

Thanks!

[stevenjb, 2016-06-29 22:18:31]

It might be a day or two before I can look at this since I'm gardener this
week, but I will do what I can.


On Wed, Jun 29, 2016 at 3:09 PM, <lgcheng@google.com> wrote:

> Reviewers: stevenjb, elijahtaylor1
> CL: https://codereview.chromium.org/2106663004/
>
> Message:
> Hi Steven,
>
> PTAL at this patch.
>
> I am implementing Chrome sync for Arc packages. The chrome sync server
> side is
> still not updated for the new sync data type yet, I can't observe desired
> result
> and I am keeping all the test log so far.
>
> If you have a moment, can you take a look and give some, maybe high level
> advises?
>
> cc Elijah.
>
> Thanks!
>
> Description:
> arc: Initial implemetation of Chrome sync for Arc packages.
> Sync protos is introduced in:
> https://codereview.chromium.org/2092893002/.
>
> BUG=28024050
> TEST=Manual Test with simulation Android package manager.
> Waiting for Chrome sync server side update to test.
> TEST=Sync integration test WIP.
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+728, -2 lines):
> M chrome/browser/sync/chrome_sync_client.cc
> M chrome/browser/ui/app_list/arc/arc_app_list_prefs.h
> M chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
> A chrome/browser/ui/app_list/arc/arc_package_syncable_service.h
> A chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc
> A chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.h
> A chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.cc
> M chrome/chrome_browser_ui.gypi
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-06-30 00:23:43]

Looking now. You will also want to add zea@ or another c/b/sync owner.

[lgcheng, 2016-06-30 01:12:59]

lgcheng@google.com changed reviewers:
+ pavely@chromium.org

[lgcheng, 2016-06-30 01:13:43]

On 2016/06/30 00:23:43, stevenjb wrote:
> Looking now. You will also want to add zea@ or another c/b/sync owner.

Thanks for the update!

Adding pavely for chrome/browser/sync/chrome_sync_client.cc updates.

Thanks!

[stevenjb, 2016-06-30 01:27:44]

We definitely need tests with this also.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:222: : prefs_(prefs),
sync_service_(NULL), binding_(this),
nullptr

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:27: constexpr
char kUninstallPackage[] = "uninstall package";
These only appear to be used for logging? If so the logging should be moved (see
comment below) and these eliminated, otherwise they should be commented.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:76: // Gets
arc::AppInstance. Creates log if there is error.
nit: Comment not really helpful.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:78: const
std::string& service_name, const std::string& package_name) {
Use clang format ('git cl format')

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: VLOG(2) <<
"Request to " << service_name
service_name is only used in the log, which is confusing, I would remove the arg
and move the logging to the caller(s).

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:162:
DCHECK(!sync_processor_.get());
nit: move this just above where it is assigned, it's a little confusing here.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:186: if
(!local_package_set.count(package_name)) {
nit: !ContainsKey(local_package_set, package_name) is a little more readable.
(In stl_util.h).

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:195: // Sends
unsynced items.
nit: This comment is confusing, the next line is just a declaration, maybe
combine this with the comment at line 198 and make it more descriptive?

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:200: if
(sync_items_.count(local_package_name))
ContainsKey

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:236: iter !=
sync_items_.end(); ++iter) {
auto?

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:241: iter !=
pending_install_items_.end(); ++iter) {
auto?

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:259: const
SyncChange& change = *iter;
auto?

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:413: if
(pending_iter != sync_items_.end()) {
fix indent (clang format)

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.h (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:81: // Creates or
Update syncItem from sync data change. Send request to
updates, sends

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:82: //
install/update to android side.
nit: this sends data to the sync server, which presumably is also being consumed
by Android, but that's a detail that isn't specifically relevant to this code. I
would s/android/server/ (or 'sync server') throughout.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:87: // side.
Sends
+ merge lines

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:91: // to
different situations.
I don't understand the second sentence.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:104: // returns
constant results from different devices.
s/constant/consistent/ ?

[lgcheng, 2016-06-30 02:20:40]

Thanks for comments!
Yes we definitely need pass manual test and add add integration test before we
can move further. But it is still super helpful to get some issue addressed at
this point.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:222: : prefs_(prefs),
sync_service_(NULL), binding_(this),
On 2016/06/30 01:27:43, stevenjb wrote:
> nullptr

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:27: constexpr
char kUninstallPackage[] = "uninstall package";
On 2016/06/30 01:27:43, stevenjb wrote:
> These only appear to be used for logging? If so the logging should be moved
(see
> comment below) and these eliminated, otherwise they should be commented.

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:76: // Gets
arc::AppInstance. Creates log if there is error.
On 2016/06/30 01:27:43, stevenjb wrote:
> nit: Comment not really helpful.
Removed.
Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:78: const
std::string& service_name, const std::string& package_name) {
On 2016/06/30 01:27:43, stevenjb wrote:
> Use clang format ('git cl format')

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: VLOG(2) <<
"Request to " << service_name
On 2016/06/30 01:27:43, stevenjb wrote:
> service_name is only used in the log, which is confusing, I would remove the
arg
> and move the logging to the caller(s).

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:162:
DCHECK(!sync_processor_.get());
On 2016/06/30 01:27:43, stevenjb wrote:
> nit: move this just above where it is assigned, it's a little confusing here.

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:186: if
(!local_package_set.count(package_name)) {
On 2016/06/30 01:27:43, stevenjb wrote:
> nit: !ContainsKey(local_package_set, package_name) is a little more readable.
> (In stl_util.h).

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:195: // Sends
unsynced items.
On 2016/06/30 01:27:43, stevenjb wrote:
> nit: This comment is confusing, the next line is just a declaration, maybe
> combine this with the comment at line 198 and make it more descriptive?

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:200: if
(sync_items_.count(local_package_name))
On 2016/06/30 01:27:43, stevenjb wrote:
> ContainsKey

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:236: iter !=
sync_items_.end(); ++iter) {
On 2016/06/30 01:27:43, stevenjb wrote:
> auto?

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:241: iter !=
pending_install_items_.end(); ++iter) {
On 2016/06/30 01:27:43, stevenjb wrote:
> auto?

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:259: const
SyncChange& change = *iter;
On 2016/06/30 01:27:43, stevenjb wrote:
> auto?

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:413: if
(pending_iter != sync_items_.end()) {
On 2016/06/30 01:27:43, stevenjb wrote:
> fix indent (clang format)

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.h (right):

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:81: // Creates or
Update syncItem from sync data change. Send request to
On 2016/06/30 01:27:43, stevenjb wrote:
> updates, sends

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:82: //
install/update to android side.
On 2016/06/30 01:27:43, stevenjb wrote:
> nit: this sends data to the sync server, which presumably is also being
consumed
> by Android, but that's a detail that isn't specifically relevant to this code.
I
> would s/android/server/ (or 'sync server') throughout.

I create new description. WDYT?

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:87: // side.
On 2016/06/30 01:27:44, stevenjb wrote:
> Sends
> + merge lines

Done.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:91: // to
different situations.
On 2016/06/30 01:27:44, stevenjb wrote:
> I don't understand the second sentence.

Currently, every user installed package from play store are marked as should
sync. Later we will add support for admin installed packages, which will be a
little bit more complicated. So I leave a interface here as start point of later
integration. I am changing the wording for better explain.

https://codereview.chromium.org/2106663004/diff/160001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:104: // returns
constant results from different devices.
On 2016/06/30 01:27:44, stevenjb wrote:
> s/constant/consistent/ ?
Should be consistent.
Done.

[pavely, 2016-06-30 21:03:22]

chrome/browser/sync lgtm

[stevenjb, 2016-07-07 23:10:38]

OK, this code looks fine, but we definitely need to add tests
(single_client_arc_package_sync_test and two_client_arc_package_sync_test)
before landing this.

[lgcheng, 2016-07-07 23:15:50]

On 2016/07/07 23:10:38, stevenjb wrote:
> OK, this code looks fine, but we definitely need to add tests
> (single_client_arc_package_sync_test and two_client_arc_package_sync_test)
> before landing this.

Thanks for the comment!

Sure. I am currently working on adding the two integration test.

Since several Arc services are global singleton(unlike services which are
profile singleton), I am also working on some refactor to break the singleton
only for test so that I can use those service in the sync test.

Should be coming soon.

[Luis Héctor Chávez, 2016-07-20 00:01:13]

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

[Luis Héctor Chávez, 2016-07-20 00:01:15]

first round

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:246: arc::ArcBridgeService*
bridge_service = arc::ArcBridgeService::Get();
Unneeded.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:250: if
(!bridge_service->ready())
if (!app_instance_holder_->instance())

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:135: app_instance_.reset(new
arc::FakeAppInstance(arc_app_list_pref_));
Can you add a bridge_service_->app()->CloseChannel() before?

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.h (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.h:47: // CloseChannl(). Reset
AppInstance to continue unit_tests.
nit: CloseChannel()

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.h (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:46: };
Given that this will always be wrapped in an unique_ptr,
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:69: using
SyncItemMap = std::map<std::string, std::unique_ptr<SyncItem>>;
std::unordered_map?

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:102: Profile*
profile_;
Profile* const profile_;

?

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:127:
ArcAppListPrefs* prefs_;
ArcAppListPrefs* const prefs_;

?

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:129:
mojom::AppInstance* app_instance_;
You shouldn't keep a reference to this. If necessary, pass the InstanceHolder
instead (and make it a const-pointer).

https://codereview.chromium.org/2106663004/diff/220001/components/arc/arc_bri...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/2106663004/diff/220001/components/arc/arc_bri...
components/arc/arc_bridge_service.cc:66: app_.CloseChannel();
Hmmmm I don't like this. Is there any way you can avoid this?

https://codereview.chromium.org/2106663004/diff/220001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2106663004/diff/220001/components/arc/instanc...
components/arc/instance_holder.h:65: if (!ptr_ && !raw_ptr_)
Why is this needed?

[lgcheng, 2016-07-20 00:17:03]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Manual Test with simulation Android package manager.
Waiting for Chrome sync server side update to test.
TEST=Sync integration test WIP.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Dependent sync integration test CL WIP.
==========

[lgcheng, 2016-07-20 01:20:25]

Patchset #7 (id:240001) has been deleted

[lgcheng, 2016-07-20 01:23:59]

Thank Luis for review. I was not even expecting a detailed review at this time.

I have questions and will ask you offline.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:246: arc::ArcBridgeService*
bridge_service = arc::ArcBridgeService::Get();
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> Unneeded.

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:250: if
(!bridge_service->ready())
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> if (!app_instance_holder_->instance())

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:135: app_instance_.reset(new
arc::FakeAppInstance(arc_app_list_pref_));
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> Can you add a bridge_service_->app()->CloseChannel() before?

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.h (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.h:47: // CloseChannl(). Reset
AppInstance to continue unit_tests.
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> nit: CloseChannel()

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.h (right):

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:46: };
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> Given that this will always be wrapped in an unique_ptr,
> DISALLOW_COPY_AND_ASSIGN?

I have seen reviewers require to remove DISALLOW_COPY_AND_ASSIGN for local
structs...

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:69: using
SyncItemMap = std::map<std::string, std::unique_ptr<SyncItem>>;
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> std::unordered_map?

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:102: Profile*
profile_;
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> Profile* const profile_;
> 
> ?

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:127:
ArcAppListPrefs* prefs_;
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> ArcAppListPrefs* const prefs_;
> 
> ?

Done.

https://codereview.chromium.org/2106663004/diff/220001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.h:129:
mojom::AppInstance* app_instance_;
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> You shouldn't keep a reference to this. If necessary, pass the InstanceHolder
> instead (and make it a const-pointer).

I think you are right. Let me ask some questions offline first.

https://codereview.chromium.org/2106663004/diff/220001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2106663004/diff/220001/components/arc/instanc...
components/arc/instance_holder.h:65: if (!ptr_ && !raw_ptr_)
On 2016/07/20 00:01:15, Luis Héctor Chávez wrote:
> Why is this needed?

In test, ptr_ is never set. only raw_ptr_ is set. Without this, CloseChannel()
will directly return while OnInstanceClosed() is never sent to its observers.

[lgcheng, 2016-07-22 20:28:32]

Hi Guys,

Integration test is created. PTAL.

[lgcheng, 2016-07-22 20:32:41]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Dependent sync integration test CL WIP.
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=https://codereview.chromium.org/2174753004/
==========

[lgcheng, 2016-07-22 20:41:17]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=https://codereview.chromium.org/2174753004/
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

[stevenjb, 2016-07-25 16:48:10]

Thanks for adding the tests in 2174753004. lgtm.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.cc
(right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.cc:44:
ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile);
nit: no need for local here

[Luis Héctor Chávez, 2016-07-25 17:31:17]

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:22: #include
"components/arc/arc_bridge_service.h"
You should be able to remove this import. (If you can't, there's still a
reference to ABS lurking somewhere. Let's remove that.)

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:229: profile_(profile),
Can you reorder the declaration of the members so |profile_| appears first? That
way you can do |prefs_(profile_->GetPrefs())| and make |prefs_| be a
PrefService* const.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:254: arc::ArcBridgeService*
bridge_service = arc::ArcBridgeService::Get();
Not needed anymore.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:584:
sync_service_->StopSyncing(syncer::ARC_PACKAGE);
nit:

if (sync_service_) {
  sync_service_...
  sync_service_ = nullptr;
}

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:135: bridge_service_->SetReady();
Discussed offline, please try to not touch the bridge even in tests (if
possible).

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:129: std::string
package_name;
nit: remove

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:139: package_name
= sync_item->package_name;
nit: const std::string& package_name?

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:151: for (auto
local_package_name : local_packages) {
can this be const auto&?

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:187: for (auto
iter = sync_items_.begin(); iter != sync_items_.end(); ++iter) {
nit: can this be for (const auto& item : sync_items_)? Same in the rest of the
iterables below.

https://codereview.chromium.org/2106663004/diff/300001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2106663004/diff/300001/components/arc/instanc...
components/arc/instance_holder.h:65: if (!ptr_ && !raw_ptr_)
I prefer if this would only be |!raw_ptr_|, since resetting an already-reset
|ptr_| is a no-op.

[lgcheng, 2016-07-25 20:45:17]

Issue fixed. PTAL

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:22: #include
"components/arc/arc_bridge_service.h"
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> You should be able to remove this import. (If you can't, there's still a
> reference to ABS lurking somewhere. Let's remove that.)

Keep it for browser_test destruction.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:229: profile_(profile),
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> Can you reorder the declaration of the members so |profile_| appears first?
That
> way you can do |prefs_(profile_->GetPrefs())| and make |prefs_| be a
> PrefService* const.

Done.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:254: arc::ArcBridgeService*
bridge_service = arc::ArcBridgeService::Get();
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> Not needed anymore.

Keep this reference for browser_test destruction.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:584:
sync_service_->StopSyncing(syncer::ARC_PACKAGE);
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> nit:
> 
> if (sync_service_) {
>   sync_service_...
>   sync_service_ = nullptr;
> }

Done

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:135: bridge_service_->SetReady();
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> Discussed offline, please try to not touch the bridge even in tests (if
> possible).

Touch to bridge_service removed.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:129: std::string
package_name;
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> nit: remove

Done.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:139: package_name
= sync_item->package_name;
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> nit: const std::string& package_name?

Done.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:151: for (auto
local_package_name : local_packages) {
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> can this be const auto&?

Done.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:187: for (auto
iter = sync_items_.begin(); iter != sync_items_.end(); ++iter) {
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> nit: can this be for (const auto& item : sync_items_)? Same in the rest of the
> iterables below.

Done.

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.cc
(right):

https://codereview.chromium.org/2106663004/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_syncable_service_factory.cc:44:
ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile);
On 2016/07/25 16:48:10, stevenjb wrote:
> nit: no need for local here

Done.

https://codereview.chromium.org/2106663004/diff/300001/components/arc/instanc...
File components/arc/instance_holder.h (right):

https://codereview.chromium.org/2106663004/diff/300001/components/arc/instanc...
components/arc/instance_holder.h:65: if (!ptr_ && !raw_ptr_)
On 2016/07/25 17:31:17, Luis Héctor Chávez wrote:
> I prefer if this would only be |!raw_ptr_|, since resetting an already-reset
> |ptr_| is a no-op.

Done.

[Luis Héctor Chávez, 2016-07-25 20:50:55]

lgtm with nits.

Thanks!

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:132: void
ArcAppTest::SetBridgeServiceStopped() {
nit: rename to StopArcInstance.

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:136: void
ArcAppTest::ResetBridgeServiceReady() {
nit: rename to RestartArcInstance

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.h (right):

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.h:48: // AppInstance is destoryed
after bridge stops and InstanceHolder::
nit: s/destoryed/destroyed/

[lgcheng, 2016-07-25 21:10:11]

Thank you guys for review this patch!

Will submit after sync integration test passed review.

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.cc (right):

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:132: void
ArcAppTest::SetBridgeServiceStopped() {
On 2016/07/25 20:50:54, Luis Héctor Chávez wrote:
> nit: rename to StopArcInstance.

Done.

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.cc:136: void
ArcAppTest::ResetBridgeServiceReady() {
On 2016/07/25 20:50:54, Luis Héctor Chávez wrote:
> nit: rename to RestartArcInstance

Done.

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_app_test.h (right):

https://codereview.chromium.org/2106663004/diff/320001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_app_test.h:48: // AppInstance is destoryed
after bridge stops and InstanceHolder::
On 2016/07/25 20:50:54, Luis Héctor Chávez wrote:
> nit: s/destoryed/destroyed/

Done.

[lgcheng, 2016-07-25 23:04:21]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=28024050
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

[lgcheng, 2016-08-03 00:20:18]

The CQ bit was checked by lgcheng@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 00:20:53]

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-08-03 00:20:55]

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

[commit-bot: I haz the power, 2016-08-03 00:20:56]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[lgcheng, 2016-08-03 00:36:46]

The CQ bit was checked by lgcheng@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 00:37:36]

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-08-03 01:44:09]

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

[commit-bot: I haz the power, 2016-08-03 01:44:10]

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

[lgcheng, 2016-08-03 01:45:41]

The CQ bit was checked by lgcheng@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 01:46: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-08-03 02:15:17]

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

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

Dry run: This issue passed the CQ dry run.

[lgcheng, 2016-08-03 02:29:13]

The CQ bit was checked by lgcheng@google.com

[lgcheng, 2016-08-03 02:29:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
stevenjb@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2106663004/#ps420001
(title: "Modify target")

[commit-bot: I haz the power, 2016-08-03 02:29:29]

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-08-03 02:33:04]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

[commit-bot: I haz the power, 2016-08-03 02:33:05]

Committed patchset #15 (id:420001)

[commit-bot: I haz the power, 2016-08-03 02:34:25]

Description was changed from

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/
==========

to

==========
arc: Initial implemetation of Chrome sync for Arc packages.
Sync protos is introduced in:
https://codereview.chromium.org/2092893002/.

BUG=631253
TEST=Dependent Patchset: https://codereview.chromium.org/2174753004/

Committed: https://crrev.com/68bc3e7017f97a20f7deaf3ae5601f2f68c62e16
Cr-Commit-Position: refs/heads/master@{#409437}
==========

[commit-bot: I haz the power, 2016-08-03 02:34:27]

Patchset 15 (id:??) landed as
https://crrev.com/68bc3e7017f97a20f7deaf3ae5601f2f68c62e16
Cr-Commit-Position: refs/heads/master@{#409437}