Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7184010:
MediaStreamDispatcher (Closed)
|
Created:
9 years, 6 months ago by Per K Modified:
9 years, 5 months ago Reviewers:
commit-bot: I haz the power, jam, mflodman1, Leandro Graciá Gil, scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, sjl, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., joi+watch-content_chromium.org, acolwell+watch_chromium.org, annacc, darin-cc_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionMediaStreamDispatcher
This is the second Chromium patch needed to support media streams
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#video-conferencing-and-peer-to-peer-communication. The first patch is here http://codereview.chromium.org/7192007/.
The patch contain types needed in both the render and browser process and MediaStreamDispatcher that is used for sending request from the render process to the browser process for granting a webpage access to audio input and video capture devices.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90752
Patch Set 1 #Patch Set 2 : '' #
Total comments: 86
Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 3
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #
Total comments: 7
Patch Set 10 : Fixed code review issues found by jam. #Patch Set 11 : '' #Messages
Total messages: 22 (0 generated)
http://codereview.chromium.org/7184010/diff/5001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/7184010/diff/5001/content/content_common.gypi#... content/content_common.gypi:146: 'common/media/media_stream_options.cc', Looks like the media_stream_options files belong to patch 1, not to this one. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:69: const std::string &label, & at the wrong side. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:70: const media_stream::StreamDeviceInfoArray audio_array, This is an STL vector. Is there any reason not to pass it by reference? Why is it const if it's a copy? http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:106: void MediaStreamDispatcher::OnVideoDeviceFailed(std::string label, int index) { Are you sure this can't be const std::string& ? Same for the other methods. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:124: it->second.audio_array.erase(device_it + index); I think a few DCHECKs here would be good. If for some reason the message ordering fails or something strange happens this can do weird things. Is there anything else you're assuming when doing this? http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:151: return &label_stream_map_[label].video_array; I don't think this is safe at all. Maybe I'm wrong, but AFAIK STL maps can change the address where they store their contents and re-organize themselves. Also, you're returning a pointer to an STL vector. Why not return always a const reference and have a local static empty vector for the cases where you return NULL? I'm not sure if that's usual in Chromium but I've seen it many times in WebKit. It should make your test code cleaner too. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:29: const std::string &label, & in the wrong side. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:30: const media_stream::StreamDeviceInfoArray audio_device_array, This should be a reference since you're passing an STL vector. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:38: // error occur. Minor nit: occurs. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:44: // error occur. Same as before. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:60: EventHandler* event_handler, What is this EventHandler exactly for and who's providing it? I'm especially interested in its lifetime and ownership. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:102: const std::string &label, This should be const std::string& label (the & on the other side). http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:114: typedef std::list <Request> RequestList; Not sure, but I think this has an extra space. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:27: label_(""), This is not required as the default constructor will initialize to the empty string. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:33: const std::string &label, Same as mentioned in the media_stream_dispatcher.h http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:77: dispatcher->GenerateStream(kRequestId2, handler.get(), components, Probably not part of this test yet, but are you checking that no more that one request is using the same id?
please also have BUG= and TEST= http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... File content/common/media_stream_messages.h (right): http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. you should put this under content/common/media as well http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:36: media_stream::StreamDeviceInfoArray /*audio_device_list*/, nit: spaces around the /* */ + rest of this file http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:67: nit: whack extra lines here http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:13: main_message_loop_(NULL) { what about putting MessageLoop::current() in here http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:24: DCHECK(MessageLoop::current()== main_message_loop_); DCHECK_EQ http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:29: routing_id(), indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:36: DCHECK(MessageLoop::current()== main_message_loop_); DCHECK_EQ http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:45: routing_id(), nit: indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:74: it != requests_.end(); ++it) { indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:75: Request& request = *it; nit: get rid of extra space http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:94: it != requests_.end(); ++it) { indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:95: Request& request = *it; nit: get rid of extra space http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:122: media_stream::StreamDeviceInfoArray::iterator device_it = nit: drop = to next line http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:130: const std::string& label) { nit: this can fit on one line http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:131: DCHECK(MessageLoop::current()== main_message_loop_); DCHECK_EQ http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:143: const std::string& label) { nit: this can fit on one line http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:144: DCHECK(MessageLoop::current()== main_message_loop_); DCHECK_EQ http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:24: class EventHandler { as discussed before (not sure if you were on that CL thread or not...) but as per style guide inner classes are only permitted if they're used internally by the class in other words, please split this out to a separate .h file http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:69: const media_stream::StreamDeviceInfoArray* audio_device_array( const ptr to an array? if the point is to verify if a label is open why not provide that method directly? IsAudioDeviceOpen(const std::string& label) etc http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:72: const std::string& label); nit: indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:78: typedef int IPC_REQUEST_ID; nit: why is this type name ALL_CAPS? we use CamelCase for types http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:79: struct Request { this is an example where inner classes is ok :) http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:114: typedef std::list <Request> RequestList; On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Not sure, but I think this has an extra space. yes it does :) http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:116: MessageLoop* main_message_loop_; nit: s/main_message_loop_/message_loop_/ (no need to say it's the "main" one) http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:126: EXPECT_TRUE(dispatcher->audio_device_array(stream_label2) == NULL); if you're testing for null pointer you can use EXPECT_FALSE w/o the == NULL part or use EXPECT_EQ http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:148: // Test failure when creating a stream. indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:151: security_origin); indentation http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:162: security_origin); indentation
Thanks guys. New upload. http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... File content/common/media_stream_messages.h (right): http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2011/06/17 03:31:59, scherkus wrote: > you should put this under content/common/media as well Done. http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:36: media_stream::StreamDeviceInfoArray /*audio_device_list*/, On 2011/06/17 03:31:59, scherkus wrote: > nit: spaces around the /* */ > > + rest of this file Done. http://codereview.chromium.org/7184010/diff/5001/content/common/media_stream_... content/common/media_stream_messages.h:67: On 2011/06/17 03:31:59, scherkus wrote: > nit: whack extra lines here Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:13: main_message_loop_(NULL) { On 2011/06/17 03:31:59, scherkus wrote: > what about putting MessageLoop::current() in here Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:24: DCHECK(MessageLoop::current()== main_message_loop_); On 2011/06/17 03:31:59, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:29: routing_id(), On 2011/06/17 03:31:59, scherkus wrote: > indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:36: DCHECK(MessageLoop::current()== main_message_loop_); On 2011/06/17 03:31:59, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:45: routing_id(), On 2011/06/17 03:31:59, scherkus wrote: > nit: indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:69: const std::string &label, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > & at the wrong side. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:70: const media_stream::StreamDeviceInfoArray audio_array, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > This is an STL vector. Is there any reason not to pass it by reference? Why is > it const if it's a copy? Nope. My mistake. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:74: it != requests_.end(); ++it) { On 2011/06/17 03:31:59, scherkus wrote: > indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:75: Request& request = *it; On 2011/06/17 03:31:59, scherkus wrote: > nit: get rid of extra space Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:94: it != requests_.end(); ++it) { On 2011/06/17 03:31:59, scherkus wrote: > indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:95: Request& request = *it; On 2011/06/17 03:31:59, scherkus wrote: > nit: get rid of extra space Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:106: void MediaStreamDispatcher::OnVideoDeviceFailed(std::string label, int index) { On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Are you sure this can't be const std::string& ? > Same for the other methods. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:122: media_stream::StreamDeviceInfoArray::iterator device_it = On 2011/06/17 03:31:59, scherkus wrote: > nit: drop = to next line http://www.chromium.org/developers/coding-style bool result = YetAnotherLongFunctionCall(first_argument, ??? http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:124: it->second.audio_array.erase(device_it + index); On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > I think a few DCHECKs here would be good. If for some reason the message > ordering fails or something strange happens this can do weird things. > > Is there anything else you're assuming when doing this? Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:130: const std::string& label) { On 2011/06/17 03:31:59, scherkus wrote: > nit: this can fit on one line Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:131: DCHECK(MessageLoop::current()== main_message_loop_); On 2011/06/17 03:31:59, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:143: const std::string& label) { On 2011/06/17 03:31:59, scherkus wrote: > nit: this can fit on one line Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:144: DCHECK(MessageLoop::current()== main_message_loop_); On 2011/06/17 03:31:59, scherkus wrote: > DCHECK_EQ Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:151: return &label_stream_map_[label].video_array; On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > I don't think this is safe at all. Maybe I'm wrong, but AFAIK STL maps can > change the address where they store their contents and re-organize themselves. > > Also, you're returning a pointer to an STL vector. Why not return always a const > reference and have a local static empty vector for the cases where you return > NULL? I'm not sure if that's usual in Chromium but I've seen it many times in > WebKit. It should make your test code cleaner too. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:24: class EventHandler { On 2011/06/17 03:31:59, scherkus wrote: > as discussed before (not sure if you were on that CL thread or not...) but as > per style guide inner classes are only permitted if they're used internally by > the class > > in other words, please split this out to a separate .h file I heard about the problem with VideoCaptureController on VS2005 where the class both inherited from a class called EventHandler and had a pointer to another class called Eventhandler. But does this mean that ex AudioMessageFilter::Delegate is violating the style? To me, it looks much neater to have them as inner classes. Anyway - I changed. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:29: const std::string &label, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > & in the wrong side. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:30: const media_stream::StreamDeviceInfoArray audio_device_array, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > This should be a reference since you're passing an STL vector. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:38: // error occur. On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Minor nit: occurs. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:44: // error occur. On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Same as before. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:60: EventHandler* event_handler, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > What is this EventHandler exactly for and who's providing it? I'm especially > interested in its lifetime and ownership. We wanted to make MediaStreamDispatcher independent of the WebKit types in order to be able to support for example Pepper if they want to be able to open a video capture and audio microphone. Therefore we split your first implementation in two. There will be a new class with the following declaration: class MediaStreamImpl : public WebKit::WebMediaStreamClient, public media_stream::StreamClient, public MediaStreamDispatcher::EventHandler It can not be submitted yet since WebMediaStreamClient is still under development and also the peer connection part of WebMediaStreamClient is not implemented. Henrik G is hopefully working on in. In the case of MediaStreamImpl the lifetime is the same as the renderview to my understanding. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:69: const media_stream::StreamDeviceInfoArray* audio_device_array( On 2011/06/17 03:31:59, scherkus wrote: > const ptr to an array? > > if the point is to verify if a label is open why not provide that method > directly? > > IsAudioDeviceOpen(const std::string& label) > > etc Changed to reference. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:72: const std::string& label); On 2011/06/17 03:31:59, scherkus wrote: > nit: indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:78: typedef int IPC_REQUEST_ID; On 2011/06/17 03:31:59, scherkus wrote: > nit: why is this type name ALL_CAPS? > > we use CamelCase for types Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:79: struct Request { On 2011/06/17 03:31:59, scherkus wrote: > this is an example where inner classes is ok :) Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:102: const std::string &label, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > This should be const std::string& label (the & on the other side). Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:114: typedef std::list <Request> RequestList; On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Not sure, but I think this has an extra space. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:116: MessageLoop* main_message_loop_; On 2011/06/17 03:31:59, scherkus wrote: > nit: s/main_message_loop_/message_loop_/ > > (no need to say it's the "main" one) Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher_unittest.cc (right): http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:27: label_(""), On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > This is not required as the default constructor will initialize to the empty > string. Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:33: const std::string &label, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Same as mentioned in the media_stream_dispatcher.h Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:77: dispatcher->GenerateStream(kRequestId2, handler.get(), components, On 2011/06/16 17:40:01, Leandro Graciá Gil wrote: > Probably not part of this test yet, but are you checking that no more that one > request is using the same id? The ipc_request_id is unique per MediaStreamDispatcher. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:126: EXPECT_TRUE(dispatcher->audio_device_array(stream_label2) == NULL); On 2011/06/17 03:31:59, scherkus wrote: > if you're testing for null pointer you can use EXPECT_FALSE w/o the == NULL part > > or use EXPECT_EQ Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:148: // Test failure when creating a stream. On 2011/06/17 03:31:59, scherkus wrote: > indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:151: security_origin); On 2011/06/17 03:31:59, scherkus wrote: > indentation Done. http://codereview.chromium.org/7184010/diff/5001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher_unittest.cc:162: security_origin); On 2011/06/17 03:31:59, scherkus wrote: > indentation Done.
http://codereview.chromium.org/7184010/diff/7001/content/common/content_messa... File content/common/content_message_generator.h (right): http://codereview.chromium.org/7184010/diff/7001/content/common/content_messa... content/common/content_message_generator.h:26: #include "content/common/media_stream_messages.h" looks like you forgot to update this! http://codereview.chromium.org/7184010/diff/7001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/7184010/diff/7001/content/content_common.gypi#... content/content_common.gypi:146: 'common/media/media_stream_messages.h', alphabetize http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:12: static const media_stream::StreamDeviceInfoArray empty_stream_array = no good -- we avoid static non-basic data types (i.e., anything from std:: namespace) http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:31: VLOG(1) << "MediaStreamDispatcher::GenerateStream" << request_id << ")"; nit: you're missing a ( in your log message http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:33: requests_.push_back(Request(event_handler, request_id, next_ipc_id_)); what purpose does next_ipd_id_ serve? I don't see it being used anywhere other than sitting inside the Request struct http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:43: << ", {label = " << label; nit: you're not closing your { in your log message :) http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:97: Request& request = *it; nit: remove extra space http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:41: const media_stream::StreamDeviceInfoArray& audio_device_array( this isn't any better since it's too easy to have the original array go out of scope and Bad Things Happen. once again would having a method like IsAudioDeviceOpen(...) suit your purposes? if you absolutely have to have a copy of the array then pass in a StreamDeviceInfoArray* as an out-param http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:79: void OnStreamGenerationFailed(int requestId); requestId -> request_id
oh and don't forget BUG= and TEST= in your CL description!
Updated after Sherkus review. http://codereview.chromium.org/7184010/diff/7001/content/common/content_messa... File content/common/content_message_generator.h (right): http://codereview.chromium.org/7184010/diff/7001/content/common/content_messa... content/common/content_message_generator.h:26: #include "content/common/media_stream_messages.h" On 2011/06/17 17:42:13, scherkus wrote: > looks like you forgot to update this! Sorry. http://codereview.chromium.org/7184010/diff/7001/content/content_common.gypi File content/content_common.gypi (right): http://codereview.chromium.org/7184010/diff/7001/content/content_common.gypi#... content/content_common.gypi:146: 'common/media/media_stream_messages.h', On 2011/06/17 17:42:13, scherkus wrote: > alphabetize Done. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:12: static const media_stream::StreamDeviceInfoArray empty_stream_array = On 2011/06/17 17:42:13, scherkus wrote: > no good -- we avoid static non-basic data types (i.e., anything from std:: > namespace) removed. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:31: VLOG(1) << "MediaStreamDispatcher::GenerateStream" << request_id << ")"; On 2011/06/17 17:42:13, scherkus wrote: > nit: you're missing a ( in your log message Done. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:33: requests_.push_back(Request(event_handler, request_id, next_ipc_id_)); On 2011/06/17 17:42:13, scherkus wrote: > what purpose does next_ipd_id_ serve? > > I don't see it being used anywhere other than sitting inside the Request struct Send(new MediaStreamHostMsg_GenerateStream(routing_id(), next_ipc_id_++, components, security_origin)); It idenfies the requests from clients and maps them to responses in OnStreamGenerated or OnStreamFailed. The reason I did not use the request_id as it is in GenerateStream is to handle the not very likely case of Webkit and another client (pepper ?) calling GenerateStream at the same time with the same request_id. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:43: << ", {label = " << label; On 2011/06/17 17:42:13, scherkus wrote: > nit: you're not closing your { in your log message :) Done. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.cc:97: Request& request = *it; On 2011/06/17 17:42:13, scherkus wrote: > nit: remove extra space Done. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:41: const media_stream::StreamDeviceInfoArray& audio_device_array( On 2011/06/17 17:42:13, scherkus wrote: > this isn't any better since it's too easy to have the original array go out of > scope and Bad Things Happen. > > once again would having a method like IsAudioDeviceOpen(...) suit your purposes? > > if you absolutely have to have a copy of the array then pass in a > StreamDeviceInfoArray* as an out-param The purpose of these two methods are for the media filter later to be able to get the session_id that identifies an opened device so it can start/stop the device and set resolution etc.. For video this id is used in VideoCaptureImplManager::AddDevice - (VideoCaptureSesionId). At the moment it is only the session_id I know is needed so therefore I only keep getter methods for getting that. Also - the stream API states that there can be multiple devices in each list but only one selected video device. the index is therefor the index in this video_device_array. http://codereview.chromium.org/7184010/diff/7001/content/renderer/media/media... content/renderer/media/media_stream_dispatcher.h:79: void OnStreamGenerationFailed(int requestId); On 2011/06/17 17:42:13, scherkus wrote: > requestId -> request_id Hard do change an old habit... Done.
LGTM w/ one nit http://codereview.chromium.org/7184010/diff/16002/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/16002/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:36: void MediaStreamDispatcher::StopStream(const std::string &label) { & goes with type
Updated due to updated in http://codereview.chromium.org/7192007/ Also fixed last nit. Thanks http://codereview.chromium.org/7184010/diff/16002/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/16002/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:36: void MediaStreamDispatcher::StopStream(const std::string &label) { On 2011/06/21 00:12:01, scherkus wrote: > & goes with type Done.
LGTM with a few minor details to check/fix. http://codereview.chromium.org/7184010/diff/20012/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/20012/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:110: // index is the index in the video_array that have failed. Nit: has failed. Repeated more times in this file. http://codereview.chromium.org/7184010/diff/20012/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:111: DCHECK_GT(it->second.video_array.size(), static_cast<size_t>(index)); Is index 0-based or 1-based? Should this and similar ones be DCHECK_GE ?
Please ignore my last comment about GE/GT and indices. I've just realized it's right as it is now. I was reading it the other way around.
Updated to Chromium head. All needed prerequisites to this patch is now landed. Andrew , can you land this for me? Thanks Per http://codereview.chromium.org/7184010/diff/20012/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/20012/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:110: // index is the index in the video_array that have failed. On 2011/06/21 18:24:22, Leandro Graciá Gil wrote: > Nit: has failed. Repeated more times in this file. Done.
http://codereview.chromium.org/7184010/diff/25001/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/25001/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.h:51: typedef int IpcRequestId; nit: is it worth having a typedef? the Request struct itself stores it as an int I'll commit anyway but you can decide in a follow up patch if you want
Presubmit check for 7184010-25001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/common/content_message_generator.h Presubmit checks took 3.5s to calculate.
ah yes don't forget a TEST= and BUG= +jam for content/common/content_message_generator.h (and feel free to driveby)
Fixed Scherkus comment. Thanks Per http://codereview.chromium.org/7184010/diff/25001/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.h (right): http://codereview.chromium.org/7184010/diff/25001/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.h:51: typedef int IpcRequestId; On 2011/06/22 22:27:24, scherkus wrote: > nit: is it worth having a typedef? > > the Request struct itself stores it as an int > > I'll commit anyway but you can decide in a follow up patch if you want Done.
On 2011/06/22 22:30:25, scherkus wrote: > ah yes don't forget a TEST= and BUG= no no no! please don't add blank TEST= and BUG= lines, that's just bureaucratic! only add a bug line if you have a bug number, and only add a test line if you have specific instructions to test team. > > +jam for content/common/content_message_generator.h (and feel free to driveby)
lgtm with some tiny nits http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... File content/common/media/media_stream_messages.h (right): http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... content/common/media/media_stream_messages.h:21: IPC_ENUM_TRAITS(media_stream::MediaStreamType) nit: the convention is to put the enum_traits beside each other at the top http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:26: DCHECK_EQ(MessageLoop::current(), message_loop_); if you want to check that you're being called on the main thread, you can just do DCHECK(RenderThread::current()). it doesn't seem necessary to store message_loop_ pointers. http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h#new... ipc/ipc_message_utils.h:90: MediaStreamMsgStart, nit: we usually jsut add to the end
Fixed nits. Removed DCHECK of the thread calling functions in order for the unit test to work. Thanks Per http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... File content/common/media/media_stream_messages.h (right): http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... content/common/media/media_stream_messages.h:21: IPC_ENUM_TRAITS(media_stream::MediaStreamType) On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > nit: the convention is to put the enum_traits beside each other at the top Done. http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:26: DCHECK_EQ(MessageLoop::current(), message_loop_); DCHECK(RenderThread::current()) does not work in the unit test. Any ideas ? I will remove this DCHECK for now. On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > if you want to check that you're being called on the main thread, you can just > do DCHECK(RenderThread::current()). it doesn't seem necessary to store > message_loop_ pointers. http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h#new... ipc/ipc_message_utils.h:90: MediaStreamMsgStart, On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > nit: we usually jsut add to the end Done.
still lgtm On 2011/06/27 13:11:07, Per K wrote: > Fixed nits. Removed DCHECK of the thread calling functions in order for the unit > test to work. > > Thanks > Per > > http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... > File content/common/media/media_stream_messages.h (right): > > http://codereview.chromium.org/7184010/diff/32002/content/common/media/media_... > content/common/media/media_stream_messages.h:21: > IPC_ENUM_TRAITS(media_stream::MediaStreamType) > On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > > nit: the convention is to put the enum_traits beside each other at the top > > Done. > > http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... > File content/renderer/media/media_stream_dispatcher.cc (right): > > http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... > content/renderer/media/media_stream_dispatcher.cc:26: > DCHECK_EQ(MessageLoop::current(), message_loop_); > DCHECK(RenderThread::current()) does not work in the unit test. Any ideas ? > I will remove this DCHECK for now. this is a common problem. removing it seems fine. > > On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > > if you want to check that you're being called on the main thread, you can just > > do DCHECK(RenderThread::current()). it doesn't seem necessary to store > > message_loop_ pointers. > > http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h > File ipc/ipc_message_utils.h (right): > > http://codereview.chromium.org/7184010/diff/32002/ipc/ipc_message_utils.h#new... > ipc/ipc_message_utils.h:90: MediaStreamMsgStart, > On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > > nit: we usually jsut add to the end > > Done.
LGTM w/ a note about the RenderThread stuff http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... File content/renderer/media/media_stream_dispatcher.cc (right): http://codereview.chromium.org/7184010/diff/32002/content/renderer/media/medi... content/renderer/media/media_stream_dispatcher.cc:26: DCHECK_EQ(MessageLoop::current(), message_loop_); On 2011/06/27 13:11:09, Per K wrote: > DCHECK(RenderThread::current()) does not work in the unit test. Any ideas ? > I will remove this DCHECK for now. > > On 2011/06/23 18:27:26, John Abd-El-Malek wrote: > > if you want to check that you're being called on the main thread, you can just > > do DCHECK(RenderThread::current()). it doesn't seem necessary to store > > message_loop_ pointers. > You'll have to initialize a (Mock)RenderThread inside your unit tests Talk to henrika@ for details.
Try job failure for 7184010-40001 (retry) on linux_clang for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Change committed as 90752 |
