Bring Audio Share and Tab Share Default On

Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed.

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539, 597540
Committed: https://crrev.com/85ce71fd37de5c52cd536a17a94db6d2fbead34d
Cr-Commit-Position: refs/heads/master@{#384656}

[qiangchen, 2016-03-25 21:22:24]

Description was changed from

==========
Bring Audio Share and Tab Share Default On

BUG=597539,597540
==========

to

==========
Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed. 

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539,597540
==========

[qiangchen, 2016-03-25 21:22:24]

qiangchen@chromium.org changed reviewers:
+ gyzhou@chromium.org

[GeorgeZ, 2016-03-25 22:29:04]

lgtm

https://codereview.chromium.org/1838623002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1838623002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:109:
request_audio = !has_flag;
Is the temporarily variable has_flag needed?

[qiangchen, 2016-03-28 16:27:27]

Patchset #1 (id:1) has been deleted

[qiangchen, 2016-03-28 16:36:58]

qiangchen@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org, sergeyu@chromium.org

[qiangchen, 2016-03-28 16:36:58]

This CL brings the new functionality on by default.
It essentially changes the enable flag to disable flag.

PTAL.

[jam, 2016-03-28 20:54:45]

which files do you want me to look at? if just content, lgtm

[qiangchen, 2016-03-28 21:09:53]

On 2016/03/28 20:54:45, jam wrote:
> which files do you want me to look at? if just content, lgtm

Yes. Thanks.

sergeyu@: Can you take a look at the files under chrome/browser/extensions and
chrome/browser/media?

isherman@: Can you take a look at the histograms.xml? (Just the hash value
changed according to string change.)

[Ilya Sherman, 2016-03-28 23:30:39]

histograms.xml lgtm % a nit:

https://codereview.chromium.org/1838623002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1838623002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:72722: -  <int value="157217034"
label="enable-tab-for-desktop-share"/>
Please leave the previous (now unused) values in place as well.

[qiangchen, 2016-03-29 16:14:06]

Unremoved Unused Label

https://codereview.chromium.org/1838623002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1838623002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:72722: -  <int value="157217034"
label="enable-tab-for-desktop-share"/>
On 2016/03/28 23:30:39, Ilya Sherman (Away Mar 19-27) wrote:
> Please leave the previous (now unused) values in place as well.

Done.

[Sergey Ulanov, 2016-03-29 18:48:20]

Do we actually need these flags? When are they going to be useful? Can we just
remove the flags?

Also please see my comments on https://codereview.chromium.org/1644073002/ .

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:109:
request_audio = !has_flag;
What if the application doesn't care about audio? Is there a way for it to hide
the check box in that case?

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:369: const bool has_flag
= base::CommandLine::ForCurrentProcess()->HasSwitch(
See my next comment first, but if you decide to keep this flag then please
rename this variable. E.g. audio_support_disabled, or something like this.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:371: const bool
capture_audio = (!has_flag ? audio_permitted : true) &&
This looks weird. capture_audio may still be set to true when the --disable flag
is specified. I think the problem here is that the name of the flag doesn't
reflect the meaning. It doesn't really disable audio capture for desktop
streams. Audio capture support is always enabled for desktop streams. The flag
disabled the new mechanism used to request audio permissions. Maybe rename the
flag to make the meaning clear?

But I'm not sure we need that flag at all. Can it be removed?

[qiangchen, 2016-03-29 20:10:44]

Niklas suggests that we keep the flag there so far, in case that the code change
really breaks something, the testers could turn on the disable flag to continue
their workflow.

We will remove the flags after the functionality is fully tested.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:109:
request_audio = !has_flag;
On 2016/03/29 18:48:19, Sergey Ulanov wrote:
> What if the application doesn't care about audio? Is there a way for it to
hide
> the check box in that case?

Yes. When here we detect a request with type
"DESKTOP_CAPTURE_SOURCE_TYPE_AUDIO", it means the javascript chooseDesktopMedia
include "audio" as source, thus it means the application is interested in audio
capture.

If the application is not interested in audio capture, it should not include
"audio" in the source types, and then request_audio will be false here.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:369: const bool has_flag
= base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/03/29 18:48:19, Sergey Ulanov wrote:
> See my next comment first, but if you decide to keep this flag then please
> rename this variable. E.g. audio_support_disabled, or something like this.

Done.

I changed to check_audio_permission.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:371: const bool
capture_audio = (!has_flag ? audio_permitted : true) &&
On 2016/03/29 18:48:19, Sergey Ulanov wrote:
> This looks weird. capture_audio may still be set to true when the --disable
flag
> is specified. I think the problem here is that the name of the flag doesn't
> reflect the meaning. It doesn't really disable audio capture for desktop
> streams. Audio capture support is always enabled for desktop streams. The flag
> disabled the new mechanism used to request audio permissions. Maybe rename the
> flag to make the meaning clear?
> 
> But I'm not sure we need that flag at all. Can it be removed?

I found it weird also. Before my change, we do not support audio for desktop
sharing, but in the code, we can see it handles audio case. But I am not sure
whether this code path may serve some use cases unknown to me.

Original logic is capture_audio = audio_requested && audio_supported, and after
my change it should be capture_audio = audio_requested && audio_supported &&
audio_permitted.

Using tentative expression here is to make the logic exactly the same as before
when the enable-flag is off (or now the disable-flag is on).

[Sergey Ulanov, 2016-03-30 19:34:22]

> Niklas suggests that we keep the flag there so far, in case that the code
change
> really breaks something, the testers could turn on the disable flag to
continue
> their workflow.

IMO the chances of this feature breaking something are super slim, so it's not
worth keeping the flags around (and even if it does happen to break something
reverts are easy). As far as I can tell that these flags can only affect apps
that pass 'tab' or 'audio' in the sources parameter. Flags add extra code and
that added logic may have bugs in it as well. But feel free to keep the flags if
you disagree.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc
(right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_base.cc:109:
request_audio = !has_flag;
On 2016/03/29 20:10:44, qiangchenC wrote:
> On 2016/03/29 18:48:19, Sergey Ulanov wrote:
> > What if the application doesn't care about audio? Is there a way for it to
> hide
> > the check box in that case?
> 
> Yes. When here we detect a request with type
> "DESKTOP_CAPTURE_SOURCE_TYPE_AUDIO", it means the javascript
chooseDesktopMedia
> include "audio" as source, thus it means the application is interested in
audio
> capture.

I see. To be honest this doesn't look like a good API design. Did this API
change go through extension API review? The sources parameter is for list of
sources that the user can choose from. Audio support orthogonal, e.g. it's not
possible to choose audio without video. IMO it would be better to add a separate
optional parameter to that call. If you decide to stick with this API then you
should update the description for the sources parameter in
chrome/common/extensions/api/desktop_capture.json . Otherwise it's not clear
that the 'audio' has special meaning. Obviously this shouldn't be part of this
CL.


> 
> If the application is not interested in audio capture, it should not include
> "audio" in the source types, and then request_audio will be false here.

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
File chrome/browser/media/desktop_capture_access_handler.cc (right):

https://codereview.chromium.org/1838623002/diff/40001/chrome/browser/media/de...
chrome/browser/media/desktop_capture_access_handler.cc:371: const bool
capture_audio = (!has_flag ? audio_permitted : true) &&
On 2016/03/29 20:10:44, qiangchenC wrote:
> On 2016/03/29 18:48:19, Sergey Ulanov wrote:
> > This looks weird. capture_audio may still be set to true when the --disable
> flag
> > is specified. I think the problem here is that the name of the flag doesn't
> > reflect the meaning. It doesn't really disable audio capture for desktop
> > streams. Audio capture support is always enabled for desktop streams. The
flag
> > disabled the new mechanism used to request audio permissions. Maybe rename
the
> > flag to make the meaning clear?
> > 
> > But I'm not sure we need that flag at all. Can it be removed?
> 
> I found it weird also. Before my change, we do not support audio for desktop
> sharing, but in the code, we can see it handles audio case.

Audio was supported  for rather long time. See crbug.com/223639. Chromecast
relies on that functionality when casting screen. The difference is that
previously chooseDesktopMedia() API allowed it implicitly, vs now there is an
explicit check-box for it.

> But I am not sure
> whether this code path may serve some use cases unknown to me.

Yes, chromecast uses that functionality when casting screen.

> 
> Original logic is capture_audio = audio_requested && audio_supported, and
after
> my change it should be capture_audio = audio_requested && audio_supported &&
> audio_permitted.
> 
> Using tentative expression here is to make the logic exactly the same as
before
> when the enable-flag is off (or now the disable-flag is on).

I'm not arguing that the logic is not correct. It looks correct. It's the name
of the flag that needs to be changed. Currently the flag doesn't do what the
name says.
E.g. it could be renamed to --disable-audio-permission-in-desktop-capture-api.

[qiangchen, 2016-03-30 21:44:44]

We've decided today that Tab Sharing must be enabled after the picker window UI
project, because the current thumbnail for tab is ugly.

Thus, Tab Sharing should not be enabled by default on Stable Channel. But we
wish it enabled by default on DEV and BETA. Thus I changed tab flag to
three-valued.

[qiangchen, 2016-03-31 16:44:10]

Patchset #5 (id:100001) has been deleted

[qiangchen, 2016-04-01 16:18:34]

On 2016/03/30 21:44:44, qiangchenC wrote:
> We've decided today that Tab Sharing must be enabled after the picker window
UI
> project, because the current thumbnail for tab is ugly.
> 
> Thus, Tab Sharing should not be enabled by default on Stable Channel. But we
> wish it enabled by default on DEV and BETA. Thus I changed tab flag to
> three-valued.

Hi, Sergey, Can you take a look at the change?

[Sergey Ulanov, 2016-04-01 16:57:27]

On 2016/04/01 16:18:34, qiangchenC wrote:
> On 2016/03/30 21:44:44, qiangchenC wrote:
> > We've decided today that Tab Sharing must be enabled after the picker window
> UI
> > project, because the current thumbnail for tab is ugly.
> > 
> > Thus, Tab Sharing should not be enabled by default on Stable Channel. But we
> > wish it enabled by default on DEV and BETA. Thus I changed tab flag to
> > three-valued.
> 
> Hi, Sergey, Can you take a look at the change?

I was waiting for you to respond to my comments I sent you couple days ago (at
2016-03-30 19:34:22 UTC).
Anyway, to avoid dragging this review any longer, LGTM. I assume that the flag
will be removed soon, so the name of the flag doesn't matter that much.
But please do take into account my feedback regarding the API change. If you
decide to keep the current design then please update the API documentation.

[qiangchen, 2016-04-01 17:07:38]

The CQ bit was checked by qiangchen@chromium.org

[qiangchen, 2016-04-01 17:07:38]

The patchset sent to the CQ was uploaded after l-g-t-m from gyzhou@chromium.org,
jam@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1838623002/#ps120001
(title: "Three value flag for Tab")

[commit-bot: I haz the power, 2016-04-01 17:08:06]

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

[qiangchen, 2016-04-01 17:46:02]

Sorry, did not notice all of your comments in that reply. I just stopped
reading, when encounter ">>", simply thought everything below is from an old
post.

> I see. To be honest this doesn't look like a good API design. Did this API
> change go through extension API review? The sources parameter is for list of
> sources that the user can choose from. Audio support orthogonal, e.g. it's not
> possible to choose audio without video. IMO it would be better to add a
separate
> optional parameter to that call. If you decide to stick with this API then you
> should update the description for the sources parameter in
> chrome/common/extensions/api/desktop_capture.json . Otherwise it's not clear
> that the 'audio' has special meaning. Obviously this shouldn't be part of this
> CL.

I've gone through the API approval stage. The main reason doing this rather than
adding a parameter is for backward compatibility. When API change gets landed,
all the webpages using that API will be affected. Another way to keep backward
compatibility is to make the new parameter optional, but chooseDesktopMedia
already has an optional parameter called "target". Most of cases, "target" is
not used. If we add "request_audio_permission" as optional parameter after
"target", then it will force web developers who wants audio to specify a value
for "target" too. 

Finally we decided to put it in sources array, as audio can also be interpreted
as a source, and definitely this way has no compatibility issue.

> I'm not arguing that the logic is not correct. It looks correct. It's the name
> of the flag that needs to be changed. Currently the flag doesn't do what the
> name says.
> E.g. it could be renamed to --disable-audio-permission-in-desktop-capture-api.

Sorry, did not see this.

[commit-bot: I haz the power, 2016-04-01 19:10:12]

Description was changed from

==========
Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed. 

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539,597540
==========

to

==========
Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed. 

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539,597540
==========

[commit-bot: I haz the power, 2016-04-01 19:10:13]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-04-01 19:11:54]

Description was changed from

==========
Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed. 

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539,597540
==========

to

==========
Bring Audio Share and Tab Share Default On

Desktop share with audio and Desktop share using a chrome tab
as source are two new functionality developed.

Previously the functionalities are behind the flags
--enable-audio-support-for-desktop-share
--enable-tab-for-desktop-share

This CL changes the flags to be disable flags, namely:
--disable-audio-support-for-desktop-share
--disable-tab-for-desktop-share

In another word, we bring the functionalities on by default.

BUG=597539,597540

Committed: https://crrev.com/85ce71fd37de5c52cd536a17a94db6d2fbead34d
Cr-Commit-Position: refs/heads/master@{#384656}
==========

[commit-bot: I haz the power, 2016-04-01 19:11:55]

Patchset 5 (id:??) landed as
https://crrev.com/85ce71fd37de5c52cd536a17a94db6d2fbead34d
Cr-Commit-Position: refs/heads/master@{#384656}