Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(318)

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

Created:
3 years, 7 months ago by mark a. foltz
Modified:
3 years, 7 months 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

Messages

Total messages: 34 (14 generated)
mark a. foltz
PTAL It works via manual testing, but unit testing this is tricky: - MediaRouterUI tests ...
3 years, 7 months 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 ...
3 years, 7 months 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. ...
3 years, 7 months ago (2017-05-16 20:46:13 UTC) #9
takumif
A few questions about the expected behavior: - If the user wants to use a ...
3 years, 7 months 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 ...
3 years, 7 months 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: > ...
3 years, 7 months 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: > ...
3 years, 7 months 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: > ...
3 years, 7 months ago (2017-05-16 22:29:35 UTC) #14
takumif
> We could introduce a new cast mode - AUTO_WITH_ORIGIN - that tries to replicate ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months ago (2017-05-17 17:16:51 UTC) #17
mark a. foltz
Updated test case. PTAL
3 years, 7 months 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?
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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
3 years, 7 months 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)
3 years, 7 months 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
3 years, 7 months ago (2017-05-19 20:15:09 UTC) #31
commit-bot: I haz the power
3 years, 7 months 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...

Powered by Google App Engine
This is Rietveld 408576698