Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2862393002: [Media Router] Force DEFAULT cast mode when starting presentations from content. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 6 days ago by mark a. foltz
Modified:
6 days, 9 hours ago
Reviewers:
takumif, apacible
CC:
arv+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, mfoltz+watch_chromium.org, jam
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Force DEFAULT cast mode when starting presentation from content. The Presentation API spec recommends that the origin requesting presentation be shown to the user when PresentationRequest.start() is called. This patch sets a new flag on a CastMode, isForced, that will cause the cast mode picker to show it initially when it is rendered. This flag is set when the Media Router UI is initialized from a PresentationRequest (e.g., from PresentationRequest.start()). There are a few cleanups that could be done as followup changes: - DEFAULT cast mode is more logically PRESENTATION since the request can come from any PresentationRequest (not just the page-default one). - CreatePresentationConnectionRequest and PresentationRequest are very similar and could be combined. - A default content::PresentationError is initialized with type NO_AVAILABLE_SCREENS. It would seem more logical to initialize with type UNKNOWN. TBR jam@ for a comment only change to presentation_info.h. BUG=704964 TBR=jam CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2862393002 Cr-Commit-Position: refs/heads/master@{#473330} Committed: https://chromium.googlesource.com/chromium/src/+/eeb1406b336d5834d9362fad5e16af8ca45e6f0e

Patch Set 1 #

Patch Set 2 : Updating unittests. #

Patch Set 3 : Adds unit tests where possible. #

Patch Set 4 : Update media_router_ui.cc #

Patch Set 5 : Update unittests #

Total comments: 4

Patch Set 6 : Respond to takumif@ comments #

Patch Set 7 : Fix YouTube bug #

Patch Set 8 : Extend CastModeListTests to check sink list #

Total comments: 4

Patch Set 9 : Remove logging #

Patch Set 10 : Address apacible@ comment #

Patch Set 11 : Rebase #

Patch Set 12 : Fix bug found by closure compiler #

Unified diffs Side-by-side diffs Delta from patch set Stats (+260 lines, -57 lines) Patch
M chrome/browser/media/router/create_presentation_connection_request.h View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js View 1 2 3 4 5 6 4 chunks +36 lines, -14 lines 0 comments Download
M chrome/browser/resources/media_router/media_router_data.js View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_cast_mode.h View 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui.h View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui.cc View 1 2 3 4 5 6 4 chunks +16 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_ui_unittest.cc View 1 2 3 4 3 chunks +64 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_webui_message_handler.h View 1 2 3 4 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc View 4 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc View 1 5 chunks +48 lines, -22 lines 0 comments Download
M chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js View 1 2 3 4 5 6 7 8 9 3 chunks +42 lines, -0 lines 0 comments Download
M chrome/test/data/webui/media_router/media_router_container_test_base.js View 1 2 3 4 3 chunks +18 lines, -6 lines 0 comments Download
M content/public/common/presentation_info.h View 1 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 34 (14 generated)
mark a. foltz
PTAL It works via manual testing, but unit testing this is tricky: - MediaRouterUI tests ...
2 weeks, 3 days ago (2017-05-08 20:52:57 UTC) #5
mark a. foltz
On 2017/05/08 at 20:52:57, mark a. foltz wrote: > PTAL > > It works via ...
2 weeks, 3 days ago (2017-05-08 21:10:05 UTC) #6
mark a. foltz
On 2017/05/08 at 21:10:05, mark a. foltz wrote: > On 2017/05/08 at 20:52:57, mark a. ...
1 week, 2 days ago (2017-05-16 20:46:13 UTC) #9
takumif
A few questions about the expected behavior: - If the user wants to use a ...
1 week, 2 days ago (2017-05-16 21:51:29 UTC) #10
mark a. foltz
https://codereview.chromium.org/2862393002/diff/80001/chrome/browser/media/router/create_presentation_connection_request.h File chrome/browser/media/router/create_presentation_connection_request.h (right): https://codereview.chromium.org/2862393002/diff/80001/chrome/browser/media/router/create_presentation_connection_request.h#newcode47 chrome/browser/media/router/create_presentation_connection_request.h:47: // |is_top_level_frame|: True if the request is from a ...
1 week, 2 days ago (2017-05-16 22:02:11 UTC) #11
mark a. foltz
On 2017/05/16 at 21:51:29, takumif wrote: > A few questions about the expected behavior: > ...
1 week, 2 days ago (2017-05-16 22:07:56 UTC) #12
takumif
On 2017/05/16 22:07:56, mark a. foltz wrote: > On 2017/05/16 at 21:51:29, takumif wrote: > ...
1 week, 2 days ago (2017-05-16 22:25:05 UTC) #13
mark a. foltz
On 2017/05/16 at 22:25:05, takumif wrote: > On 2017/05/16 22:07:56, mark a. foltz wrote: > ...
1 week, 2 days ago (2017-05-16 22:29:35 UTC) #14
takumif
> We could introduce a new cast mode - AUTO_WITH_ORIGIN - that tries to replicate ...
1 week, 2 days ago (2017-05-16 22:50:38 UTC) #15
mark a. foltz
Found the bug in YouTube. Will write a test case to regress this once the ...
1 week, 2 days ago (2017-05-17 00:20:19 UTC) #16
mark a. foltz
Per chat with skonig@ the new behavior is less confusing and fine to land, but ...
1 week, 1 day ago (2017-05-17 17:16:51 UTC) #17
mark a. foltz
Updated test case. PTAL
1 week, 1 day ago (2017-05-17 21:33:53 UTC) #18
takumif
LGTM https://codereview.chromium.org/2862393002/diff/140001/chrome/browser/resources/media_router/media_router_ui_interface.js File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2862393002/diff/140001/chrome/browser/resources/media_router/media_router_ui_interface.js#newcode155 chrome/browser/resources/media_router/media_router_ui_interface.js:155: console.log('setSinkListAndIdentity: ' + JSON.stringify(data['sinks'])); Remove the logging?
1 week, 1 day ago (2017-05-17 22:06:47 UTC) #19
mark a. foltz
https://codereview.chromium.org/2862393002/diff/140001/chrome/browser/resources/media_router/media_router_ui_interface.js File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2862393002/diff/140001/chrome/browser/resources/media_router/media_router_ui_interface.js#newcode155 chrome/browser/resources/media_router/media_router_ui_interface.js:155: console.log('setSinkListAndIdentity: ' + JSON.stringify(data['sinks'])); On 2017/05/17 at 22:06:47, takumif ...
1 week ago (2017-05-18 17:35:25 UTC) #20
apacible
lgtm https://codereview.chromium.org/2862393002/diff/140001/chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/2862393002/diff/140001/chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js#newcode359 chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:359: // The sink list contains only sinks compatible ...
6 days, 16 hours ago (2017-05-19 14:54:38 UTC) #21
mark a. foltz
https://codereview.chromium.org/2862393002/diff/140001/chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/2862393002/diff/140001/chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js#newcode359 chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:359: // The sink list contains only sinks compatible with ...
6 days, 12 hours ago (2017-05-19 18:30:32 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862393002/200001
6 days, 12 hours ago (2017-05-19 18:33:08 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compilation/builds/8792)
6 days, 12 hours ago (2017-05-19 18:47:18 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862393002/220001
6 days, 11 hours ago (2017-05-19 20:15:09 UTC) #31
commit-bot: I haz the power
6 days, 9 hours ago (2017-05-19 21:35:34 UTC) #34
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/eeb1406b336d5834d9362fad5e16...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06