Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487673003:
[Media Router] Make per-hostname cast mode selections persist (Closed)
Description[Media Router] Make per-hostname cast mode selections persist
This change adds a non-synced pref to record per-hostname cast mode selections
made by user in a MR dialog. Whenever the user opens a MR dialog on a site with
a recorded preference, the cast mode selection in the dialog will default to it.
We do not record the user selecting the desktop mirroring mode, since that cast
mode is not associated with the sites the user is on.
BUG=664671
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/e5d669a3e9782b50b35a24d2e380b577c73b7a16
Cr-Commit-Position: refs/heads/master@{#435461}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address Jennifer's comments, rebase #
Total comments: 12
Patch Set 3 : Rebase #Patch Set 4 : Address Derek's comments #
Total comments: 7
Patch Set 5 : Address Jennifer's comments #Patch Set 6 : Use origin instead of hostname, rebase #Patch Set 7 : Use url::Origin for serializing origins, rebase #Messages
Total messages: 37 (21 generated)
Description was changed from ========== . BUG= ========== to ========== . BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== . BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
takumif@chromium.org changed reviewers: + anthonyvd@chromium.org, apacible@chromium.org
apacible@: all the files anthonyvd@: c/b/profiles/profile.cc Please take a look, thank you! https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: if (initial_cast_mode != MediaCastMode::DEFAULT) { Right now, GetCastModeSelectionForCurrentHost() returns MediaCastMode::DEFAULT if user hasn't chosen anything, so we can't distinguish between sites where the user has chosen DEFAULT and hasn't chosen anything. So in the case the user has chosen DEFAULT we don't set the initial cast mode, but the webui defaults to DEFAULT anyways. I thought about making GetCastModeSelectionForCurrentHost() return an int (instead of MediaCastMode) and make it return 0 to indicate the user hasn't chosen anything, but that didn't seem as clean. apacible@, I wonder if the current way is okay, or there's a better way to do this.
Please create a crbug and attach it to the description. Will follow up patches handle determining the expected cast modes when the user is navigating between pages? https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:523: if (cast_mode == MediaCastMode::DESKTOP_MIRROR) { nitty nit: prefer comments before if (), then omit curly braces. https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: if (initial_cast_mode != MediaCastMode::DEFAULT) { On 2016/11/08 22:52:16, takumif wrote: > Right now, GetCastModeSelectionForCurrentHost() returns MediaCastMode::DEFAULT > if user hasn't chosen anything, so we can't distinguish between sites where the > user has chosen DEFAULT and hasn't chosen anything. So in the case the user has > chosen DEFAULT we don't set the initial cast mode, but the webui defaults to > DEFAULT anyways. > > I thought about making GetCastModeSelectionForCurrentHost() return an int > (instead of MediaCastMode) and make it return 0 to indicate the user hasn't > chosen anything, but that didn't seem as clean. apacible@, I wonder if the > current way is okay, or there's a better way to do this. This looks fine.
Description was changed from ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for reviewing! On 2016/11/11 22:40:23, apacible wrote: > Please create a crbug and attach it to the description. Done. > > Will follow up patches handle determining the expected cast modes when the user > is navigating between pages? I wonder if that's necessary. My understanding is that the dialog gets destroyed every time the user navigates, so I think it's sufficient just to fetch the preference every time the dialog is initialized. https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:523: if (cast_mode == MediaCastMode::DESKTOP_MIRROR) { On 2016/11/11 22:40:23, apacible wrote: > nitty nit: prefer comments before if (), then omit curly braces. Done. https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: if (initial_cast_mode != MediaCastMode::DEFAULT) { On 2016/11/11 22:40:23, apacible wrote: > On 2016/11/08 22:52:16, takumif wrote: > > Right now, GetCastModeSelectionForCurrentHost() returns MediaCastMode::DEFAULT > > if user hasn't chosen anything, so we can't distinguish between sites where > the > > user has chosen DEFAULT and hasn't chosen anything. So in the case the user > has > > chosen DEFAULT we don't set the initial cast mode, but the webui defaults to > > DEFAULT anyways. > > > > I thought about making GetCastModeSelectionForCurrentHost() return an int > > (instead of MediaCastMode) and make it return 0 to indicate the user hasn't > > chosen anything, but that didn't seem as clean. apacible@, I wonder if the > > current way is okay, or there's a better way to do this. > > This looks fine. Okay!
On 2016/11/12 00:08:31, takumif wrote: > Thanks for reviewing! > > On 2016/11/11 22:40:23, apacible wrote: > > Please create a crbug and attach it to the description. > Done. > > > > > Will follow up patches handle determining the expected cast modes when the > user > > is navigating between pages? > I wonder if that's necessary. My understanding is that the dialog gets destroyed > every time the user navigates, so I think it's sufficient just to fetch the > preference every time the dialog is initialized. Possible scenario: The initial site has an override of tab mirroring, but while navigating, there's a mix of cast enabled sites/flinging and more mirroring (and possibly a mix of overrides there . We should be switching modes for each site rather than using the override for the entire session.
c/b/profiles/* lgtm
Description was changed from ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
imcheng@chromium.org changed reviewers: + imcheng@chromium.org
Drive-by comments: https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2132: this.setShownCastMode_(initialCastMode); Is it possible to refactor this logic with onCastModeClick_? https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_ui_interface.js:97: * showDomain: boolean Please document initialCastMode. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:519: return static_cast<MediaCastMode>(cast_mode); DCHECK the value from pref that it is a valid cast mode. You can use the IsValidCastModeNum function. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:132: // If user hasn't chosen any, this returns MediaCastMode::DEFAULT. MediaCastMode::DEFAULT is a valid value that represents presentation. Are we sure we want to use it if the user did not choose a cast mode before? https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:425: initial_data.Set( You can use SetInteger instead which is a wrapper for Set. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:710: static_cast<MediaCastMode>(cast_mode_type)); Check if cast_mode_type is valid before recording.
https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:422: MediaCastMode initial_cast_mode = What if |initial_cast_mode| is not in |cast_modes|? https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: if (initial_cast_mode != MediaCastMode::DEFAULT) { On 2016/11/12 00:08:31, takumif wrote: > On 2016/11/11 22:40:23, apacible wrote: > > On 2016/11/08 22:52:16, takumif wrote: > > > Right now, GetCastModeSelectionForCurrentHost() returns > MediaCastMode::DEFAULT > > > if user hasn't chosen anything, so we can't distinguish between sites where > > the > > > user has chosen DEFAULT and hasn't chosen anything. So in the case the user > > has > > > chosen DEFAULT we don't set the initial cast mode, but the webui defaults to > > > DEFAULT anyways. > > > > > > I thought about making GetCastModeSelectionForCurrentHost() return an int > > > (instead of MediaCastMode) and make it return 0 to indicate the user hasn't > > > chosen anything, but that didn't seem as clean. apacible@, I wonder if the > > > current way is okay, or there's a better way to do this. > > > > This looks fine. > > Okay! Saw this comment after I posted my own comments. Note there is a slight difference in behavior if we don't set initialCastMode = DEFAULT. - if initialCastMode is not set, the dialog is in AUTO mode - which means we will show sinks that are compatible with any one cast mode, and we will create route using the best cast mode (DEFAULT > TAB_MIRROR > DESKTOP_MIRROR) when selected. - if set, the dialog will only show devices that can do DEFAULT mode. The code here effectively means we are only setting initialCastMode for TAB_MIRROR only, or not at all. Do we plan to support initialCastMode = DEFAULT? (It doesn't look very useful to me, under the assumption that DEFAULT > TAB_MIRROR). If not, would it suffice to just store a boolean instead of int as the pref? Finally, please document the logic here and any assumptions made regarding the use of DEFAULT.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
On 2016/11/14 19:56:59, imcheng wrote: > https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc > (right): > > https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:422: > MediaCastMode initial_cast_mode = > What if |initial_cast_mode| is not in |cast_modes|? > > https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: > if (initial_cast_mode != MediaCastMode::DEFAULT) { > On 2016/11/12 00:08:31, takumif wrote: > > On 2016/11/11 22:40:23, apacible wrote: > > > On 2016/11/08 22:52:16, takumif wrote: > > > > Right now, GetCastModeSelectionForCurrentHost() returns > > MediaCastMode::DEFAULT > > > > if user hasn't chosen anything, so we can't distinguish between sites > where > > > the > > > > user has chosen DEFAULT and hasn't chosen anything. So in the case the > user > > > has > > > > chosen DEFAULT we don't set the initial cast mode, but the webui defaults > to > > > > DEFAULT anyways. > > > > > > > > I thought about making GetCastModeSelectionForCurrentHost() return an int > > > > (instead of MediaCastMode) and make it return 0 to indicate the user > hasn't > > > > chosen anything, but that didn't seem as clean. apacible@, I wonder if the > > > > current way is okay, or there's a better way to do this. > > > > > > This looks fine. > > > > Okay! > > Saw this comment after I posted my own comments. Note there is a slight > difference in behavior if we don't set initialCastMode = DEFAULT. > - if initialCastMode is not set, the dialog is in AUTO mode - which means we > will show sinks that are compatible with any one cast mode, and we will create > route using the best cast mode (DEFAULT > TAB_MIRROR > DESKTOP_MIRROR) when > selected. > - if set, the dialog will only show devices that can do DEFAULT mode. > > > The code here effectively means we are only setting initialCastMode for > TAB_MIRROR only, or not at all. Do we plan to support initialCastMode = DEFAULT? > (It doesn't look very useful to me, under the assumption that DEFAULT > > TAB_MIRROR). If not, would it suffice to just store a boolean instead of int as > the pref? > > Finally, please document the logic here and any assumptions made regarding the > use of DEFAULT. Talked offline -- we should keep track of TAB_MIRROR only.
https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:422: MediaCastMode initial_cast_mode = On 2016/11/14 19:56:59, imcheng wrote: > What if |initial_cast_mode| is not in |cast_modes|? Checking to make sure TAB_MIRROR is in |cast_modes| when overriding with tab mirroring. https://codereview.chromium.org/2487673003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: if (initial_cast_mode != MediaCastMode::DEFAULT) { On 2016/11/14 19:56:59, imcheng wrote: > On 2016/11/12 00:08:31, takumif wrote: > > On 2016/11/11 22:40:23, apacible wrote: > > > On 2016/11/08 22:52:16, takumif wrote: > > > > Right now, GetCastModeSelectionForCurrentHost() returns > > MediaCastMode::DEFAULT > > > > if user hasn't chosen anything, so we can't distinguish between sites > where > > > the > > > > user has chosen DEFAULT and hasn't chosen anything. So in the case the > user > > > has > > > > chosen DEFAULT we don't set the initial cast mode, but the webui defaults > to > > > > DEFAULT anyways. > > > > > > > > I thought about making GetCastModeSelectionForCurrentHost() return an int > > > > (instead of MediaCastMode) and make it return 0 to indicate the user > hasn't > > > > chosen anything, but that didn't seem as clean. apacible@, I wonder if the > > > > current way is okay, or there's a better way to do this. > > > > > > This looks fine. > > > > Okay! > > Saw this comment after I posted my own comments. Note there is a slight > difference in behavior if we don't set initialCastMode = DEFAULT. > - if initialCastMode is not set, the dialog is in AUTO mode - which means we > will show sinks that are compatible with any one cast mode, and we will create > route using the best cast mode (DEFAULT > TAB_MIRROR > DESKTOP_MIRROR) when > selected. > - if set, the dialog will only show devices that can do DEFAULT mode. > > > The code here effectively means we are only setting initialCastMode for > TAB_MIRROR only, or not at all. Do we plan to support initialCastMode = DEFAULT? > (It doesn't look very useful to me, under the assumption that DEFAULT > > TAB_MIRROR). If not, would it suffice to just store a boolean instead of int as > the pref? > > Finally, please document the logic here and any assumptions made regarding the > use of DEFAULT. Changing the pref to only keep track of hostnames set to TAB_MIRROR. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:2132: this.setShownCastMode_(initialCastMode); On 2016/11/14 19:32:27, imcheng wrote: > Is it possible to refactor this logic with onCastModeClick_? Done. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... File chrome/browser/resources/media_router/media_router_ui_interface.js (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/resource... chrome/browser/resources/media_router/media_router_ui_interface.js:97: * showDomain: boolean On 2016/11/14 19:32:27, imcheng wrote: > Please document initialCastMode. Done. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:519: return static_cast<MediaCastMode>(cast_mode); On 2016/11/14 19:32:27, imcheng wrote: > DCHECK the value from pref that it is a valid cast mode. You can use the > IsValidCastModeNum function. Done. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.h (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.h:132: // If user hasn't chosen any, this returns MediaCastMode::DEFAULT. On 2016/11/14 19:32:27, imcheng wrote: > MediaCastMode::DEFAULT is a valid value that represents presentation. Are we > sure we want to use it if the user did not choose a cast mode before? Changing this to bool UserSelectedTabMirroringForCurrentHost(). https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:425: initial_data.Set( On 2016/11/14 19:32:27, imcheng wrote: > You can use SetInteger instead which is a wrapper for Set. Done. https://codereview.chromium.org/2487673003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:710: static_cast<MediaCastMode>(cast_mode_type)); On 2016/11/14 19:32:27, imcheng wrote: > Check if cast_mode_type is valid before recording. Done.
https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:173: registry->RegisterDictionaryPref(prefs::kMediaRouterTabMirroringSelected); nit: maybe kMediaRouterTabMirroringSources ? https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:173: registry->RegisterDictionaryPref(prefs::kMediaRouterTabMirroringSelected); Can this be a ListPref? https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:428: media_router_ui_->UserSelectedTabMirroringForCurrentHost(); nitty nit: indent four spaces.
https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:173: registry->RegisterDictionaryPref(prefs::kMediaRouterTabMirroringSelected); On 2016/11/16 18:51:10, apacible wrote: > nit: maybe kMediaRouterTabMirroringSources ? Changed. https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/profile... chrome/browser/profiles/profile.cc:173: registry->RegisterDictionaryPref(prefs::kMediaRouterTabMirroringSelected); On 2016/11/16 18:51:09, apacible wrote: > Can this be a ListPref? Using ListPref. https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:428: media_router_ui_->UserSelectedTabMirroringForCurrentHost(); On 2016/11/16 18:51:10, apacible wrote: > nitty nit: indent four spaces. Do you mean that L428 should be indented by four more spaces compared to L427? "git cl format" fixes that to this.
lgtm
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by takumif@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.
lgtm personal preference -- split out rebase and changes into separate patches. It makes it easier to see diffs. https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/2487673003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:428: media_router_ui_->UserSelectedTabMirroringForCurrentHost(); On 2016/11/17 01:54:51, takumif wrote: > On 2016/11/16 18:51:10, apacible wrote: > > nitty nit: indent four spaces. > > Do you mean that L428 should be indented by four more spaces compared to L427? > "git cl format" fixes that to this. Yes, but I defer to the formatting tool. I'd gotten feedback to have four more spaces to indent when I started, so that's what I've been doing.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2487673003/#ps220001 (title: "Use url::Origin for serializing origins, rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480546368445100,
"parent_rev": "ba16ca1bf754f6441e9a867533dd4cd2d4006691", "commit_rev":
"407c1f8a03a8201989a3cd1fa2a0fbc9b35e2fc1"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [Media Router] Make per-hostname cast mode selections persist This change adds a non-synced pref to record per-hostname cast mode selections made by user in a MR dialog. Whenever the user opens a MR dialog on a site with a recorded preference, the cast mode selection in the dialog will default to it. We do not record the user selecting the desktop mirroring mode, since that cast mode is not associated with the sites the user is on. BUG=664671 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e5d669a3e9782b50b35a24d2e380b577c73b7a16 Cr-Commit-Position: refs/heads/master@{#435461} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e5d669a3e9782b50b35a24d2e380b577c73b7a16 Cr-Commit-Position: refs/heads/master@{#435461}
Message was sent while issue was closed.
Patchset #8 (id:240001) has been deleted |
