New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 623330 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
no longer active
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[Media Router] Add full test coverage for MediaRouterContextualMenu.

Project Member Reported by apaci...@chromium.org, Jun 25 2016

Issue description

What the title says.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19a9b8f1411afe64b2163261a04245f617e25e17

commit 19a9b8f1411afe64b2163261a04245f617e25e17
Author: apacible <apacible@chromium.org>
Date: Sat Jun 25 23:57:12 2016

[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 

Review-Url: https://codereview.chromium.org/2078213002
Cr-Commit-Position: refs/heads/master@{#402072}

[modify] https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[add] https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
[modify] https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17/chrome/chrome_tests_unit.gypi

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03b18e91b52deceb82445a6739abeb595d781c15

commit 03b18e91b52deceb82445a6739abeb595d781c15
Author: apacible <apacible@chromium.org>
Date: Sun Jun 26 00:21:17 2016

Revert of [Media Router] Allow users to update cloud services pref when sync is not active. (patchset #3 id:160001 of https://codereview.chromium.org/2078213002/ )

Reason for revert:
Caused failure on official bots.

Original issue's description:
> [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}

TBR=pkasting@chromium.org,imcheng@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 621255 ,  623330 

Review-Url: https://codereview.chromium.org/2098193002
Cr-Commit-Position: refs/heads/master@{#402073}

[modify] https://crrev.com/03b18e91b52deceb82445a6739abeb595d781c15/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[delete] https://crrev.com/19a9b8f1411afe64b2163261a04245f617e25e17/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/03b18e91b52deceb82445a6739abeb595d781c15/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
[modify] https://crrev.com/03b18e91b52deceb82445a6739abeb595d781c15/chrome/chrome_tests_unit.gypi

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f

commit eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f
Author: apacible <apacible@chromium.org>
Date: Tue Jun 28 23:45:33 2016

[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)

Review-Url: https://codereview.chromium.org/2093353002
Cr-Commit-Position: refs/heads/master@{#402596}

[modify] https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[add] https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
[modify] https://crrev.com/eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f/chrome/chrome_tests_unit.gypi

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 2 2016

Labels: merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/df14c0d9212169faf3450c4d82b60d9d358fab1e

commit df14c0d9212169faf3450c4d82b60d9d358fab1e
Author: apacible <apacible@chromium.org>
Date: Sat Jul 02 02:44:07 2016

[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 
TBR=msw, imcheng
NOTRY=true
NOPRESUBMIT=true

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

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

Review-Url: https://codereview.chromium.org/2093353002
Cr-Commit-Position: refs/heads/master@{#402596}
(cherry picked from commit eae3fc0f7fd78faa2d97ce24c150af30f7f4de1f)

Review-Url: https://codereview.chromium.org/2115203002
Cr-Commit-Position: refs/branch-heads/2743@{#575}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/df14c0d9212169faf3450c4d82b60d9d358fab1e/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[add] https://crrev.com/df14c0d9212169faf3450c4d82b60d9d358fab1e/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/df14c0d9212169faf3450c4d82b60d9d358fab1e/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
[modify] https://crrev.com/df14c0d9212169faf3450c4d82b60d9d358fab1e/chrome/chrome_tests_unit.gypi

Cc: brajkumar@chromium.org
Labels: Needs-Feedback
Apacible@ Is there any manual repro steps available to verify this issue from Chrome TE end.


Labels: -Needs-Feedback
brajkumar@: No, this issue just tracks adding extra test coverage for a menu. There are no new features or bug fixes. The earlier patch that landed was verified on the other bug it was attached to.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5dc8d635fceca17f8bf56044865ae4afb462f48c

commit 5dc8d635fceca17f8bf56044865ae4afb462f48c
Author: apacible <apacible@chromium.org>
Date: Fri Jul 29 15:23:45 2016

[Media Router Contextual Menu] Test 'checked' state of cloud services item.

Add a test to check whether the "Enable cloud services" item is currently checked or not.

BUG= 623330 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2190223002
Cr-Commit-Position: refs/heads/master@{#408640}

[modify] https://crrev.com/5dc8d635fceca17f8bf56044865ae4afb462f48c/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/003691355b04b283dd67dd12ea838816d1bc1184

commit 003691355b04b283dd67dd12ea838816d1bc1184
Author: apacible <apacible@chromium.org>
Date: Fri Jul 29 17:49:47 2016

Revert of [Media Router Contextual Menu] Test 'checked' state of cloud services item. (patchset #2 id:60001 of https://codereview.chromium.org/2190223002/ )

Reason for revert:
Broke official build.

Original issue's description:
> [Media Router Contextual Menu] Test 'checked' state of cloud services item.
>
> Add a test to check whether the "Enable cloud services" item is currently checked or not.
>
> BUG= 623330 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
>
> Committed: https://crrev.com/5dc8d635fceca17f8bf56044865ae4afb462f48c
> Cr-Commit-Position: refs/heads/master@{#408640}

TBR=msw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 623330 

Review-Url: https://codereview.chromium.org/2196453004
Cr-Commit-Position: refs/heads/master@{#408681}

[modify] https://crrev.com/003691355b04b283dd67dd12ea838816d1bc1184/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efa5e4609725d61ef253b54cd45762baba8b9020

commit efa5e4609725d61ef253b54cd45762baba8b9020
Author: apacible <apacible@chromium.org>
Date: Tue Aug 09 07:21:51 2016

[Reland] [Media Router Contextual Menu] Test 'checked' state of cloud services item.

Add a test to check whether the "Enable cloud services" item is currently checked or not.

Previous patch was reverted as it broke official bots. I removed the friend test declaration inadvertently.
Patch: https://codereview.chromium.org/2190223002

BUG= 623330 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2201423003
Cr-Commit-Position: refs/heads/master@{#410605}

[modify] https://crrev.com/efa5e4609725d61ef253b54cd45762baba8b9020/chrome/browser/ui/toolbar/media_router_contextual_menu.h
[modify] https://crrev.com/efa5e4609725d61ef253b54cd45762baba8b9020/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment