[Media Router] Allow users to update cloud services pref when sync is not active.

[Media Router] Allow users to update cloud services pref when sync is not active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync enabled. There is no technical tie-in between having sync active and using cloud services. This change makes it such that users can toggle their cloud services pref locally if sync is inactive. Now, we only check that the user is authenticated. While sync is off, however, the pref will not sync across their devices.

In the event where the user has already acknowledged the first run flow (locally on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330
Committed: https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17
Cr-Commit-Position: refs/heads/master@{#402072}

[apacible, 2016-06-19 04:15:08]

Description was changed from

==========
cloud without sync

BUG=
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.



BUG=621255
==========

[apacible, 2016-06-19 04:32:27]

Description was changed from

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.



BUG=621255
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services.

This change makes it such that users can toggle their cloud services pref
locally if sync is inactive. While sync is off, however, the pref will not sync
across their devices.

BUG=621255
==========

[apacible, 2016-06-19 04:36:04]

Description was changed from

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services.

This change makes it such that users can toggle their cloud services pref
locally if sync is inactive. While sync is off, however, the pref will not sync
across their devices.

BUG=621255
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255
==========

[apacible, 2016-06-20 16:26:38]

apacible@chromium.org changed reviewers:
+ imcheng@chromium.org, pkasting@chromium.org

[apacible, 2016-06-20 16:26:38]

PTAL, thanks!

[Peter Kasting, 2016-06-20 21:08:42]

LGTM.

Should this have a unit test?

[imcheng, 2016-06-20 21:29:03]

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:73: if
(browser_->profile()->IsSyncAllowed()) {
Do we still need to check IsSyncAllowed()?

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:819:
ProfileSyncServiceFactory::GetForProfile(profile)->IsSyncActive()) {
Do we not need to check IsSyncAllowed() anymore because it is implied by
IsSyncActive()?

[apacible, 2016-06-22 05:34:48]

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/toolbar/m...
File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right):

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/toolbar/m...
chrome/browser/ui/toolbar/media_router_contextual_menu.cc:73: if
(browser_->profile()->IsSyncAllowed()) {
On 2016/06/20 21:29:03, imcheng wrote:
> Do we still need to check IsSyncAllowed()?

Removed.

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2078213002/diff/1/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:819:
ProfileSyncServiceFactory::GetForProfile(profile)->IsSyncActive()) {
On 2016/06/20 21:29:03, imcheng wrote:
> Do we not need to check IsSyncAllowed() anymore because it is implied by
> IsSyncActive()?

Correct.

[imcheng, 2016-06-22 17:22:46]

lgtm

[imcheng, 2016-06-22 17:27:19]

+1 pkasting@'s comment on unit tests.

[apacible, 2016-06-22 18:08:24]

On 2016/06/22 17:27:19, imcheng wrote:
> +1 pkasting@'s comment on unit tests.

Thanks! Missed the unit test comment; I'll look into adding tests later today.

[apacible, 2016-06-25 18:47:10]

Patchset #3 (id:40001) has been deleted

[apacible, 2016-06-25 22:18:24]

Patchset #4 (id:80001) has been deleted

[apacible, 2016-06-25 22:26:40]

Patchset #3 (id:60001) has been deleted

[apacible, 2016-06-25 22:27:13]

Patchset #3 (id:100001) has been deleted

[apacible, 2016-06-25 22:57:34]

Patchset #3 (id:120001) has been deleted

[apacible, 2016-06-25 23:17:19]

Patchset #3 (id:140001) has been deleted

[apacible, 2016-06-25 23:50:54]

Description was changed from

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330
==========

[apacible, 2016-06-25 23:53:14]

Updated with some basic tests for the contextual menu. I have some questions
about writing tests specifically for ifdef MR cases, which affect testing our
prefs, so I filed crbug/623330 for that.

[apacible, 2016-06-25 23:53:31]

The CQ bit was checked by apacible@chromium.org

[apacible, 2016-06-25 23:53:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2078213002/#ps160001
(title: "Add basic unit tests. Rebase.")

[commit-bot: I haz the power, 2016-06-25 23:53:43]

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-06-25 23:57:33]

Description was changed from

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330
==========

[commit-bot: I haz the power, 2016-06-25 23:57:34]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-06-25 23:58:58]

Description was changed from

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330
==========

to

==========
[Media Router] Allow users to update cloud services pref when sync is not
active.

There are two places users can update their cloud services pref:
- First run flow, with a checkbox
- Contextual menu, with a toggle

Currently, users can only update their cloud services pref if they have sync
enabled. There is no technical tie-in between having sync active and using cloud
services. This change makes it such that users can toggle their cloud services
pref locally if sync is inactive. Now, we only check that the user is
authenticated. While sync is off, however, the pref will not sync across their
devices.

In the event where the user has already acknowledged the first run flow (locally
on the profile), then turned on sync, we continue to enable cloud services.

BUG=621255, 623330

Committed: https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17
Cr-Commit-Position: refs/heads/master@{#402072}
==========

[commit-bot: I haz the power, 2016-06-25 23:58:59]

Patchset 3 (id:??) landed as
https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17
Cr-Commit-Position: refs/heads/master@{#402072}

[apacible, 2016-06-26 00:20:51]

A revert of this CL (patchset #3 id:160001) has been created in
https://codereview.chromium.org/2098193002/ by apacible@chromium.org.

The reason for reverting is: Caused failure on official bots..