Issue 1415333002: [Media Router] Add experiment control logic.

Issue 1415333002: [Media Router] Add experiment control logic. (Closed)

Created:
5 years, 2 months ago by imcheng

Modified:
5 years, 1 month ago

Reviewers:
mark a. foltz, Devlin, nikunjb, lazyboy, msw

CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_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.

More Reviews

Description

[Media Router] Add experiment control logic. Moved MediaRouterEnabled() from chrome_switches to media_router_feature. For desktop, the new function will use a FeatureSwitch integrated with an experiment "EnableMediaRouter" to determine whether media router should be enabled. Properly wrap MediaRouterEnabled() calls with #if define()s. The FeatureSwitch use the regular switch ("enable-media-router") first, then use experiment if switch is not present. The default behavior is to disable media router. (See feature_switch.h for details on precedence). BUG=541315 Committed: https://crrev.com/aa022abfc5393ba0f2f030228070504ffd2d432a Cr-Commit-Position: refs/heads/master@{#357003}

Patch Set 1 #

Patch Set 2 : Change RegisterChromeServicesForFrame, use ScopedOverride in tests #

Patch Set 3 : #

Total comments: 33

Patch Set 4 : Addressed msw@'s comments #

Total comments: 4

Patch Set 5 : Addressed comments #11 #

Patch Set 6 : Moved MediaRouterEnabled back to chrome_switches, added/removed some #if defineds #

Patch Set 7 : add dependencies from /chrome/common:constants to //extensions/common:common #

Patch Set 8 : #

Patch Set 9 : moved back to MediaRouterFeature but made it available even if MR is disabled #

Patch Set 10 : Rebase #

Patch Set 11 : fix macros #

Patch Set 12 : Add one more #if defined guard, investigate / fix mac failure in separate patch #

Patch Set 13 : #

Patch Set 14 : #

Total comments: 10

Patch Set 15 : Addressed msw's comments #

Patch Set 16 : Rebase #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+119 lines, -69 lines)			Patch
M	chrome/browser/chrome_content_browser_client.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/chrome_mojo_service_registration.cc	View	1 2 3 6 7 8	2 chunks	+2 lines, -5 lines	0 comments	Download
M	chrome/browser/extensions/external_component_loader.cc	View	1 2 3 6 7 8	3 chunks	+5 lines, -2 lines	0 comments	Download
A	chrome/browser/media/router/media_router_feature.h	View	6 7 8	1 chunk	+15 lines, -0 lines	0 comments	Download
A	chrome/browser/media/router/media_router_feature.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+29 lines, -0 lines	0 comments	Download
M	chrome/browser/renderer_context_menu/render_view_context_menu.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	6 chunks	+13 lines, -9 lines	0 comments	Download
M	chrome/browser/ui/browser_commands.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	6 chunks	+9 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/toolbar/app_menu_model.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+5 lines, -1 line	0 comments	Download
M	chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc	View	6 7 8	2 chunks	+2 lines, -1 line	0 comments	Download
M	chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	3 chunks	+4 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/webui/media_router/media_router_test.h	View	1 2 3 4	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/webui/media_router/media_router_test.cc	View	1 2 3 4	1 chunk	+2 lines, -10 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/common/chrome_switches.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/common/chrome_switches.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+0 lines, -15 lines	0 comments	Download
M	chrome/test/media_router/media_router_base_browsertest.h	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/test/media_router/media_router_base_browsertest.cc	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	extensions/common/feature_switch.h	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	extensions/common/feature_switch.cc	View	1 2 3 4 5	5 chunks	+13 lines, -6 lines	0 comments	Download

Messages

Total messages: 35 (8 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

imcheng

Mark: PTAL. Once it looks ok, I will add owners for the respective files for ...

5 years, 2 months ago (2015-10-22 01:46:34 UTC) #2

mark a. foltz

On 2015/10/22 at 01:46:34, imcheng wrote: > Mark: PTAL. Once it looks ok, I will ...

5 years, 2 months ago (2015-10-22 16:50:09 UTC) #3

imcheng

On 2015/10/22 16:50:09, mark a. foltz wrote: > On 2015/10/22 at 01:46:34, imcheng wrote: > ...

5 years, 2 months ago (2015-10-22 19:55:45 UTC) #5

imcheng

Adding the following reviewers: msw: chrome/browser/ui/browser_commands.cc chrome/browser/ui/browser_commands.h chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc chrome/browser/ui/toolbar/wrench_menu_model.cc chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc rdevlin.cronin: chrome/browser/extensions/chrome_mojo_service_registration.cc chrome/browser/extensions/external_component_loader.cc chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc extensions/common/feature_switch.cc ...

5 years, 2 months ago (2015-10-22 23:35:19 UTC) #8

msw

https://codereview.chromium.org/1415333002/diff/40001/chrome/browser/extensions/chrome_mojo_service_registration.cc File chrome/browser/extensions/chrome_mojo_service_registration.cc (right): https://codereview.chromium.org/1415333002/diff/40001/chrome/browser/extensions/chrome_mojo_service_registration.cc#newcode8 chrome/browser/extensions/chrome_mojo_service_registration.cc:8: #include "base/command_line.h" nit: remove https://codereview.chromium.org/1415333002/diff/40001/chrome/browser/extensions/chrome_mojo_service_registration.cc#newcode10 chrome/browser/extensions/chrome_mojo_service_registration.cc:10: #include "chrome/common/chrome_switches.h" nit: ...

5 years, 1 month ago (2015-10-23 20:18:43 UTC) #9

imcheng

5 years, 1 month ago (2015-10-23 21:39:27 UTC) #10

msw

lgtm with minor nits https://codereview.chromium.org/1415333002/diff/40001/chrome/browser/extensions/external_component_loader.cc File chrome/browser/extensions/external_component_loader.cc (right): https://codereview.chromium.org/1415333002/diff/40001/chrome/browser/extensions/external_component_loader.cc#newcode56 chrome/browser/extensions/external_component_loader.cc:56: #if defined(ENABLE_MEDIA_ROUTER) && defined(GOOGLE_CHROME_BUILD) On ...

5 years, 1 month ago (2015-10-23 22:30:49 UTC) #11

imcheng

https://codereview.chromium.org/1415333002/diff/60001/chrome/browser/ui/webui/media_router/media_router_test.cc File chrome/browser/ui/webui/media_router/media_router_test.cc (right): https://codereview.chromium.org/1415333002/diff/60001/chrome/browser/ui/webui/media_router/media_router_test.cc#newcode15 chrome/browser/ui/webui/media_router/media_router_test.cc:15: void MediaRouterTest::SetUp() { On 2015/10/23 22:30:49, msw wrote: > ...

5 years, 1 month ago (2015-10-23 22:37:17 UTC) #12

Devlin

I share Mike's regret about the sheer number of #defines we have in this patch ...

5 years, 1 month ago (2015-10-23 23:46:50 UTC) #13

imcheng

On 2015/10/23 23:46:50, Devlin wrote: > I share Mike's regret about the sheer number of ...

5 years, 1 month ago (2015-10-24 00:33:07 UTC) #14

Devlin

On 2015/10/24 00:33:07, imcheng1 wrote: > On 2015/10/23 23:46:50, Devlin wrote: > > I share ...

5 years, 1 month ago (2015-10-24 00:41:50 UTC) #15

On 2015/10/24 00:33:07, imcheng1 wrote:
> On 2015/10/23 23:46:50, Devlin wrote:
> > I share Mike's regret about the sheer number of #defines we have in this
patch
> > (and in MEDIA_ROUTER in general).  I appreciate that we want to reduce the
> > amount of compiling code, but would it be so bad to have a shared
> > MediaRouterEnabled() function (across platforms) that then has all of the
> > #defines handled internally?  It seems like it would greatly reduce the
> > complexity of this patch, and media router code in general...
> 
> I have thought about this before. If we really want MediaRouterEnabled() to
> handle #defines internally, then we can make that happen, but unless I am
> missing something, it's not straightforward either. I can think of couple of
> options:
> - make media_router_features.{cc,h} available to all builds. Currently
> chrome/browser/media/router code is only built when MEDIA_ROUTER is defined.
So
> we have to make an exception to that rule.
> - move MediaRouterEnabled() to somewhere that can be used by all platforms.
> Before this CL, it's in chrome/common/chrome_switches.{cc,h}. So we could move
> it to a file called chrome/common/feature_switches.{cc,h}. But why is
> feature_switch.{h,cc} defined in extensions/?
> 
> Any suggestions?

feature_switch is defined in extensions because, well, it primarily concerns
extension features. :)  I'd be hesitant to move it just yet.

Out of curiosity, why doesn't the following work:
Leave MediaRouterEnabled in chrome_switches
Have the body of MediaRouterEnabled be something like
#if defined(MEDIA_ROUTER_ENABLED)
#if defined(ANDROID)
return true;
#else
// There's no cases of media router being enabled on non-android without
extensions, right?
return extensions::FeatureSwitch::media_router()->IsEnabled();
#endif
#else  // (!)MEDIA_ROUTER_ENABLED
#return false
#endif  // MEDIA_ROUTER_ENABLED

Seems like that would eliminate most of the #defines, while keeping the
compilable code small, and keeping the same logic?

imcheng

On 2015/10/24 00:41:50, Devlin wrote: > On 2015/10/24 00:33:07, imcheng1 wrote: > > On 2015/10/23 ...

5 years, 1 month ago (2015-10-24 00:53:03 UTC) #16

On 2015/10/24 00:41:50, Devlin wrote:
> On 2015/10/24 00:33:07, imcheng1 wrote:
> > On 2015/10/23 23:46:50, Devlin wrote:
> > > I share Mike's regret about the sheer number of #defines we have in this
> patch
> > > (and in MEDIA_ROUTER in general).  I appreciate that we want to reduce the
> > > amount of compiling code, but would it be so bad to have a shared
> > > MediaRouterEnabled() function (across platforms) that then has all of the
> > > #defines handled internally?  It seems like it would greatly reduce the
> > > complexity of this patch, and media router code in general...
> > 
> > I have thought about this before. If we really want MediaRouterEnabled() to
> > handle #defines internally, then we can make that happen, but unless I am
> > missing something, it's not straightforward either. I can think of couple of
> > options:
> > - make media_router_features.{cc,h} available to all builds. Currently
> > chrome/browser/media/router code is only built when MEDIA_ROUTER is defined.
> So
> > we have to make an exception to that rule.
> > - move MediaRouterEnabled() to somewhere that can be used by all platforms.
> > Before this CL, it's in chrome/common/chrome_switches.{cc,h}. So we could
move
> > it to a file called chrome/common/feature_switches.{cc,h}. But why is
> > feature_switch.{h,cc} defined in extensions/?
> > 
> > Any suggestions?
> 
> feature_switch is defined in extensions because, well, it primarily concerns
> extension features. :)  I'd be hesitant to move it just yet.
> 
> Out of curiosity, why doesn't the following work:
> Leave MediaRouterEnabled in chrome_switches
> Have the body of MediaRouterEnabled be something like
> #if defined(MEDIA_ROUTER_ENABLED)
> #if defined(ANDROID)
> return true;
> #else
> // There's no cases of media router being enabled on non-android without
> extensions, right?
> return extensions::FeatureSwitch::media_router()->IsEnabled();
> #endif
> #else  // (!)MEDIA_ROUTER_ENABLED
> #return false
> #endif  // MEDIA_ROUTER_ENABLED
> 
> 
> Seems like that would eliminate most of the #defines, while keeping the
> compilable code small, and keeping the same logic?

That did come across my mind. It just feels a bit strange to have it in
chrome_switches.{cc,h} since it does not directly query the switch. Which is why
I suggested adding another file for that purposes.

Devlin

On 2015/10/24 00:53:03, imcheng1 wrote: > On 2015/10/24 00:41:50, Devlin wrote: > > On 2015/10/24 ...

5 years, 1 month ago (2015-10-24 01:23:09 UTC) #17

On 2015/10/24 00:53:03, imcheng1 wrote:
> On 2015/10/24 00:41:50, Devlin wrote:
> > On 2015/10/24 00:33:07, imcheng1 wrote:
> > > On 2015/10/23 23:46:50, Devlin wrote:
> > > > I share Mike's regret about the sheer number of #defines we have in this
> > patch
> > > > (and in MEDIA_ROUTER in general).  I appreciate that we want to reduce
the
> > > > amount of compiling code, but would it be so bad to have a shared
> > > > MediaRouterEnabled() function (across platforms) that then has all of
the
> > > > #defines handled internally?  It seems like it would greatly reduce the
> > > > complexity of this patch, and media router code in general...
> > > 
> > > I have thought about this before. If we really want MediaRouterEnabled()
to
> > > handle #defines internally, then we can make that happen, but unless I am
> > > missing something, it's not straightforward either. I can think of couple
of
> > > options:
> > > - make media_router_features.{cc,h} available to all builds. Currently
> > > chrome/browser/media/router code is only built when MEDIA_ROUTER is
defined.
> > So
> > > we have to make an exception to that rule.
> > > - move MediaRouterEnabled() to somewhere that can be used by all
platforms.
> > > Before this CL, it's in chrome/common/chrome_switches.{cc,h}. So we could
> move
> > > it to a file called chrome/common/feature_switches.{cc,h}. But why is
> > > feature_switch.{h,cc} defined in extensions/?
> > > 
> > > Any suggestions?
> > 
> > feature_switch is defined in extensions because, well, it primarily concerns
> > extension features. :)  I'd be hesitant to move it just yet.
> > 
> > Out of curiosity, why doesn't the following work:
> > Leave MediaRouterEnabled in chrome_switches
> > Have the body of MediaRouterEnabled be something like
> > #if defined(MEDIA_ROUTER_ENABLED)
> > #if defined(ANDROID)
> > return true;
> > #else
> > // There's no cases of media router being enabled on non-android without
> > extensions, right?
> > return extensions::FeatureSwitch::media_router()->IsEnabled();
> > #endif
> > #else  // (!)MEDIA_ROUTER_ENABLED
> > #return false
> > #endif  // MEDIA_ROUTER_ENABLED
> > 
> > 
> > Seems like that would eliminate most of the #defines, while keeping the
> > compilable code small, and keeping the same logic?
> 
> That did come across my mind. It just feels a bit strange to have it in
> chrome_switches.{cc,h} since it does not directly query the switch. Which is
why
> I suggested adding another file for that purposes.

But right now, *all* that file does is query the switch, which seems a bit
excessive.  I think I'd rather just have the chrome file query it, and eliminate
a bunch of extra ifdefs.  WDYT?

msw

On 2015/10/24 01:23:09, Devlin wrote: > But right now, *all* that file does is query ...

5 years, 1 month ago (2015-10-26 17:00:07 UTC) #18

imcheng

Thanks for the comments. I agree that it helps reduce number of #if defined and ...

5 years, 1 month ago (2015-10-26 18:09:01 UTC) #20

imcheng

The patch ran into the following error: ERROR at //chrome/common/chrome_switches.cc:11:11: Include not allowed. #include "extensions/common/feature_switch.h" ...

5 years, 1 month ago (2015-10-26 18:21:25 UTC) #21

Devlin

On 2015/10/26 18:21:25, imcheng1 wrote: > The patch ran into the following error: > > ...

5 years, 1 month ago (2015-10-26 19:05:45 UTC) #22

imcheng

Chatted with Devlin. The issue with putting it in chrome_switches is that there are two ...

5 years, 1 month ago (2015-10-26 23:30:14 UTC) #23

Devlin

extensions lgtm. I was kind of hoping to reduce the number of if-defs yet further, ...

5 years, 1 month ago (2015-10-28 18:06:30 UTC) #25

msw

latest c/b/ui lgtm with iwyu nits. https://codereview.chromium.org/1415333002/diff/320001/chrome/browser/ui/browser_commands.cc File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1415333002/diff/320001/chrome/browser/ui/browser_commands.cc#newcode53 chrome/browser/ui/browser_commands.cc:53: #include "chrome/common/chrome_switches.h" nit: ...

5 years, 1 month ago (2015-10-29 18:41:14 UTC) #29

imcheng

https://codereview.chromium.org/1415333002/diff/320001/chrome/browser/ui/browser_commands.cc File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1415333002/diff/320001/chrome/browser/ui/browser_commands.cc#newcode53 chrome/browser/ui/browser_commands.cc:53: #include "chrome/common/chrome_switches.h" On 2015/10/29 18:41:13, msw wrote: > nit: ...

5 years, 1 month ago (2015-10-29 21:44:56 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415333002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415333002/360001

5 years, 1 month ago (2015-10-30 00:44:50 UTC) #33

commit-bot: I haz the power

5 years, 1 month ago (2015-10-30 00:52:00 UTC) #35

Message was sent while issue was closed.

Patchset 16 (id:??) landed as
https://crrev.com/aa022abfc5393ba0f2f030228070504ffd2d432a
Cr-Commit-Position: refs/heads/master@{#357003}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages