Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517833004:
[MR] Cancel auto-switching from tab mirroring to Cast SDK at page navigation based on user pref (Closed)
Description[MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref
This change makes PresentationServiceDelegateImpl cancel requests to join
session if they have the "auto-join" presentation ID and the user pref is set to
tab mirroring for the origin. This way, when a page requests to auto-switch from
tab mirroring to Cast SDK on a site where the user has chosen to use tab
mirroring, the auto-switching gets cancelled.
We also change the settings for prefs::kMediaRouterTabMirroringSources, so that
cast mode selections made in incognito last only for the lifetime of the
incognito profile.
This CL relies on a profile pref introduced by
https://codereview.chromium.org/2487673003/.
BUG=664671
Committed: https://crrev.com/f5da68f68b47ec803e5e9da660c885afb6713efe
Cr-Commit-Position: refs/heads/master@{#437699}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase #Patch Set 3 : Address Mark's comments #
Total comments: 14
Patch Set 4 : Rebase #Patch Set 5 : Address Derek and Mark's comments #Patch Set 6 : Add checks for OS_ANDROID #
Total comments: 8
Patch Set 7 : Address Jennifer and Mark's comments #Patch Set 8 : Make incognito pref non-persistent #Patch Set 9 : Rebase #
Total comments: 10
Patch Set 10 : Address Mark and Jennifer's comments #
Messages
Total messages: 45 (24 generated)
Description was changed from ========== . BUG= ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref BUG=664671 ==========
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref BUG=664671 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. BUG=664671 ==========
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. BUG=664671 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. BUG=664671 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. BUG=664671 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on profile prefs introduced by https://codereview.chromium.org/2487673003/. BUG=664671 ==========
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on profile prefs introduced by https://codereview.chromium.org/2487673003/. BUG=664671 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 ==========
takumif@chromium.org changed reviewers: + apacible@chromium.org, imcheng@chromium.org, mfoltz@chromium.org
This CL implements tab-mirroring override in PresentationServiceDelegateImpl. It depends on the other sticky cast mode CL: https://codereview.chromium.org/2487673003/
A couple of comments. Unit tests as well please :) https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:49: const char kAutoJoinPresentationId[] = "auto-join"; Slight preference for moving the logic for detecting auto-join into media_source_helpers.h https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:831: GURL origin = GetLastCommittedURLForFrame( We should use url::Origin for origins going forward instead of GURL. https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:989: const base::ListValue* hostnames = What if hostnames is not set? https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:990: Profile::FromBrowserContext(web_contents_->GetBrowserContext()) What if this is an incognito window? https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:992: ->GetList(prefs::kMediaRouterTabMirroringSources); How expensive is looking up this pref? Can it change over the lifetime of PSDImpl?
https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:992: ->GetList(prefs::kMediaRouterTabMirroringSources); On 2016/11/22 22:23:21, mark a. foltz wrote: > How expensive is looking up this pref? Can it change over the lifetime of > PSDImpl? I'm not sure how expensive this is. It's a disk read, right? If we want to reduce pref lookups I think we can move this functionality to MRMojoImpl, since that has a longer lifespan than PSDImpl, and make it cache the pref value (and become a pref observer and update the cached value when it changes). What do you think?
https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:992: ->GetList(prefs::kMediaRouterTabMirroringSources); On 2016/11/29 at 23:56:45, takumif wrote: > On 2016/11/22 22:23:21, mark a. foltz wrote: > > How expensive is looking up this pref? Can it change over the lifetime of > > PSDImpl? > > I'm not sure how expensive this is. It's a disk read, right? > If we want to reduce pref lookups I think we can move this functionality to MRMojoImpl, since that has a longer lifespan than PSDImpl, and make it cache the pref value (and become a pref observer and update the cached value when it changes). What do you think? It appears unlikely to cause a disk read. But it is hard to tell because there are many implementations of the preference store. My advice: If the pref value can't change outside of this class, I would cache it. If it can, then keep the code as-is.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:49: const char kAutoJoinPresentationId[] = "auto-join"; On 2016/11/22 22:23:21, mark a. foltz wrote: > Slight preference for moving the logic for detecting auto-join into > media_source_helpers.h Done. https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:831: GURL origin = GetLastCommittedURLForFrame( On 2016/11/22 22:23:21, mark a. foltz wrote: > We should use url::Origin for origins going forward instead of GURL. Done. https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:989: const base::ListValue* hostnames = On 2016/11/22 22:23:21, mark a. foltz wrote: > What if hostnames is not set? Adding a check for that. https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:990: Profile::FromBrowserContext(web_contents_->GetBrowserContext()) On 2016/11/22 22:23:21, mark a. foltz wrote: > What if this is an incognito window? I think we should ignore the preferences (and don't modify them) in guest/incognito modes. Changing. https://codereview.chromium.org/2517833004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:992: ->GetList(prefs::kMediaRouterTabMirroringSources); On 2016/12/01 00:04:34, mark a. foltz wrote: > On 2016/11/29 at 23:56:45, takumif wrote: > > On 2016/11/22 22:23:21, mark a. foltz wrote: > > > How expensive is looking up this pref? Can it change over the lifetime of > > > PSDImpl? > > > > I'm not sure how expensive this is. It's a disk read, right? > > If we want to reduce pref lookups I think we can move this functionality to > MRMojoImpl, since that has a longer lifespan than PSDImpl, and make it cache the > pref value (and become a pref observer and update the cached value when it > changes). What do you think? > > It appears unlikely to cause a disk read. But it is hard to tell because there > are many implementations of the preference store. > > My advice: If the pref value can't change outside of this class, I would cache > it. If it can, then keep the code as-is. Got it. Keeping it as is, as MediaRouterUI changes the pref.
lgtm https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:986: Profile* profile = would it be sufficient to just check IsOffTheRecord(), which is true for both guest and incognito profiles? https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:476: TEST_F(PresentationServiceDelegateImplTest, AutoJoinRequest) { Consider adding a PresentationServiceDelegateImplIncognitoTest case.
https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.cc:28: const char kAutoJoinPresentationId[] = "auto-join"; constexpr char https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:830: url::Origin origin = url::Origin(GetLastCommittedURLForFrame( You'l need to rebase and pass |web_contents_| here for the crasher fix. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:837: "Auto-join request overridden by user preferences.")); s/overridden/cancelled/ https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:988: // If this is a guest or incognito profile, ignore the preferences. Why are we ignoring the preference for incognito profiles? https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:202: bool ShouldOverrideAutoJoinForOrigin(const url::Origin& origin) const; ShouldCancelAutoJoinForOrigin?
Thanks for reviewing! https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_source_helper.cc:28: const char kAutoJoinPresentationId[] = "auto-join"; On 2016/12/05 21:32:21, mark a. foltz wrote: > constexpr char Done. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:830: url::Origin origin = url::Origin(GetLastCommittedURLForFrame( On 2016/12/05 21:32:21, mark a. foltz wrote: > You'l need to rebase and pass |web_contents_| here for the crasher fix. Done. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:837: "Auto-join request overridden by user preferences.")); On 2016/12/05 21:32:22, mark a. foltz wrote: > s/overridden/cancelled/ Done. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:986: Profile* profile = On 2016/12/05 20:12:33, imcheng wrote: > would it be sufficient to just check IsOffTheRecord(), which is true for both > guest and incognito profiles? I think so. Changing. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.cc:988: // If this is a guest or incognito profile, ignore the preferences. On 2016/12/05 21:32:21, mark a. foltz wrote: > Why are we ignoring the preference for incognito profiles? I thought that not reading/writing user pref would be the expected behavior in guest/incognito modes. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl.h (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl.h:202: bool ShouldOverrideAutoJoinForOrigin(const url::Origin& origin) const; On 2016/12/05 21:32:22, mark a. foltz wrote: > ShouldCancelAutoJoinForOrigin? Changed. https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2517833004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:476: TEST_F(PresentationServiceDelegateImplTest, AutoJoinRequest) { On 2016/12/05 20:12:33, imcheng wrote: > Consider adding a PresentationServiceDelegateImplIncognitoTest case. Added.
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...
https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:28: #if !defined(OS_ANDROID) nit: move all #if !defined(OS_ANDROID) includes to below the general list of includes. https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:828: void PresentationServiceDelegateImpl::JoinSession( Remind me -- when does JoinSession get called? Is it on navigate?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:842: url::Origin origin = url::Origin(GetLastCommittedURLForFrame( Use a const ref https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:1003: // If this is a guest or incognito profile, ignore the preferences. The design doc describes how to override prefs in an incognito profile. https://www.chromium.org/developers/design-documents/preferences#TOC-Incognit... If they are backed by the user_pref store, the incognito value will be held in memory for the lifetime of the profile. That way the incognito profile will be able to support the mirroring override pref.
https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:28: #if !defined(OS_ANDROID) On 2016/12/07 00:53:09, apacible wrote: > nit: move all #if !defined(OS_ANDROID) includes to below the general list of > includes. Done. https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:828: void PresentationServiceDelegateImpl::JoinSession( On 2016/12/07 00:53:09, apacible wrote: > Remind me -- when does JoinSession get called? Is it on navigate? It's called with "auto-join" presentation ID when user navigates to a Cast SDK site while tab mirroring. https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:842: url::Origin origin = url::Origin(GetLastCommittedURLForFrame( On 2016/12/07 04:19:01, mark a. foltz wrote: > Use a const ref Done. https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:1003: // If this is a guest or incognito profile, ignore the preferences. On 2016/12/07 04:19:01, mark a. foltz wrote: > The design doc describes how to override prefs in an incognito profile. > > https://www.chromium.org/developers/design-documents/preferences#TOC-Incognit... > > If they are backed by the user_pref store, the incognito value will be held in > memory for the lifetime of the profile. > > That way the incognito profile will be able to support the mirroring override > pref. We are using user_prefs (checked by calling PrefService::Preference::IsUserControlled()), and an incognito PrefService does get used (Profile::GetOffTheRecordPrefs() gets called), but pref changes made in incognito do persist (checked by killing and restarting Chrome). I haven't been able to figure out why this is the case.
On 2016/12/08 at 00:01:30, takumif wrote: > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > chrome/browser/media/router/presentation_service_delegate_impl.cc:28: #if !defined(OS_ANDROID) > On 2016/12/07 00:53:09, apacible wrote: > > nit: move all #if !defined(OS_ANDROID) includes to below the general list of > > includes. > > Done. > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > chrome/browser/media/router/presentation_service_delegate_impl.cc:828: void PresentationServiceDelegateImpl::JoinSession( > On 2016/12/07 00:53:09, apacible wrote: > > Remind me -- when does JoinSession get called? Is it on navigate? > > It's called with "auto-join" presentation ID when user navigates to a Cast SDK site while tab mirroring. > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > chrome/browser/media/router/presentation_service_delegate_impl.cc:842: url::Origin origin = url::Origin(GetLastCommittedURLForFrame( > On 2016/12/07 04:19:01, mark a. foltz wrote: > > Use a const ref > > Done. > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > chrome/browser/media/router/presentation_service_delegate_impl.cc:1003: // If this is a guest or incognito profile, ignore the preferences. > On 2016/12/07 04:19:01, mark a. foltz wrote: > > The design doc describes how to override prefs in an incognito profile. > > > > https://www.chromium.org/developers/design-documents/preferences#TOC-Incognit... > > > > If they are backed by the user_pref store, the incognito value will be held in > > memory for the lifetime of the profile. > > > > That way the incognito profile will be able to support the mirroring override > > pref. > > We are using user_prefs (checked by calling PrefService::Preference::IsUserControlled()), and an incognito PrefService does get used (Profile::GetOffTheRecordPrefs() gets called), but pref changes made in incognito do persist (checked by killing and restarting Chrome). I haven't been able to figure out why this is the case. That would be a possibly-serious bug in the Incognito implementation for preferences. Can you reproduce it with an already-used preference and file it?
Patchset #8 (id:180001) has been deleted
On 2016/12/08 01:16:15, mark a. foltz wrote: > On 2016/12/08 at 00:01:30, takumif wrote: > > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > > File chrome/browser/media/router/presentation_service_delegate_impl.cc > (right): > > > > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > > chrome/browser/media/router/presentation_service_delegate_impl.cc:28: #if > !defined(OS_ANDROID) > > On 2016/12/07 00:53:09, apacible wrote: > > > nit: move all #if !defined(OS_ANDROID) includes to below the general list of > > > includes. > > > > Done. > > > > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > > chrome/browser/media/router/presentation_service_delegate_impl.cc:828: void > PresentationServiceDelegateImpl::JoinSession( > > On 2016/12/07 00:53:09, apacible wrote: > > > Remind me -- when does JoinSession get called? Is it on navigate? > > > > It's called with "auto-join" presentation ID when user navigates to a Cast SDK > site while tab mirroring. > > > > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > > chrome/browser/media/router/presentation_service_delegate_impl.cc:842: > url::Origin origin = url::Origin(GetLastCommittedURLForFrame( > > On 2016/12/07 04:19:01, mark a. foltz wrote: > > > Use a const ref > > > > Done. > > > > > https://codereview.chromium.org/2517833004/diff/140001/chrome/browser/media/r... > > chrome/browser/media/router/presentation_service_delegate_impl.cc:1003: // If > this is a guest or incognito profile, ignore the preferences. > > On 2016/12/07 04:19:01, mark a. foltz wrote: > > > The design doc describes how to override prefs in an incognito profile. > > > > > > > https://www.chromium.org/developers/design-documents/preferences#TOC-Incognit... > > > > > > If they are backed by the user_pref store, the incognito value will be held > in > > > memory for the lifetime of the profile. > > > > > > That way the incognito profile will be able to support the mirroring > override > > > pref. > > > > We are using user_prefs (checked by calling > PrefService::Preference::IsUserControlled()), and an incognito PrefService does > get used (Profile::GetOffTheRecordPrefs() gets called), but pref changes made in > incognito do persist (checked by killing and restarting Chrome). I haven't been > able to figure out why this is the case. > > That would be a possibly-serious bug in the Incognito implementation for > preferences. Can you reproduce it with an already-used preference and file it? It turns out that we must explicitly add the pref to a list in pref_service_syncable_util.cc to make it non-persistent in incognito. Now the changes made in incognito don't affect the normal profile, and they disappear with the incognito profile.
takumif@chromium.org changed reviewers: + gab@chromium.org
gab@: please take a look at pref_service_syncable_util.cc Thanks!
On 2016/12/08 19:59:58, takumif wrote: > gab@: please take a look at pref_service_syncable_util.cc > Thanks! lgtm
lgtm Thanks for tracking down the fix on the incognito pref. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:838: RenderFrameHostId(render_process_id, render_frame_id), web_contents_)); There might be a slight issue with this approach: if you call this for a frame that's not considered a "live" frame for the RenderFrameHost it will return an empty origin. (Otherwise this call can crash.) I don't think this will be a major issue though; I'm going to open up a thread with some folks on how to detect frame origins more reliably. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:488: int routing_id = main_frame->GetRoutingID(); A lot of this setup code seems to be duplicated between tests; it would be a good cleanup to declare some of these values in the base class. However, not necessary to fix in this patch. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:499: update.reset(); Instead of manually resetting the unique_ptr, slightly cleaner to scope the declaration in a { } block.
lgtm https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:69: // compare host, port, scheme, and path prefix for source.url() nit: capitalize "compare" and period at the end of comment. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:70: if (!source.url().SchemeIs(url::kHttpsScheme) || Personal preference to just have this function just have a return statement (rather than if() return x, return y), but up to you.
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Thanks for reviewing! https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/media_source_helper.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:69: // compare host, port, scheme, and path prefix for source.url() On 2016/12/09 18:04:09, apacible wrote: > nit: capitalize "compare" and period at the end of comment. Done. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/media_source_helper.cc:70: if (!source.url().SchemeIs(url::kHttpsScheme) || On 2016/12/09 18:04:09, apacible wrote: > Personal preference to just have this function just have a return statement > (rather than if() return x, return y), but up to you. Changed. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl.cc:838: RenderFrameHostId(render_process_id, render_frame_id), web_contents_)); On 2016/12/09 04:07:42, mark a. foltz wrote: > There might be a slight issue with this approach: if you call this for a frame > that's not considered a "live" frame for the RenderFrameHost it will return an > empty origin. (Otherwise this call can crash.) > > I don't think this will be a major issue though; I'm going to open up a thread > with some folks on how to detect frame origins more reliably. > Acknowledged. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc (right): https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:488: int routing_id = main_frame->GetRoutingID(); On 2016/12/09 04:07:42, mark a. foltz wrote: > A lot of this setup code seems to be duplicated between tests; it would be a > good cleanup to declare some of these values in the base class. However, not > necessary to fix in this patch. I'll do that in a separate patch. https://codereview.chromium.org/2517833004/diff/220001/chrome/browser/media/r... chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:499: update.reset(); On 2016/12/09 04:07:42, mark a. foltz wrote: > Instead of manually resetting the unique_ptr, slightly cleaner to scope the > declaration in a { } block. Done.
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 takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, gab@chromium.org, mfoltz@chromium.org, apacible@chromium.org Link to the patchset: https://codereview.chromium.org/2517833004/#ps240001 (title: "Address Mark and Jennifer's comments")
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": 240001, "attempt_start_ts": 1481323715644390,
"parent_rev": "d0f2351e2cb9a8d68fe7082a036d2ac9fd914cde", "commit_rev":
"72c92d60b36db4834aace9a883f78046cfdd1855"}
Message was sent while issue was closed.
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 Review-Url: https://codereview.chromium.org/2517833004 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 Review-Url: https://codereview.chromium.org/2517833004 ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 Committed: https://crrev.com/f5da68f68b47ec803e5e9da660c885afb6713efe Cr-Commit-Position: refs/heads/master@{#437699} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f5da68f68b47ec803e5e9da660c885afb6713efe Cr-Commit-Position: refs/heads/master@{#437699}
Message was sent while issue was closed.
Description was changed from ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 Committed: https://crrev.com/f5da68f68b47ec803e5e9da660c885afb6713efe Cr-Commit-Position: refs/heads/master@{#437699} ========== to ========== [MR] Cancel auto-switching from tab mirroring to Cast SDK based on user pref This change makes PresentationServiceDelegateImpl cancel requests to join session if they have the "auto-join" presentation ID and the user pref is set to tab mirroring for the origin. This way, when a page requests to auto-switch from tab mirroring to Cast SDK on a site where the user has chosen to use tab mirroring, the auto-switching gets cancelled. We also change the settings for prefs::kMediaRouterTabMirroringSources, so that cast mode selections made in incognito last only for the lifetime of the incognito profile. This CL relies on a profile pref introduced by https://codereview.chromium.org/2487673003/. BUG=664671 Committed: https://crrev.com/f5da68f68b47ec803e5e9da660c885afb6713efe Cr-Commit-Position: refs/heads/master@{#437699} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
