Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907673002:
[Media Router] Ensure the same FeatureSwitch is used for lifetime of the (Closed)
|
Created:
4 years, 8 months ago by imcheng Modified:
4 years, 8 months ago Reviewers:
mark a. foltz CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Ensure the same FeatureSwitch is used for lifetime of the
browser.
We noticed that with Cast Extension enabled, the component action
toolbar logic will end up using the EnableMediaRouterWithCastExtension
field trial during the call to MediaRouterEnabled(). If so, the
Cast Extension will be unloaded. Code executed subsequently use
EnableMediaRouter field trial during the call to MediaRouterEnabled().
This could be a problem for users with certain field trial group
configurations and results in inconsistencies (e.g., has Cast button
in toolbar, but opens error page when clicked due to WebUI not found).
The decision is to no longer rely on the EMRWCE field trial. For one,
the mechanism to switch on BrowserContext to return the correct field
trial consistently require a complicated mechanism, and we will still
end up with some corner cases that result in strange behavior. In
addition, having both field trials make analyzing the data more
difficult.
BUG=605165
Committed: https://crrev.com/f735fb21eb9fdc1cade2b8a30217b8c5dc626b90
Cr-Commit-Position: refs/heads/master@{#389175}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Simplified experiments #Messages
Total messages: 18 (7 generated)
Patchset #1 (id:1) has been deleted
imcheng@chromium.org changed reviewers: + mfoltz@chromium.org
PTAL. This solution has its limitations, but it should fix the bug we've been seeing (verified manually).
The CQ bit was checked by imcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907673002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907673002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_router_feature.cc (right): https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_feature.cc:52: static bool has_cast_extension = HasCastExtension(context); This needs to be per-|context|, since one profile might have an enabled extension and another might not. There's also an issue where an incognito profile might not have the extension enabled, but the parent profile will. In that case, the user would see inconsistent results depending on which context was passed in (off-the-record or not). Finally, I'm not sure this solves the problem, since the extension will remain unloaded after the migration, so users could have MR enabled by the 'media_router_with_cast_extension' experiment on browser startup, then disabled on subsequent starts because they are out of the 'media_router' experiment. https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_router_feature.cc:60: #endif // defined(OS_ANDROID) There are only two #if's above, but there are three #endif's below.
On 2016/04/21 19:30:56, mark a. foltz wrote: > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > File chrome/browser/media/router/media_router_feature.cc (right): > > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > chrome/browser/media/router/media_router_feature.cc:52: static bool > has_cast_extension = HasCastExtension(context); > This needs to be per-|context|, since one profile might have an enabled > extension and another might not. > > There's also an issue where an incognito profile might not have the extension > enabled, but the parent profile will. In that case, the user would see > inconsistent results depending on which context was passed in (off-the-record or > not). > We will probably need to use KeyedService to make it per BrowserContext and have it return a consistent answer. I would like to try avoid doing that, but I am not sure if there's another way. I have also tried changing the logic to fall back to checking the component action migration pref (and not have a static variable), but it is still broken in some corner cases (e.g., user in EMR group but not in EMRWCE group, installs Cast Extension). Since the cast extension will still be available in the webstore when we roll out MR, this will not be a good user experience. > Finally, I'm not sure this solves the problem, since the extension will remain > unloaded after the migration, so users could have MR enabled by the > 'media_router_with_cast_extension' experiment on browser startup, then disabled > on subsequent starts because they are out of the 'media_router' experiment. > So the migration's effects only lasts for that runtime since the unloading of the extension is not permanent. The next time the browser is started up the migration logic will be run again. So the first time it is queried (by the component_migration_helper) in the next runtime it will still be use the MRWCE field trial. But for completeness we should also check the migration pref. > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > chrome/browser/media/router/media_router_feature.cc:60: #endif // > defined(OS_ANDROID) > There are only two #if's above, but there are three #endif's below.
On 2016/04/21 21:14:59, imcheng wrote: > On 2016/04/21 19:30:56, mark a. foltz wrote: > > > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > > File chrome/browser/media/router/media_router_feature.cc (right): > > > > > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > > chrome/browser/media/router/media_router_feature.cc:52: static bool > > has_cast_extension = HasCastExtension(context); > > This needs to be per-|context|, since one profile might have an enabled > > extension and another might not. > > > > There's also an issue where an incognito profile might not have the extension > > enabled, but the parent profile will. In that case, the user would see > > inconsistent results depending on which context was passed in (off-the-record > or > > not). > > > > We will probably need to use KeyedService to make it per BrowserContext and have > it return a consistent answer. I would like to try avoid doing that, but I am > not sure if there's another way. > > I have also tried changing the logic to fall back to checking the component > action migration pref (and not have a static variable), but it is still broken > in some corner cases (e.g., user in EMR group but not in EMRWCE group, installs > Cast Extension). Since the cast extension will still be available in the > webstore when we roll out MR, this will not be a good user experience. > > Another way is to add a map from BrowserContext to a bool in MediaRouterFactory. It will work as follows: When MediaRouterEnabled() is called, it will: (1) query MediaRouterFactory to see if a bool value already exists for the given BrowserContext. (2a) If yes, return that value. (2b) If no, compute the value using enabled extension set + migration pref, store that value in MediaRouterFactory, and return that value. The key-value pair will be erased on BrowserContextShutdown, which MediaRouterFactory already overrides. WDYT? > > Finally, I'm not sure this solves the problem, since the extension will remain > > unloaded after the migration, so users could have MR enabled by the > > 'media_router_with_cast_extension' experiment on browser startup, then > disabled > > on subsequent starts because they are out of the 'media_router' experiment. > > > > So the migration's effects only lasts for that runtime since the unloading of > the extension is not permanent. The next time the browser is started up the > migration logic will be run again. So the first time it is queried (by the > component_migration_helper) in the next runtime it will still be use the MRWCE > field trial. But for completeness we should also check the migration pref. > > > > https://codereview.chromium.org/1907673002/diff/20001/chrome/browser/media/ro... > > chrome/browser/media/router/media_router_feature.cc:60: #endif // > > defined(OS_ANDROID) > > There are only two #if's above, but there are three #endif's below.
Description was changed from ========== [Media Router] Ensure the same FeatureSwitch is used for lifetime of the browser. We noticed that with Cast Extension enabled, the component action toolbar logic will end up using the EnableMediaRouterWithCastExtension field trial during the call to MediaRouterEnabled(). If so, the Cast Extension will be unloaded. Code executed subsequently use EnableMediaRouter field trial during the call to MediaRouterEnabled(). This could be a problem for users with certain field trial group configurations and results in inconsistencies (e.g., has Cast button in toolbar, but opens error page when clicked due to WebUI not found). The solution here is to ensure the same field trial / FeatureSwitch is queried for all calls to MediaRouterEnabled() for the lifetime of the browser. The caveat is we only use the BrowserContext the function is first called with to determine which field trial to use for the lifetime of the browser. BUG=605165 ========== to ========== [Media Router] Ensure the same FeatureSwitch is used for lifetime of the browser. We noticed that with Cast Extension enabled, the component action toolbar logic will end up using the EnableMediaRouterWithCastExtension field trial during the call to MediaRouterEnabled(). If so, the Cast Extension will be unloaded. Code executed subsequently use EnableMediaRouter field trial during the call to MediaRouterEnabled(). This could be a problem for users with certain field trial group configurations and results in inconsistencies (e.g., has Cast button in toolbar, but opens error page when clicked due to WebUI not found). The decision is to no longer rely on the EMRWCE field trial. For one, the mechanism to switch on BrowserContext to return the correct field trial consistently require a complicated mechanism, and we will still end up with some corner cases that result in strange behavior. In addition, having both field trials make analyzing the data more difficult. BUG=605165 ==========
I have updated the patch per the conversation yesterday. PTAL.
lgtm
The CQ bit was checked by imcheng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907673002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907673002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Ensure the same FeatureSwitch is used for lifetime of the browser. We noticed that with Cast Extension enabled, the component action toolbar logic will end up using the EnableMediaRouterWithCastExtension field trial during the call to MediaRouterEnabled(). If so, the Cast Extension will be unloaded. Code executed subsequently use EnableMediaRouter field trial during the call to MediaRouterEnabled(). This could be a problem for users with certain field trial group configurations and results in inconsistencies (e.g., has Cast button in toolbar, but opens error page when clicked due to WebUI not found). The decision is to no longer rely on the EMRWCE field trial. For one, the mechanism to switch on BrowserContext to return the correct field trial consistently require a complicated mechanism, and we will still end up with some corner cases that result in strange behavior. In addition, having both field trials make analyzing the data more difficult. BUG=605165 ========== to ========== [Media Router] Ensure the same FeatureSwitch is used for lifetime of the browser. We noticed that with Cast Extension enabled, the component action toolbar logic will end up using the EnableMediaRouterWithCastExtension field trial during the call to MediaRouterEnabled(). If so, the Cast Extension will be unloaded. Code executed subsequently use EnableMediaRouter field trial during the call to MediaRouterEnabled(). This could be a problem for users with certain field trial group configurations and results in inconsistencies (e.g., has Cast button in toolbar, but opens error page when clicked due to WebUI not found). The decision is to no longer rely on the EMRWCE field trial. For one, the mechanism to switch on BrowserContext to return the correct field trial consistently require a complicated mechanism, and we will still end up with some corner cases that result in strange behavior. In addition, having both field trials make analyzing the data more difficult. BUG=605165 Committed: https://crrev.com/f735fb21eb9fdc1cade2b8a30217b8c5dc626b90 Cr-Commit-Position: refs/heads/master@{#389175} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f735fb21eb9fdc1cade2b8a30217b8c5dc626b90 Cr-Commit-Position: refs/heads/master@{#389175} |
