Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829190:
Resolve the problems where we can leak the system tray UI (Closed)
|
Created:
8 years, 4 months ago by no longer working on chromium Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWe can leak the system tray UI when closing down the tab due to several possibilities:
# An error happens on the video_capture_device and we erase the request without notifying the UI.
# When the tab is shutting down, it goes away before we we delete its entry in tabs_ in media_stream_capture_indicator
# When canceling a request in media_stream_device_settings, we search the next request and bring it up the UI before we delete the old request, in this case we might bring the the old request back to UI again.
And this patch also fixed a problem when hitting a DCHECK in media_stream_device_settings.cc(196)] Check failed: request->devices_full.count(stream_type) == static_cast<size_t>(0) (1 vs. 0)
This happened because calling devices_full[type].empty() will create an empty entry to the map.
BUG=140441
Test=content_unittests,chrome/test/functional/webrtc_brutality_test.py
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151486
Patch Set 1 #Patch Set 2 : addressed more relevant problems, ready for review now. #
Total comments: 33
Patch Set 3 : added dispatcher_host_unittests and addressed Perk's and Magnus' comments. #
Total comments: 27
Patch Set 4 : addressed the comments and added unit tests to media_stream_device_settings_unittest #
Total comments: 8
Patch Set 5 : rebased and addressed the comments. #
Total comments: 18
Patch Set 6 : addressed Andrew's comments. #Patch Set 7 : fixed testbots. #Messages
Total messages: 19 (0 generated)
Hi Wei and Magnus, could you please take a quick look at the CL? BR, /SX
Please review. FYI, we need to merge the code to the branch.
Looks good. Can you please also add unit tests. Ex Test OnChannelClosing in MediaStreamDispatcherHostTest when a stream have been generated. Test OnChannelClosing while a stream is beeing generated. Add tests for MediaStreamDeviceSettingsTest http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:117: StreamOptions options; options_ http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:119: // Note, never call devices_full[stream_type] before making sure the entry nit- this comment seems irrelevant in this class http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:121: DeviceMap devices_full; device_full_ http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:124: bool posted_task; posted_task_ http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:186: // TODO(xians): Post a cancel request on UI thread to dismiss the infobar Is this todo rellevant still? Sounds important. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:230: // Used to fake UI, which is needed for server based testing. Can this whole else case be moved to a new function ie if (use_fake_ui) { HandleFakeUi return; } ... no normal handling here. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:244: // video capture devices are already opened, |in_use| isn't set for Is this comment valid? Can't you open the same device multiple times now? http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:247: if (it->second.begin() != it->second.end() && How about using a temp variable above instead of it? Ie const DeviceMap& device_map = it->second; and here if (!device_map.empty() && device_it == device_map.end()) { ... http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:505: // We don't erase the |requests_| here so that we can update the UI ? devices or requests? This previousely did devices->erase(device_it) http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:664: for (StreamDeviceInfoArray::const_iterator it = Why do you need to check both the state and each device? Seems better to check the state ? And the state is a vector. Isn't it enough to just loop through all the states and don't care about the stream_type? http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:680: request.video_devices.begin(); it != request.video_devices.end(); dito?
http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:104: !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)) Since it is counting, I would prefer '== 0' rather than '!'. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:167: if (requests_.find(label) != requests_.end()) { This should never happen, can switch to a DCHECK instead? http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:193: std::string new_label = FindReadyRequestForView(render_view_id, This should only be done if the removed request has been posted to the ui, to avoid posting two infobars at the same time. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:230: // Used to fake UI, which is needed for server based testing. On 2012/08/09 07:31:14, perkj wrote: > Can this whole else case be moved to a new function ie > if (use_fake_ui) { > HandleFakeUi > return; > } > ... > no normal handling here. +1, this would be nice to hide. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:244: // video capture devices are already opened, |in_use| isn't set for On 2012/08/09 07:31:14, perkj wrote: > Is this comment valid? Can't you open the same device multiple times now? This was to be able to open several devices before the UI existed and there were any selection. It should be moved inside the if below, and possibly rewritten, considering the new code. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:508: } else { A comment regarding the request is not done?
Hi, I think I have already addressed Per and Magnus' comments. And also added unittests for OnChannelClosing(). I will look into adding unittest to media_stream_device_settings tomorrow. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:104: !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)) On 2012/08/09 09:57:05, mflodman wrote: > Since it is counting, I would prefer '== 0' rather than '!'. Hope this is fine. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:117: StreamOptions options; On 2012/08/09 07:31:14, perkj wrote: > options_ Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:119: // Note, never call devices_full[stream_type] before making sure the entry On 2012/08/09 07:31:14, perkj wrote: > nit- this comment seems irrelevant in this class It is relevant. One of the bug we are fixing is introduced by calling devices_full_[type].empty() which creates an empty entry on the map. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:121: DeviceMap devices_full; On 2012/08/09 07:31:14, perkj wrote: > device_full_ Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:124: bool posted_task; On 2012/08/09 07:31:14, perkj wrote: > posted_task_ Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:167: if (requests_.find(label) != requests_.end()) { On 2012/08/09 09:57:05, mflodman wrote: > This should never happen, can switch to a DCHECK instead? Good point. Done http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:186: // TODO(xians): Post a cancel request on UI thread to dismiss the infobar On 2012/08/09 07:31:14, perkj wrote: > Is this todo rellevant still? Sounds important. No, they are not relevant to the issues we are fixing here. This should be addressed by a separate CL. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:193: std::string new_label = FindReadyRequestForView(render_view_id, On 2012/08/09 09:57:05, mflodman wrote: > This should only be done if the removed request has been posted to the ui, to > avoid posting two infobars at the same time. Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:230: // Used to fake UI, which is needed for server based testing. On 2012/08/09 07:31:14, perkj wrote: > Can this whole else case be moved to a new function ie > if (use_fake_ui) { > HandleFakeUi > return; > } > ... > no normal handling here. Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:244: // video capture devices are already opened, |in_use| isn't set for On 2012/08/09 09:57:05, mflodman wrote: > On 2012/08/09 07:31:14, perkj wrote: > > Is this comment valid? Can't you open the same device multiple times now? > This was to be able to open several devices before the UI existed and there were > any selection. It should be moved inside the if below, and possibly rewritten, > considering the new code. > No idea if the comment is valid or not. If will be great if someone can verify it and tell me how this should be changed. Done with moving the comments into the if case. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:247: if (it->second.begin() != it->second.end() && On 2012/08/09 07:31:14, perkj wrote: > > How about using a temp variable above instead of it? > Ie > const DeviceMap& device_map = it->second; > and here > if (!device_map.empty() && device_it == device_map.end()) { > ... > > > How about it->second.size() != 0 && device_it == it->second.end() It fits in one line and does not need an temp variable. and for some unknown reason empty() behaves differently with .size(), using it destroys the logic and media_stream_dispatcher_host_unittests failed by hitting a DCHECK. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:505: // We don't erase the |requests_| here so that we can update the UI On 2012/08/09 07:31:14, perkj wrote: > ? devices or requests? This previousely did devices->erase(device_it) Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:508: } else { On 2012/08/09 09:57:05, mflodman wrote: > A comment regarding the request is not done? Done. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:664: for (StreamDeviceInfoArray::const_iterator it = On 2012/08/09 07:31:14, perkj wrote: > Why do you need to check both the state and each device? > Seems better to check the state ? > > And the state is a vector. Isn't it enough to just loop through all the states > and don't care about the stream_type? I honestly never understand why we have the state and in_use flag. Maybe Magnus and Wei can answer if it is enough to check the state here. And we need to check the state, previously we can return true even though the audio/video_devices is empty. http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:680: request.video_devices.begin(); it != request.video_devices.end(); On 2012/08/09 07:31:14, perkj wrote: > dito? Please see reply above.
http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:109: return false; combine both if above. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:283: it->second->posted_task_) { this looks weird, some members have "_" while others don't. it looks like MediaStreamDeviceSettingsRequest should be struct, instead of class. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:342: it != request->devices_full_.end(); ++it) { indent. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:355: // video capture devices are already opened, |in_use| isn't set for is this comment still valid? http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:365: // Post result and delete request. ditto. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:369: weak_ptr_factory_.GetWeakPtr(), label, devices_to_use)); is this safe? POstResponse will call PostRequestToUi in some cases. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:322: // |label| before it would receive any event. remove the comments about posting task. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:520: } why this "else if" format change?
http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:229: PostRequestToFakeUI(label); nit: usually you should do the short exit first. ie if(use_fake_ui) { PostRequestToFakeUI return; } if (IsUiBusy( .... ... http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:324: EXPECT_CALL(*media_observer_.get(), OnCaptureDevicesClosed(_, _, _)); Move this to just before OnStopGeneratedStream http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:380: EXPECT_CALL(*media_observer_.get(), OnCaptureDevicesClosed(_, _, _)) Move this expect to just before host->OnChannelClosing. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:386: size_t pending_streams = 3; No need to test CancelPendingStreams again. You have a test above for that.
http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/2002/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:664: for (StreamDeviceInfoArray::const_iterator it = Agree with Per. It should be enough to check the state for all types. It should be done for stated Done, Error or if it's not requested. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:355: // video capture devices are already opened, |in_use| isn't set for On 2012/08/10 01:23:13, wjia wrote: > is this comment still valid? Think it should be removed. Or explained in a general since it's now general and not video specific. Right? http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:665: for (StreamDeviceInfoArray::const_iterator it = See comment in patch set #2.
Please take another look. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:109: return false; On 2012/08/10 01:23:13, wjia wrote: > combine both if above. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:229: PostRequestToFakeUI(label); On 2012/08/10 07:09:31, perkj wrote: > nit: usually you should do the short exit first. > ie > if(use_fake_ui) { > PostRequestToFakeUI > return; > } > if (IsUiBusy( .... > ... > > > Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:283: it->second->posted_task_) { On 2012/08/10 01:23:13, wjia wrote: > this looks weird, some members have "_" while others don't. it looks like > MediaStreamDeviceSettingsRequest should be struct, instead of class. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:342: it != request->devices_full_.end(); ++it) { On 2012/08/10 01:23:13, wjia wrote: > indent. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:355: // video capture devices are already opened, |in_use| isn't set for On 2012/08/10 07:15:44, mflodman wrote: > On 2012/08/10 01:23:13, wjia wrote: > > is this comment still valid? > Think it should be removed. Or explained in a general since it's now general and > not video specific. Right? Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:365: // Post result and delete request. On 2012/08/10 01:23:13, wjia wrote: > ditto. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:369: weak_ptr_factory_.GetWeakPtr(), label, devices_to_use)); On 2012/08/10 01:23:13, wjia wrote: > is this safe? POstResponse will call PostRequestToUi in some cases. Thanks for pointing out. we need to handle the fakeUI case in PostResponse. I addressed this in the new unittest. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:324: EXPECT_CALL(*media_observer_.get(), OnCaptureDevicesClosed(_, _, _)); On 2012/08/10 07:09:31, perkj wrote: > Move this to just before OnStopGeneratedStream Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:380: EXPECT_CALL(*media_observer_.get(), OnCaptureDevicesClosed(_, _, _)) On 2012/08/10 07:09:31, perkj wrote: > Move this expect to just before host->OnChannelClosing. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:386: size_t pending_streams = 3; On 2012/08/10 07:09:31, perkj wrote: > No need to test CancelPendingStreams again. You have a test above for that. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:322: // |label| before it would receive any event. On 2012/08/10 01:23:13, wjia wrote: > remove the comments about posting task. Done. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:520: } On 2012/08/10 01:23:13, wjia wrote: > why this "else if" format change? It is more readable in this way, because: The first "if" is checking the it->second.state[stream_type], and the second "else if" is checking the sizes of the devices, they don't belong to the same scope, so it should be better to separate them. http://codereview.chromium.org/10829190/diff/1003/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_manager.cc:665: for (StreamDeviceInfoArray::const_iterator it = On 2012/08/10 07:15:44, mflodman wrote: > See comment in patch set #2. After discussing offline, we decided to leave it as it is now, and remove the in_use flag on a separate CL.
Awsome (except for MediaStreamManager::DevicesAccepted that I don't get. Consider writing a TODO and explain why it is done this way) LGTM http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:143: // We have got all the requested devices, it is ready if it has not nit: already been posted http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings_unittest.cc (right): http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings_unittest.cc:116: device_settings_->RemovePendingCaptureRequest(label); anything you can check here?
LGTM, nice CL Shijing!
looks good so far. http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:229: return; No need to have "return" here and above. It can be simply: if (use_fake_ui_) PostRequestToFakeUI(label); else if (!IsUiBusy(request->render_view_id, request->render_process_id)) PostRequestToUi(label); http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:257: } This portion of code is same as in RemovePendingCaptureRequest(). It's good to have a helper function, say ProcessNextReuqestForView(). Or you can move if(!new_label.empty()) {} into FindReadyRequestForView and rename it to ProcessNextReuqestForView.
Andrew, could you please take a quick look at the media_stream_capture_indicator.cc? http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:143: // We have got all the requested devices, it is ready if it has not On 2012/08/10 12:26:58, perkj wrote: > nit: already been posted Done. http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:229: return; On 2012/08/12 16:05:45, wjia wrote: > No need to have "return" here and above. It can be simply: > if (use_fake_ui_) > PostRequestToFakeUI(label); > else if (!IsUiBusy(request->render_view_id, request->render_process_id)) > PostRequestToUi(label); Done. http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings.cc:257: } On 2012/08/12 16:05:45, wjia wrote: > This portion of code is same as in RemovePendingCaptureRequest(). It's good to > have a helper function, say ProcessNextReuqestForView(). Or you can move > if(!new_label.empty()) {} into FindReadyRequestForView and rename it to > ProcessNextReuqestForView. Done. http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... File content/browser/renderer_host/media/media_stream_device_settings_unittest.cc (right): http://codereview.chromium.org/10829190/diff/1004/content/browser/renderer_ho... content/browser/renderer_host/media/media_stream_device_settings_unittest.cc:116: device_settings_->RemovePendingCaptureRequest(label); On 2012/08/10 12:26:58, perkj wrote: > anything you can check here? Not really. This test was made as a regression test for a bug which crashed the browser when reloading the page right after generating the stream. And this test just crashed when that problem happened.
lgtm
LGTM w/ nits (I admit I'm not entirely familiar w/ this code so I'm assuming the other reviewers have covered that part of the review!) http://codereview.chromium.org/10829190/diff/13003/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10829190/diff/13003/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:337: // Delete the entry since the tab has gone away. nit: this comment is somewhat of a duplicate of the comment immediately preceding -- merge them? http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:141: return false; please use {} for if-statements spanning >1 lines http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:214: if (IsRequestReadyForView(request)) { nit: prefer if-and-early-return instead of deeply nested ifs http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:294: if (!new_label.empty()) { ditto for early return http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings.h (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.h:85: void PostRequestToFakeUI(const std::string& label); nit: there's a mix of UI and Ui in this class -- which one should it be? http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings_unittest.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings_unittest.cc:80: // Setup the dummy available device for the request. this block is over-indented by 2 spaces http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:610: + it->second.video_devices.size() <= 1) { nit: + operator should go on end of previous line I'd also recommend surrounding the arithmetic with ( ) http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:765: return false; {} for if statements > 1 line http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:780: return false; ditto
Great thanks to Andrew. I am going to land it now. http://codereview.chromium.org/10829190/diff/13003/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10829190/diff/13003/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:337: // Delete the entry since the tab has gone away. On 2012/08/13 19:51:22, scherkus wrote: > nit: this comment is somewhat of a duplicate of the comment immediately > preceding -- merge them? Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:141: return false; On 2012/08/13 19:51:22, scherkus wrote: > please use {} for if-statements spanning >1 lines Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:214: if (IsRequestReadyForView(request)) { On 2012/08/13 19:51:22, scherkus wrote: > nit: prefer if-and-early-return instead of deeply nested ifs Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.cc:294: if (!new_label.empty()) { On 2012/08/13 19:51:22, scherkus wrote: > ditto for early return Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings.h (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings.h:85: void PostRequestToFakeUI(const std::string& label); On 2012/08/13 19:51:22, scherkus wrote: > nit: there's a mix of UI and Ui in this class -- which one should it be? Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_device_settings_unittest.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_device_settings_unittest.cc:80: // Setup the dummy available device for the request. On 2012/08/13 19:51:22, scherkus wrote: > this block is over-indented by 2 spaces Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:610: + it->second.video_devices.size() <= 1) { On 2012/08/13 19:51:22, scherkus wrote: > nit: + operator should go on end of previous line > > I'd also recommend surrounding the arithmetic with ( ) Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:765: return false; On 2012/08/13 19:51:22, scherkus wrote: > {} for if statements > 1 line Done. http://codereview.chromium.org/10829190/diff/13003/content/browser/renderer_h... content/browser/renderer_host/media/media_stream_manager.cc:780: return false; On 2012/08/13 19:51:22, scherkus wrote: > ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10829190/7005
Try job failure for 10829190-7005 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10829190/9005 |
