Media Remoting: Add feature to control enabling Media Remoting.

Media Remoting: Add feature to control enabling Media Remoting.
The feature is enabled by default.

BUG=643964

[xhwang, 2016-11-02 05:18:38]

xhwang@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2016-11-02 05:18:39]

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2776: base::WeakPtr<media::MediaObserver>
media_observer = nullptr;
"= nullptr" not necessary

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2778:
std::unique_ptr<media::RemotingController> remoting_controller = nullptr;
"= nullptr" not necessary

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2837: remoting_controller ?
remoting_controller.get() : nullptr;
This change seems unnecessary; it doesn't change anything...

https://codereview.chromium.org/2470003002/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2470003002/diff/1/media/base/media_switches.c...
media/base/media_switches.cc:181: base::FEATURE_ENABLED_BY_DEFAULT};
Can we disable it by default for now and only enable it after we have it work
end-to-end and done some proper testing?

[xjz, 2016-11-02 17:48:05]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2016-11-02 17:48:05]

Addressed xhwang's comments. PTAL.

miu: Do we want to enable this feature by default for now?

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2776: base::WeakPtr<media::MediaObserver>
media_observer = nullptr;
On 2016/11/02 05:18:39, xhwang wrote:
> "= nullptr" not necessary

Done.

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2778:
std::unique_ptr<media::RemotingController> remoting_controller = nullptr;
On 2016/11/02 05:18:38, xhwang wrote:
> "= nullptr" not necessary

Done.

https://codereview.chromium.org/2470003002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:2837: remoting_controller ?
remoting_controller.get() : nullptr;
On 2016/11/02 05:18:38, xhwang wrote:
> This change seems unnecessary; it doesn't change anything...

Done.

https://codereview.chromium.org/2470003002/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2470003002/diff/1/media/base/media_switches.c...
media/base/media_switches.cc:181: base::FEATURE_ENABLED_BY_DEFAULT};
On 2016/11/02 05:18:39, xhwang wrote:
> Can we disable it by default for now and only enable it after we have it work
> end-to-end and done some proper testing?

+miu

WDYT?

[miu, 2016-11-02 23:35:19]

Agree that this feature should be disabled until it's ready-to-launch. ;)

IMHO, however, the feature switch should be placed at the top-level, browser
side (e.g., CastRemotingConnector). Note that, for now, none of this feature can
activate without the MR extension code in-place (i.e., the "sink available"
notification will never occur); and I'm still in the middle of writing that. In
the meantime, it is useful to have some of the object graph and code paths
running in release build (think "dark launch"), to ensure there aren't any
robustness or performance regressions for the more-common, non-mirroring usage.

So, before the MR extension code is deployed, we *would* want to add a feature
switch. I was planning on doing this browser-side (in
chrome/browser/media/cast_remoting_connector.cc) alongside other code changes we
will be making for Finch experiments.

[xhwang, 2016-11-02 23:51:21]

On 2016/11/02 23:35:19, miu wrote:
> Agree that this feature should be disabled until it's ready-to-launch. ;)
> 
> IMHO, however, the feature switch should be placed at the top-level, browser
> side (e.g., CastRemotingConnector). Note that, for now, none of this feature
can
> activate without the MR extension code in-place (i.e., the "sink available"
> notification will never occur); and I'm still in the middle of writing that.
In
> the meantime, it is useful to have some of the object graph and code paths
> running in release build (think "dark launch"), to ensure there aren't any
> robustness or performance regressions for the more-common, non-mirroring
usage.
> 
> So, before the MR extension code is deployed, we *would* want to add a feature
> switch. I was planning on doing this browser-side (in
> chrome/browser/media/cast_remoting_connector.cc) alongside other code changes
we
> will be making for Finch experiments.

sgtm

The base::Feature itself can still live in media_switches.*. But you can also
choose to enable/disable some browser side features as well.

[xjz, 2016-11-02 23:57:37]

On 2016/11/02 23:51:21, xhwang wrote:
> On 2016/11/02 23:35:19, miu wrote:
> > Agree that this feature should be disabled until it's ready-to-launch. ;)
> > 
> > IMHO, however, the feature switch should be placed at the top-level, browser
> > side (e.g., CastRemotingConnector). Note that, for now, none of this feature
> can
> > activate without the MR extension code in-place (i.e., the "sink available"
> > notification will never occur); and I'm still in the middle of writing that.
> In
> > the meantime, it is useful to have some of the object graph and code paths
> > running in release build (think "dark launch"), to ensure there aren't any
> > robustness or performance regressions for the more-common, non-mirroring
> usage.
> > 
> > So, before the MR extension code is deployed, we *would* want to add a
feature
> > switch. I was planning on doing this browser-side (in
> > chrome/browser/media/cast_remoting_connector.cc) alongside other code
changes
> we
> > will be making for Finch experiments.
> 
> sgtm
> 
> The base::Feature itself can still live in media_switches.*. But you can also
> choose to enable/disable some browser side features as well.

Thanks xhwang for reviewing this. As commented by miu, we will put off adding 
this feature before the MR extension code is deployed. So close this CL for now.

[xjz, 2016-11-02 23:57:47]

Description was changed from

==========
Media Remoting: Add feature to control enabling Media Remoting.
The feature is enabled by default.

BUG=643964
==========

to

==========
Media Remoting: Add feature to control enabling Media Remoting.
The feature is enabled by default.

BUG=643964
==========