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

[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on official bots; there were #includes that were mistakenly removed.

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

patch from issue 2078213002 at patchset 160001 (http://crrev.com/2078213002#ps160001)

Committed: https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f
Cr-Commit-Position: refs/heads/master@{#402596}

[apacible, 2016-06-26 03:51:41]

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

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

to

==========
[Reland] [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}

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

[apacible, 2016-06-26 03:55:52]

Description was changed from

==========
[Reland] [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}

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

to

==========
[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on
official bots; there were #includes that were mistakenly removed.

BUG=621255, 623330

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

[apacible, 2016-06-26 03:55:52]

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

[apacible, 2016-06-26 04:24:28]

Patchset #2 (id:20001) has been deleted

[apacible, 2016-06-26 04:28:52]

PTAL, thanks!

+msw for chrome/browser/ui/toolbar/ OWNERS (pkasting OOO)

[imcheng, 2016-06-27 18:16:44]

lgtm

[msw, 2016-06-27 19:10:23]

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right):

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:14: class
TestSimpleMenuModel : public ui::SimpleMenuModel {
Remove this; it's not used.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:52: #if
!defined(GOOGLE_CHROME_BUILD)
nit: add a blank line after this.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:55:
TEST_F(MediaRouterContextualMenuUnitTest, BasicNotOfficialChromeBuild) {
nit: I'd rather see these two fixtures combined into one fixture with two
smaller #if blocks:

  int expected_number_items = 7;
#if defined(GOOGLE_CHROME_BUILD)
  // Official Chrome builds have an additional menu item for cloud services.
  expected_number_items = 7;
#endif  // GOOGLE_CHROME_BUILD

...

  for (int i = 0; i < expected_number_items; i++) {
    EXPECT_TRUE(model()->IsEnabledAt(i));
    bool expected_visible = true;
#if defined(GOOGLE_CHROME_BUILD)
    // In official chrome builds, the cloud services toggle exists and is
    // enabled, but not visible until the user has authenticated their account.
    expected_visible =
        model()->GetCommandIdAt(i) != IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE;
#endif  // GOOGLE_CHROME_BUILD
    EXPECT_EQ(expected_visible, model()->IsVisibleAt(i));
  }

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:77: #else
nit: add blank lines before and after this.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:89: // The
cloud services toggle exists an is enabled, but not visible until
nit: s/an/and/

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:110: #endif 
// defined(GOOGLE_CHROME_BUILD)
nit: add a blank line before this.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:821:
ProfileSyncServiceFactory::GetForProfile(profile)->IsSyncActive()) {
q: Are these automatically propagated if/when sync is enabled later?

[apacible, 2016-06-28 16:55:54]

Patchset #4 (id:80001) has been deleted

[apacible, 2016-06-28 17:01:38]

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc (right):

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:14: class
TestSimpleMenuModel : public ui::SimpleMenuModel {
On 2016/06/27 19:10:22, msw wrote:
> Remove this; it's not used.

Done.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:52: #if
!defined(GOOGLE_CHROME_BUILD)
On 2016/06/27 19:10:22, msw wrote:
> nit: add a blank line after this.

Done.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:55:
TEST_F(MediaRouterContextualMenuUnitTest, BasicNotOfficialChromeBuild) {
On 2016/06/27 19:10:22, msw wrote:
> nit: I'd rather see these two fixtures combined into one fixture with two
> smaller #if blocks:
> 
>   int expected_number_items = 7;
> #if defined(GOOGLE_CHROME_BUILD)
>   // Official Chrome builds have an additional menu item for cloud services.
>   expected_number_items = 7;
> #endif  // GOOGLE_CHROME_BUILD
> 
> ...
> 
>   for (int i = 0; i < expected_number_items; i++) {
>     EXPECT_TRUE(model()->IsEnabledAt(i));
>     bool expected_visible = true;
> #if defined(GOOGLE_CHROME_BUILD)
>     // In official chrome builds, the cloud services toggle exists and is
>     // enabled, but not visible until the user has authenticated their
account.
>     expected_visible =
>         model()->GetCommandIdAt(i) != IDC_MEDIA_ROUTER_CLOUD_SERVICES_TOGGLE;
> #endif  // GOOGLE_CHROME_BUILD
>     EXPECT_EQ(expected_visible, model()->IsVisibleAt(i));
>   }

Thanks, changed to single test.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:77: #else
On 2016/06/27 19:10:23, msw wrote:
> nit: add blank lines before and after this.

Acknowledged.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:89: // The
cloud services toggle exists an is enabled, but not visible until
On 2016/06/27 19:10:23, msw wrote:
> nit: s/an/and/

Done.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc:110: #endif 
// defined(GOOGLE_CHROME_BUILD)
On 2016/06/27 19:10:23, msw wrote:
> nit: add a blank line before this.

Acknowledged.

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2093353002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:821:
ProfileSyncServiceFactory::GetForProfile(profile)->IsSyncActive()) {
On 2016/06/27 19:10:23, msw wrote:
> q: Are these automatically propagated if/when sync is enabled later?

Correct.

[msw, 2016-06-28 17:29:49]

lgtm

[apacible, 2016-06-28 18:00:02]

The CQ bit was checked by apacible@chromium.org

[apacible, 2016-06-28 18:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2093353002/#ps100001
(title: "Changes per msw@'s comments.")

[commit-bot: I haz the power, 2016-06-28 18:00:46]

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-28 19:56:21]

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

[commit-bot: I haz the power, 2016-06-28 19:56:22]

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

[apacible, 2016-06-28 22:03:32]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2016-06-28 22:06:17]

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-28 23:46:19]

Description was changed from

==========
[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on
official bots; there were #includes that were mistakenly removed.

BUG=621255, 623330

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

to

==========
[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on
official bots; there were #includes that were mistakenly removed.

BUG=621255, 623330

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

[commit-bot: I haz the power, 2016-06-28 23:46:20]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-06-28 23:49:39]

Description was changed from

==========
[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on
official bots; there were #includes that were mistakenly removed.

BUG=621255, 623330

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)
==========

to

==========
[Reland] [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.

[Reland Comments] This was initially reverted because it caused failures on
official bots; there were #includes that were mistakenly removed.

BUG=621255, 623330

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

patch from issue 2078213002 at patchset 160001
(http://crrev.com/2078213002#ps160001)

Committed: https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f
Cr-Commit-Position: refs/heads/master@{#402596}
==========

[commit-bot: I haz the power, 2016-06-28 23:49:40]

Patchset 4 (id:??) landed as
https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f
Cr-Commit-Position: refs/heads/master@{#402596}