Let fake UI allow a video capture device to be opened several times.

Let fake UI allow a video capture device to be opened several times.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112977

[mflodman_chromium_OOO, 2011-11-24 16:53:53]

Wei,
Can you look at my change to let fake UI allowing the same device to be opened
several times?

[wjia(left Chromium), 2011-11-24 20:44:29]

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:126: }
I was thinking of a simple way: remove the inner for loop above and replace it
with
devices_to_use.push_back(*(it->second.begin()));

In other words, it will pick up the first device in the list for both audio and
video, regardless of "in_use". Since this is just a "faked" UI, there would be
many ways to do it.

What do you think?

[mflodman_chromium_OOO, 2011-11-25 08:37:42]

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:126: }
On 2011/11/24 20:44:29, wjia wrote:
> I was thinking of a simple way: remove the inner for loop above and replace it
> with
> devices_to_use.push_back(*(it->second.begin()));
> 
> In other words, it will pick up the first device in the list for both audio
and
> video, regardless of "in_use". Since this is just a "faked" UI, there would be
> many ways to do it.
> 
> What do you think?

I thought about that too, but that would remove the possibility to test
MediaStreamDispatcherHost with multiple devices and also different cameras on
different tabs before the real UI is in place. I know the code gets more complex
this way, but I think it is worth it anyway for the testing.

Agree/disagree?

[wjia(left Chromium), 2011-11-25 15:47:07]

lgtm

[scherkus (not reviewing), 2011-11-29 02:39:53]

owners lgtm w/ nits

apologies if I'm missing something here, but what's the plan for switching away
from the fake UI and removing code like this?

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:132:
DCHECK(request->options.video_option != StreamOptions::kNoCamera);
DCHECK_NE

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:247:
EXPECT_CALL(*host_, OnStreamGenerated(kRenderId, kPageRequestId+1, 0, 1));
spaces around binary operators

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:264:
EXPECT_CALL(*host_, OnStreamGenerated(kRenderId, kPageRequestId+2, 0, 1));
spaces around binary operators

[mflodman_chromium_OOO, 2011-11-29 23:31:00]

Thanks for the look Andrew!

macourteau is working on the first version of the UI: crbug.com/105115.

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File content/browser/renderer_host/media/media_stream_device_settings.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_device_settings.cc:132:
DCHECK(request->options.video_option != StreamOptions::kNoCamera);
On 2011/11/29 02:39:53, scherkus wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
File
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc
(right):

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:247:
EXPECT_CALL(*host_, OnStreamGenerated(kRenderId, kPageRequestId+1, 0, 1));
On 2011/11/29 02:39:53, scherkus wrote:
> spaces around binary operators

Done.

http://codereview.chromium.org/8695004/diff/1/content/browser/renderer_host/m...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:264:
EXPECT_CALL(*host_, OnStreamGenerated(kRenderId, kPageRequestId+2, 0, 1));
On 2011/11/29 02:39:53, scherkus wrote:
> spaces around binary operators

Done.

[commit-bot: I haz the power, 2011-12-02 17:29:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mflodman@chromium.org/8695004/9001

[commit-bot: I haz the power, 2011-12-02 19:34:23]

Try job failure for 8695004-9001 (retry) on mac_rel for steps "browser_tests,
ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-12-05 11:19:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mflodman@chromium.org/8695004/9001

[commit-bot: I haz the power, 2011-12-05 14:42:57]

Change committed as 112977