Bring Audio Share and Tab Share Default On

chrome/browser/media/desktop_capture_access_handler.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/desktop_capture_access_handler.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 348 matching lines @@
       request.audio_type == content::MEDIA_DESKTOP_AUDIO_CAPTURE;
 
   // This value shows for a given capture type, whether the system or our code
   // can support audio sharing. Currently audio is only supported for screen and
   // tab/webcontents capture streams.
   const bool audio_supported =
       (media_id.type == content::DesktopMediaID::TYPE_SCREEN &&
        loopback_audio_supported) ||
       media_id.type == content::DesktopMediaID::TYPE_WEB_CONTENTS;
 
   const bool has_flag = base::CommandLine::ForCurrentProcess()->HasSwitch(

[Sergey Ulanov, 2016/03/29 18:48:19]

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.

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

Done.

I changed to check_audio_permission.

-      extensions::switches::kEnableDesktopCaptureAudio);
-  const bool capture_audio =
-      (has_flag ? audio_permitted : true) && audio_requested && audio_supported;
+      extensions::switches::kDisableDesktopCaptureAudio);
+  const bool capture_audio = (!has_flag ? audio_permitted : true) &&

[Sergey Ulanov, 2016/03/29 18:48:19]

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]

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]

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.
Yes, chromecast uses that functionality when casting screen.
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.

+      audio_requested && audio_supported;
 
   ui = GetDevicesForDesktopCapture(&devices, media_id, capture_audio, true,
                                    GetApplicationTitle(web_contents, extension),
                                    base::UTF8ToUTF16(original_extension_name));
 
   callback.Run(devices, content::MEDIA_DEVICE_OK, std::move(ui));
 }
 
 void DesktopCaptureAccessHandler::UpdateMediaRequestState(
     int render_process_id,
@@ skipping 22 matching lines @@
         desktop_capture_sessions_.erase(it);
         break;
       }
     }
   }
 }
 
 bool DesktopCaptureAccessHandler::IsCaptureInProgress() {
   return desktop_capture_sessions_.size() > 0;
 }