refactor EnumerateDevices to make it a persistent request.

refactor EnumerateDevices to make it a persistent request.
After the requester calls EnumerateDevices, it should expect new device list as initial response and whenever devices are changed.
A new method StopEnumerateDevices is also added to allow the request to stop device enumeration.

It also includes corresponding change in Pepper since EnumerateDevices contract is changed (http://codereview.chromium.org/10837064/).
This patch keeps current Pepper API, while using the new API of EnumerateDevices from MediaStreamDispatcher. 
Pepper calls StopEnumerateDevices when it receives the first enumerated result. 

A new Pepper API will be created in a separate patch to make EnumerateDevices persistent. 

BUG=137799
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150491

[tommi (sloooow) - chröme, 2012-07-30 12:10:20]

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:52: if (stream_type
== content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE &&
nit:

return (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE &&
        options.video) ||
       (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE &&
        options.audio);

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:148:
enumeration_in_progress_(content::NUM_MEDIA_STREAM_DEVICE_TYPES, false),
This array seems to be used in conjunction with the cache.  How about adding a
bool to the struct, enumeration_in_progress_ and reduce the bookkeeping?

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:267: if
(it->second.type == DeviceRequest::kEnumerateDevices) {
kEnumerateDevices -> ENUMERATE_DEVICES

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:318:
new_request.state[type] = DeviceRequest::kRequested;
kRequested -> REQUESTED

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:377:
requests_[label].requester->DevicesEnumerated(label, cache->devices);
nit: you've already looked up the label so you could use an iterator instead of
looking it up again via the [] operator.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:386:
base::SystemMonitor::Get()->AddDevicesChangedObserver(this);
What do you think about this:

* Remove:
  - StartMonitoring
  - StopMonitoring
  - monitoring_started_ 
  - the call to BrowserMainLoop::GetDevicesMonitor()->Start(this).
  I don't think we need a dependency on BrowserLoop here since
  all we need is provided by the SystemMonitor.

* Move the creation of the MediaStreamManager from BML::MainMessageLoopStart to
BML::BrowserThreadsStarted.

* In the MediaStreamManager ctor invoke AddDevicesChangedObserver:

BrowserThread::PostTask(BrowserThread::IO,
    FROM_HERE,
    base::Bind(&base::SystemMonitor::AddDevicesChangedObserver,
        base::SystemMonitor::Get(), this));


I think this will simplify things quite a bit without reduced functionality.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:401: void
MediaStreamManager::ClearEnumerationCache(EnumerationCache* cache) {
could an empty cache mean valid == false?
if so, we could just use the array directly instead of a struct and clear() it
here.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:403: if
(cache->valid) {
no need to check it first.  just set valid to false regardless.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:549: if
(!cache->valid || cache->devices != devices) {
I think this is a case for not needing the |valid| variable.  Just clear the
array if we want to refresh the cache.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:837: return;  //
Uninterested device change.
s/Uninterested/Uninteresting.

http://codereview.chromium.org/10830063/diff/1/content/common/media/media_str...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/10830063/diff/1/content/common/media/media_str...
content/common/media/media_stream_options.h:42: bool operator==(const
StreamDeviceInfo &other) const;
See previous comments about this operator.

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_dispatcher.cc:101: DVLOG(1) <<
"MediaStreamDispatcher::EnumerateDevices("
Is this method (and the other methods that touch the unprotected member
variables) always called on a known thread?  If so, can we DCHECK that?

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_dispatcher_unittest.cc:25: static const
char* kLabel = "test";
[]

[wjia(left Chromium), 2012-08-01 01:28:59]

Thanks! PTAL.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:52: if (stream_type
== content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE &&
On 2012/07/30 12:10:21, tommi wrote:
> nit:
> 
> return (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE &&
>         options.video) ||
>        (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE &&
>         options.audio);

Done.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:148:
enumeration_in_progress_(content::NUM_MEDIA_STREAM_DEVICE_TYPES, false),
On 2012/07/30 12:10:21, tommi wrote:
> This array seems to be used in conjunction with the cache.  How about adding a
> bool to the struct, enumeration_in_progress_ and reduce the bookkeeping?

This |enumeration_in_progress_| also include other type of requests, such as
GenerationStream, while cache is used for EnumerateDevices only.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:267: if
(it->second.type == DeviceRequest::kEnumerateDevices) {
On 2012/07/30 12:10:21, tommi wrote:
> kEnumerateDevices -> ENUMERATE_DEVICES

Done.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:318:
new_request.state[type] = DeviceRequest::kRequested;
On 2012/07/30 12:10:21, tommi wrote:
> kRequested -> REQUESTED

Done.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:377:
requests_[label].requester->DevicesEnumerated(label, cache->devices);
On 2012/07/30 12:10:21, tommi wrote:
> nit: you've already looked up the label so you could use an iterator instead
of
> looking it up again via the [] operator.

Done.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:386:
base::SystemMonitor::Get()->AddDevicesChangedObserver(this);
On 2012/07/30 12:10:21, tommi wrote:
> What do you think about this:
> 
> * Remove:
>   - StartMonitoring
>   - StopMonitoring
>   - monitoring_started_ 
>   - the call to BrowserMainLoop::GetDevicesMonitor()->Start(this).
>   I don't think we need a dependency on BrowserLoop here since
>   all we need is provided by the SystemMonitor.
> 
> * Move the creation of the MediaStreamManager from BML::MainMessageLoopStart
to
> BML::BrowserThreadsStarted.
> 
> * In the MediaStreamManager ctor invoke AddDevicesChangedObserver:
> 
> BrowserThread::PostTask(BrowserThread::IO,
>     FROM_HERE,
>     base::Bind(&base::SystemMonitor::AddDevicesChangedObserver,
>         base::SystemMonitor::Get(), this));
> 
> 
> I think this will simplify things quite a bit without reduced functionality.

The start/stop of DeviceMonitor has been removed because DeviceMonitor has been
started ever since browser is started.

It makes sense for MSM to listen to SystemMonitor only when there is such
request.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:401: void
MediaStreamManager::ClearEnumerationCache(EnumerationCache* cache) {
On 2012/07/30 12:10:21, tommi wrote:
> could an empty cache mean valid == false?
> if so, we could just use the array directly instead of a struct and clear() it
> here.

Unfortunately, empty cache doesn't mean valid or invalid since it's possible
enumeration returns an empty list of device when there is no such devices
attached to the computer.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:403: if
(cache->valid) {
On 2012/07/30 12:10:21, tommi wrote:
> no need to check it first.  just set valid to false regardless.

Done.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:549: if
(!cache->valid || cache->devices != devices) {
On 2012/07/30 12:10:21, tommi wrote:
> I think this is a case for not needing the |valid| variable.  Just clear the
> array if we want to refresh the cache.

Without |valid| flag, we have to keep cache always up-to-date. That means we
have to enumerate devices whenever their is a DevicesChanged event. That's what
we don't want to do since enumeration is known to be expensive and we try to
avoid it.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:837: return;  //
Uninterested device change.
On 2012/07/30 12:10:21, tommi wrote:
> s/Uninterested/Uninteresting.

Done.

http://codereview.chromium.org/10830063/diff/1/content/common/media/media_str...
File content/common/media/media_stream_options.h (right):

http://codereview.chromium.org/10830063/diff/1/content/common/media/media_str...
content/common/media/media_stream_options.h:42: bool operator==(const
StreamDeviceInfo &other) const;
On 2012/07/30 12:10:21, tommi wrote:
> See previous comments about this operator.

To explain why all fields are used for comparison:
1. there should be no question about |stream_type|, |name| and |device_id|.
2. for |in_use|, if a device has been opened by other clients since last
enumeration, this can be some useful information for the requester, e.g., the
requester can display only non-opened devices.
3. for |session_id|, it should be same value, i.e., kNoId.

For now, only EnumerateDevices operation is using this comparison.

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_dispatcher.cc:101: DVLOG(1) <<
"MediaStreamDispatcher::EnumerateDevices("
On 2012/07/30 12:10:21, tommi wrote:
> Is this method (and the other methods that touch the unprotected member
> variables) always called on a known thread?  If so, can we DCHECK that?

They are all called on main thread. DCHECK has been added.

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/renderer/media/media_s...
content/renderer/media/media_stream_dispatcher_unittest.cc:25: static const
char* kLabel = "test";
On 2012/07/30 12:10:21, tommi wrote:
> []

Done.

[tommi (sloooow) - chröme, 2012-08-01 10:21:37]

We're almost there.  Just a heads up that I might not be available to do a
review later today or tomorrow.  If I haven't replied to your next patch set by
eod, please go ahead and ask another reviewer to take a look so that we won't
lose time.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:103:
streams_.erase(it);
was this a bug?  I'm guessing the normal place where the entry gets removed is
in OnStopGeneratedStream, correct?

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:386:
base::SystemMonitor::Get()->AddDevicesChangedObserver(this);
On 2012/08/01 01:28:59, wjia wrote:
> The start/stop of DeviceMonitor has been removed because DeviceMonitor has
been
> started ever since browser is started.
> 
> It makes sense for MSM to listen to SystemMonitor only when there is such
> request. 

Great.  Thanks.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:401: void
MediaStreamManager::ClearEnumerationCache(EnumerationCache* cache) {
On 2012/08/01 01:28:59, wjia wrote:
> On 2012/07/30 12:10:21, tommi wrote:
> > could an empty cache mean valid == false?
> > if so, we could just use the array directly instead of a struct and clear()
it
> > here.
> 
> Unfortunately, empty cache doesn't mean valid or invalid since it's possible
> enumeration returns an empty list of device when there is no such devices
> attached to the computer.

sgtm.

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:549: if
(!cache->valid || cache->devices != devices) {
On 2012/08/01 01:28:59, wjia wrote:
> On 2012/07/30 12:10:21, tommi wrote:
> > I think this is a case for not needing the |valid| variable.  Just clear the
> > array if we want to refresh the cache.
> 
> Without |valid| flag, we have to keep cache always up-to-date. That means we
> have to enumerate devices whenever their is a DevicesChanged event. That's
what
> we don't want to do since enumeration is known to be expensive and we try to
> avoid it.

sgtm

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:411:
enumeration_in_progress_[stream_type]++;
nit: we tend do use prefix increment to avoid a temporary.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:599:
enumeration_in_progress_[stream_type]--;
nit: prefix

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:844: // change.
this makes sense... should we cancel the previous enumerations?

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.h:218: // monitoring is
stopped or there is no request for that type of devices.
s/devices/device.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.h:223: //
AudioInputDeviceManager, in order to not enumerate when not necessary.
nit: ...in order to only enumerate when necessary.
(to avoid the double negative)

...or is this rather to guard against kicking of the same enumeration type if
one is in progress?

How big can this vector grow?  Is the size static or dynamic?

What does the 'int' signify?

Edit: I see now that it's a reference count and the index into the array is a
MediaStreamType.  This sounds like a candidate for a fixed size array.  What do
you think about:

int active_enumeration_ref_count_[content::NUM_MEDIA_STREAM_DEVICE_TYPES];

I think that makes it clear how it should be indexed, what size it is and also
that the values it holds are reference counts.  An added bonus is that it
requires less code and doesn't need additional dynamic heap memory allocations.

In the constructor you can initialize it to all zeros like so:

MediaStreamManager::MediaStreamManager()
    : ...
      active_enumeration_ref_count_(),

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
content/renderer/media/media_stream_dispatcher.cc:60: DCHECK_EQ(main_loop_,
MessageLoop::current());
I really like all the added DCHECKS in this file!

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
content/renderer/media/media_stream_dispatcher.h:141: MessageLoop* main_loop_;
Remember John's comment on MessageLoop pointers.

[wjia(left Chromium), 2012-08-01 14:24:03]

Thanks! PTAL if you have time. :-)

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_dispatcher_host.cc
(right):

http://codereview.chromium.org/10830063/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_dispatcher_host.cc:103:
streams_.erase(it);
On 2012/08/01 10:21:37, tommi wrote:
> was this a bug?  I'm guessing the normal place where the entry gets removed is
> in OnStopGeneratedStream, correct?

yes, it has been fixed in patch set 2.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:411:
enumeration_in_progress_[stream_type]++;
On 2012/08/01 10:21:38, tommi wrote:
> nit: we tend do use prefix increment to avoid a temporary.

Done.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:599:
enumeration_in_progress_[stream_type]--;
On 2012/08/01 10:21:38, tommi wrote:
> nit: prefix

Done.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:844: // change.
On 2012/08/01 10:21:38, tommi wrote:
> this makes sense... should we cancel the previous enumerations?

In current API between MSM and VCM/AIDM, we are not able to cancel previous
enumerations. If each enumeration has an ID, then we can skip those previous
enumerations when MSM receives OnDevicesEnumerated().

Anyhow, it's ok to keep those previous enumerations for now.  I don't see it
would create confusion for clients. If it does, we can refactor later.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.h:218: // monitoring is
stopped or there is no request for that type of devices.
On 2012/08/01 10:21:38, tommi wrote:
> s/devices/device.

Done.

http://codereview.chromium.org/10830063/diff/12/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.h:223: //
AudioInputDeviceManager, in order to not enumerate when not necessary.
On 2012/08/01 10:21:38, tommi wrote:
> nit: ...in order to only enumerate when necessary.
> (to avoid the double negative)
> 
> ...or is this rather to guard against kicking of the same enumeration type if
> one is in progress?
> 
> How big can this vector grow?  Is the size static or dynamic?
> 
> What does the 'int' signify?
> 
> Edit: I see now that it's a reference count and the index into the array is a
> MediaStreamType.  This sounds like a candidate for a fixed size array.  What
do
> you think about:
> 
> int active_enumeration_ref_count_[content::NUM_MEDIA_STREAM_DEVICE_TYPES];
> 
> I think that makes it clear how it should be indexed, what size it is and also
> that the values it holds are reference counts.  An added bonus is that it
> requires less code and doesn't need additional dynamic heap memory
allocations.
> 
> In the constructor you can initialize it to all zeros like so:
> 
> MediaStreamManager::MediaStreamManager()
>     : ...
>       active_enumeration_ref_count_(),

Agree and done. I didn't change it since it was there. So I changed only type
from bool to int.

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/12/content/renderer/media/media_...
content/renderer/media/media_stream_dispatcher.h:141: MessageLoop* main_loop_;
On 2012/08/01 10:21:38, tommi wrote:
> Remember John's comment on MessageLoop pointers.

Right. Changed to MLP.

[scherkus (not reviewing), 2012-08-02 00:14:19]

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo &other) const {
& with type name

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
content/common/media/media_stream_options.cc:29: name == other.name &&
nit: this should be 4 space indent here + below (the same-level indent only
applies to when stuff is inside parentheses, otherwise it's a 4 space indent)

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.cc:142:
&audio_enumeration_state_) ||
I don't think there's any harm in calling RER() for each type and we can get rid
of the || stuff

the size of the request list is going to be awfully small

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.h:22: }  // namespace
nit: you don't need to // namespace for fwd decls like this

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.h:108: struct EnumerationState {
instead of stuffing everything into a struct how about this:

// If >= 0 then we've started (removes need for started variable)
int audio_enumeration_id_;
int video_enumeration_id_;

struct CachedDevices {
  media_stream::StreamDeviceInfoArray devices;
  std::string label;
};

EnumerationRequestList audio_requests_;
EnumerationRequestList video_requests_;

// Don't risk people forgetting to check cache_valid -- delete the
// cache when it's not valid
scoped_ptr<CachedDevices> cached_audio_devices_;
scoped_ptr<CachedDevices> cached_video_devices_;


WDYT?

[scherkus (not reviewing), 2012-08-02 00:14:45]

also there's a typo in your CL description + subject:

Enumerat_e_Devices

[wjia(left Chromium), 2012-08-02 22:15:27]

Thanks! PTAL.

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo &other) const {
On 2012/08/02 00:14:19, scherkus wrote:
> & with type name

Done.

http://codereview.chromium.org/10830063/diff/4006/content/common/media/media_...
content/common/media/media_stream_options.cc:29: name == other.name &&
On 2012/08/02 00:14:19, scherkus wrote:
> nit: this should be 4 space indent here + below (the same-level indent only
> applies to when stuff is inside parentheses, otherwise it's a 4 space indent)

Done.

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.cc:142:
&audio_enumeration_state_) ||
On 2012/08/02 00:14:19, scherkus wrote:
> I don't think there's any harm in calling RER() for each type and we can get
rid
> of the || stuff
> 
> the size of the request list is going to be awfully small

Done.

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.h:22: }  // namespace
On 2012/08/02 00:14:19, scherkus wrote:
> nit: you don't need to // namespace for fwd decls like this

Done.

http://codereview.chromium.org/10830063/diff/4006/content/renderer/media/medi...
content/renderer/media/media_stream_dispatcher.h:108: struct EnumerationState {
On 2012/08/02 00:14:19, scherkus wrote:
> instead of stuffing everything into a struct how about this:
> 
> // If >= 0 then we've started (removes need for started variable)
> int audio_enumeration_id_;
> int video_enumeration_id_;
> 
> struct CachedDevices {
>   media_stream::StreamDeviceInfoArray devices;
>   std::string label;
> };
> 
> EnumerationRequestList audio_requests_;
> EnumerationRequestList video_requests_;
> 
> // Don't risk people forgetting to check cache_valid -- delete the
> // cache when it's not valid
> scoped_ptr<CachedDevices> cached_audio_devices_;
> scoped_ptr<CachedDevices> cached_video_devices_;
> 
> 
> WDYT?

Done.

[scherkus (not reviewing), 2012-08-02 22:45:17]

lgtm w/ nits

thanks wei!

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:145:
active_enumeration_ref_count_(),
does this need to get initialized at all?

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:843:
ClearEnumerationCache(cache);
nit: early return where possible

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:846: // because
those enumeration commands could be sent before this devices
s/this/these

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:58:
media_stream::StreamDeviceInfoArray device_array;
nit: this can be |devices| -- no need to annotate variables names with their
type

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:157: bool
MediaStreamDispatcher::RemoveEnumerationRequest(
RER() no longer needs to return a bool

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.h:15: #include
"base/memory/scoped_ptr.h"
a->z ordering

[tommi (sloooow) - chröme, 2012-08-03 09:07:33]

lgtm from me as well.

On 2012/08/02 22:45:17, scherkus wrote:
> lgtm w/ nits
> 
> thanks wei!
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_manager.cc:145:
> active_enumeration_ref_count_(),
> does this need to get initialized at all?
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_manager.cc:843:
> ClearEnumerationCache(cache);
> nit: early return where possible
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_manager.cc:846: // because
> those enumeration commands could be sent before this devices
> s/this/these
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
> File content/renderer/media/media_stream_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
> content/renderer/media/media_stream_dispatcher.cc:58:
> media_stream::StreamDeviceInfoArray device_array;
> nit: this can be |devices| -- no need to annotate variables names with their
> type
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
> content/renderer/media/media_stream_dispatcher.cc:157: bool
> MediaStreamDispatcher::RemoveEnumerationRequest(
> RER() no longer needs to return a bool
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
> File content/renderer/media/media_stream_dispatcher.h (right):
> 
>
http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
> content/renderer/media/media_stream_dispatcher.h:15: #include
> "base/memory/scoped_ptr.h"
> a->z ordering

[tommi (sloooow) - chröme, 2012-08-03 09:10:35]

one last comment that I thought had been addressed.  slgtm.

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo& other) const {
nit: Change this method to IsEqual() (we don't override operators except for
special cases)

[wjia(left Chromium), 2012-08-03 21:53:45]

Thanks!

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:145:
active_enumeration_ref_count_(),
On 2012/08/02 22:45:18, scherkus wrote:
> does this need to get initialized at all?

yes, it's needed since active_enumeration_ref_count_[stream_type]==0 is used as
flag for calling low level enumeration code.

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:843:
ClearEnumerationCache(cache);
On 2012/08/02 22:45:18, scherkus wrote:
> nit: early return where possible

Done.

http://codereview.chromium.org/10830063/diff/13006/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:846: // because
those enumeration commands could be sent before this devices
On 2012/08/02 22:45:18, scherkus wrote:
> s/this/these

Done.

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo& other) const {
On 2012/08/03 09:10:35, tommi wrote:
> nit: Change this method to IsEqual() (we don't override operators except for
> special cases)

This overloading is needed for StreamDeviceInfoArray which is typedef'ed as
std::vector<StreamDeviceInfo>. In MediaStreamManager::DevicesEnumerated, the
incoming devices are compared against cached devices.

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:58:
media_stream::StreamDeviceInfoArray device_array;
On 2012/08/02 22:45:18, scherkus wrote:
> nit: this can be |devices| -- no need to annotate variables names with their
> type

Done.

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.cc:157: bool
MediaStreamDispatcher::RemoveEnumerationRequest(
On 2012/08/02 22:45:18, scherkus wrote:
> RER() no longer needs to return a bool

Done.

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
File content/renderer/media/media_stream_dispatcher.h (right):

http://codereview.chromium.org/10830063/diff/13006/content/renderer/media/med...
content/renderer/media/media_stream_dispatcher.h:15: #include
"base/memory/scoped_ptr.h"
On 2012/08/02 22:45:18, scherkus wrote:
> a->z ordering

Done.

[tommi (sloooow) - chröme, 2012-08-04 09:27:08]

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo& other) const {
On 2012/08/03 21:53:45, wjia wrote:
> On 2012/08/03 09:10:35, tommi wrote:
> > nit: Change this method to IsEqual() (we don't override operators except for
> > special cases)
> 
> This overloading is needed for StreamDeviceInfoArray which is typedef'ed as
> std::vector<StreamDeviceInfo>. In MediaStreamManager::DevicesEnumerated, the
> incoming devices are compared against cached devices.

You can use std::equal instead.  See for example:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/policy/poli...

[wjia(left Chromium), 2012-08-07 23:16:34]

Brett, could you transfer your lgtm of http://codereview.chromium.org/10837064/
to here for Pepper stuff? I need combine both patches since they have to landed
together.

Thanks!

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
File content/common/media/media_stream_options.cc (right):

http://codereview.chromium.org/10830063/diff/13006/content/common/media/media...
content/common/media/media_stream_options.cc:27: bool
StreamDeviceInfo::operator==(const StreamDeviceInfo& other) const {
On 2012/08/04 09:27:08, tommi wrote:
> On 2012/08/03 21:53:45, wjia wrote:
> > On 2012/08/03 09:10:35, tommi wrote:
> > > nit: Change this method to IsEqual() (we don't override operators except
for
> > > special cases)
> > 
> > This overloading is needed for StreamDeviceInfoArray which is typedef'ed as
> > std::vector<StreamDeviceInfo>. In MediaStreamManager::DevicesEnumerated, the
> > incoming devices are compared against cached devices.
> 
> You can use std::equal instead.  See for example:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/policy/poli...

Done.

[brettw, 2012-08-07 23:20:44]

lgtm