Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104463002:
[Media Router] Add a context menu item to access the chrome://cast page (Closed)
Description[Media Router] Add a context menu item to access the chrome://cast page
The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu.
BUG=623048
Committed: https://crrev.com/cb7c961aea3ee23c4e246f50441e5dc582565129
Cr-Commit-Position: refs/heads/master@{#402330}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed review comments. Explicitly whitelisted the new menu item to Windows, OSX and ChromeOS. #
Total comments: 2
Messages
Total messages: 24 (12 generated)
Description was changed from ========== . BUG=623048 ========== to ========== Added an item to Media Router's context menu. The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ==========
sheretov@chromium.org changed reviewers: + apacible@chromium.org
sheretov@chromium.org changed required reviewers: + apacible@chromium.org
lgtm+nit You'll also need to copy the title of the issue into the first line of the description. The commit logs only pull the issue descriptions into git log in chromium. https://codereview.chromium.org/2104463002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2104463002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:133: chrome::ShowSingletonTab(browser_, GURL(kManageCastDevicesUrl)); Do we expect this URL to change in the future? If not, this could be kChromeUICastURL. https://cs.chromium.org/chromium/src/chrome/common/url_constants.cc?l=150
Description was changed from ========== Added an item to Media Router's context menu. The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ========== to ========== [Media Router] Add a context menu item to access the chrome://cast page The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ==========
sheretov@chromium.org changed reviewers: + cpu@chromium.org, msw@chromium.org
sheretov@chromium.org changed required reviewers: + cpu@chromium.org, msw@chromium.org
Adding reviewers: @cpu for chrome/app/chrome_command_ids.h @msw for chrome/browser/ui/toolbar/media_router_contextual_menu.cc https://codereview.chromium.org/2104463002/diff/1/chrome/browser/ui/toolbar/m... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2104463002/diff/1/chrome/browser/ui/toolbar/m... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:133: chrome::ShowSingletonTab(browser_, GURL(kManageCastDevicesUrl)); On 2016/06/27 19:54:56, apacible wrote: > Do we expect this URL to change in the future? If not, this could be > kChromeUICastURL. > > https://cs.chromium.org/chromium/src/chrome/common/url_constants.cc?l=150 Done.
lgtm for chrome_command_ids.h
https://codereview.chromium.org/2104463002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2104463002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:46: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS) q: Is this not supported on Linux desktop?
https://codereview.chromium.org/2104463002/diff/20001/chrome/browser/ui/toolb... File chrome/browser/ui/toolbar/media_router_contextual_menu.cc (right): https://codereview.chromium.org/2104463002/diff/20001/chrome/browser/ui/toolb... chrome/browser/ui/toolbar/media_router_contextual_menu.cc:46: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS) On 2016/06/27 20:51:37, msw wrote: > q: Is this not supported on Linux desktop? Correct: not supported on Linux. https://cs.chromium.org/chromium/src/chrome/common/url_constants.h?l=140
lgtm
The CQ bit was checked by sheretov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sheretov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from apacible@chromium.org Link to the patchset: https://codereview.chromium.org/2104463002/#ps20001 (title: "Addressed review comments. Explicitly whitelisted the new menu item to Windows, OSX and ChromeOS.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add a context menu item to access the chrome://cast page The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ========== to ========== [Media Router] Add a context menu item to access the chrome://cast page The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add a context menu item to access the chrome://cast page The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 ========== to ========== [Media Router] Add a context menu item to access the chrome://cast page The new menu item is titled "Manage Cast Devices". It goes immediately below the second separator in the menu. BUG=623048 Committed: https://crrev.com/cb7c961aea3ee23c4e246f50441e5dc582565129 Cr-Commit-Position: refs/heads/master@{#402330} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cb7c961aea3ee23c4e246f50441e5dc582565129 Cr-Commit-Position: refs/heads/master@{#402330} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
