Add DCHECK to make sure Profile instance is same.

Add DCHECK to make sure Profile instance is same.

The Profile instance owned by ArcAppListPrefs and
ArcSessionManager::profile() must be same.
This CL adds the DCHECK.

During the test, it turned out that ARC is enabled for
unexpected tests.
As a result, ArcAppListPrefs is expected to be alive at most
one instance (for Primary user Profile), but many instances
are created. Such tests are not designed to use ARC,
so disable ARC for them to pass them.

The command_line flag is moved from sync_arc_package_helper.cc.
However, tests using sync_arc_package_helper are currently
failing (crbug.com/694261), and redesign looks needed.
Thus, this CL leaves those tests as is. In future, probably
with fixing the bug, the flag needs to be added there.

BUG=694261
BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2708693002
Cr-Commit-Position: refs/heads/master@{#452366}
Committed: https://chromium.googlesource.com/chromium/src/+/653559e3d446669fe50661afb09ffdc464a37c5c

[hidehiko, 2017-02-20 14:57:53]

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

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

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-20 15:34:38]

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

[commit-bot: I haz the power, 2017-02-20 15:34:39]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-20 16:41:07]

hidehiko@chromium.org changed reviewers:
+ skym@chromium.org, xiyuan@chromium.org, yusukes@chromium.org

[hidehiko, 2017-02-20 16:41:08]

While testing crrev.com/2702723002, I found a corner case.
To split the concept from it, I created another CL (this) focusing on the bug
fix.

PTAL.
yusukes@: ARC expert.
xiyuan@: c/b/ui/app_list OWNER.
skym@: c/b/sync OWNER.

CC: lgchen@ whose is the original author of sync_arc_package_helper, and its
clients.

Thank you for review in advance,
- hidehiko

[xiyuan, 2017-02-21 17:15:27]

lgtm

[lgcheng, 2017-02-21 17:27:54]

Description was changed from

==========
Add DCHECK to make sure Profile instance is same.

The Profile instance owned by ArcAppListPrefs and
ArcSessionManager::profile() must be same.
This CL adds the DCHECK.

During the test, it turned out that ARC is enabled for
unexpected tests.
As a result, ArcAppListPrefs is expected to be alive at most
one instance (for Primary user Profile), but many instances
are created. Such tests are not designed to use ARC,
so disable ARC for them to pass them.

The command_line flag is moved from sync_arc_package_helper.cc.
However, tests using sync_arc_package_helper are currently
failing (crbug.com/694261), and redesign looks needed.
Thus, this CL leaves those tests as is. In future, probably
with fixing the bug, the flag needs to be added there.

BUG=694261
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

to

==========
Add DCHECK to make sure Profile instance is same.

The Profile instance owned by ArcAppListPrefs and
ArcSessionManager::profile() must be same.
This CL adds the DCHECK.

During the test, it turned out that ARC is enabled for
unexpected tests.
As a result, ArcAppListPrefs is expected to be alive at most
one instance (for Primary user Profile), but many instances
are created. Such tests are not designed to use ARC,
so disable ARC for them to pass them.

The command_line flag is moved from sync_arc_package_helper.cc.
However, tests using sync_arc_package_helper are currently
failing (crbug.com/694261), and redesign looks needed.
Thus, this CL leaves those tests as is. In future, probably
with fixing the bug, the flag needs to be added there.

BUG=694261
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

[lgcheng, 2017-02-21 17:27:54]

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

[lgcheng, 2017-02-21 17:30:26]

+ Luis

I think we intentionally make ArcAppListPrefs non-singleton to support SyncTest
infrastructure.

[hidehiko, 2017-02-21 17:52:00]

On 2017/02/21 17:30:26, lgcheng wrote:
> + Luis
> 
> I think we intentionally make ArcAppListPrefs non-singleton to support
SyncTest
> infrastructure.

Thank you for comment. This CL focuses just fixing SyncTest tests without ARC.
And, it is out of focus to fix ARC SyncTest tests, because they are broken by
another reason.

Note:
If SyncTest requires to create multiple ArcAppListPrefs instances,
we should remove the assumption that ArcAppListPrefs::profile_ is as same as
ASM::profile(), I think.

[skym, 2017-02-21 19:14:43]

c/b/sync lgtm

The TwoClient* sync integration tests do require that multiple profiles are
running in the same Chrome process. I don't quite follow all of the ARC specific
details going on here, but I think in general it'd be better to have profile
scoped services, and not global singletons. Sync itself only wants to deal with
data that's scoped to a single profile anyways, which is why we're typically
able to fake multiple devices by using multiple profiles in these tests.

[skym, 2017-02-21 19:15:49]

On 2017/02/21 19:14:43, skym wrote:
> c/b/sync lgtm
> 
> The TwoClient* sync integration tests do require that multiple profiles are
> running in the same Chrome process. I don't quite follow all of the ARC
specific
> details going on here, but I think in general it'd be better to have profile
> scoped services, and not global singletons. Sync itself only wants to deal
with
> data that's scoped to a single profile anyways, which is why we're typically
> able to fake multiple devices by using multiple profiles in these tests.

Alternatively you could remove the TwoClientArc* tests, or we could add a
mechanism to run when we're not in E2E mode, but I think this is less than
ideal.

[Yusuke Sato, 2017-02-21 20:31:46]

lgtm

[Yusuke Sato, 2017-02-21 20:32:25]

(sorry for the delay. Monday was a US holiday.)

[hidehiko, 2017-02-23 02:07:17]

Thank you for review. Submitting.

> The TwoClient* sync integration tests do require that multiple profiles are
> running in the same Chrome process. I don't quite follow all of the ARC
specific
> details going on here, but I think in general it'd be better to have profile
> scoped services, and not global singletons. Sync itself only wants to deal
with
> data that's scoped to a single profile anyways, which is why we're typically
> able to fake multiple devices by using multiple profiles in these tests.

> Alternatively you could remove the TwoClientArc* tests, or we could add a
> mechanism to run when we're not in E2E mode, but I think this is less than
> ideal.

Thank you very much for advice.
lgcheng@, could you take a look at it as part of crbug.com/694261? I'm familiar
with ARC instance stuff, but not very familiar with the tests. So I'd appreciate
if you can work on it.

Note: ARC is designed to be system global singleton, and users other than
primary user cannot use it, considering the container communication.
If multiple ArcAppListPrefs instances are needed for the test, it may be needed
to define the behavior for the ArcAppListPrefs instance tied to non-ARC-allowed
profile.

[hidehiko, 2017-02-23 02:07:27]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2017-02-23 02:07: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-23 03:06:14]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487815647019400, "parent_rev":
"bf7bc8e1a495e0c0168bf79e1303a2eadc41df10", "commit_rev":
"653559e3d446669fe50661afb09ffdc464a37c5c"}

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

Description was changed from

==========
Add DCHECK to make sure Profile instance is same.

The Profile instance owned by ArcAppListPrefs and
ArcSessionManager::profile() must be same.
This CL adds the DCHECK.

During the test, it turned out that ARC is enabled for
unexpected tests.
As a result, ArcAppListPrefs is expected to be alive at most
one instance (for Primary user Profile), but many instances
are created. Such tests are not designed to use ARC,
so disable ARC for them to pass them.

The command_line flag is moved from sync_arc_package_helper.cc.
However, tests using sync_arc_package_helper are currently
failing (crbug.com/694261), and redesign looks needed.
Thus, this CL leaves those tests as is. In future, probably
with fixing the bug, the flag needs to be added there.

BUG=694261
BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

to

==========
Add DCHECK to make sure Profile instance is same.

The Profile instance owned by ArcAppListPrefs and
ArcSessionManager::profile() must be same.
This CL adds the DCHECK.

During the test, it turned out that ARC is enabled for
unexpected tests.
As a result, ArcAppListPrefs is expected to be alive at most
one instance (for Primary user Profile), but many instances
are created. Such tests are not designed to use ARC,
so disable ARC for them to pass them.

The command_line flag is moved from sync_arc_package_helper.cc.
However, tests using sync_arc_package_helper are currently
failing (crbug.com/694261), and redesign looks needed.
Thus, this CL leaves those tests as is. In future, probably
with fixing the bug, the flag needs to be added there.

BUG=694261
BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2708693002
Cr-Commit-Position: refs/heads/master@{#452366}
Committed:
https://chromium.googlesource.com/chromium/src/+/653559e3d446669fe50661afb09f...
==========

[commit-bot: I haz the power, 2017-02-23 03:06:51]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/653559e3d446669fe50661afb09f...