Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13355003:
Minor cleanups in MediaStreamCaptureIndicator. (Closed)
|
Created:
7 years, 8 months ago by Sergey Ulanov Modified:
7 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMinor cleanups in MediaStreamCaptureIndicator.
Fixed IsCapturingUserMedia() and IsBeingMirrored() to use WebContents pointer
to identify tabs.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192512
Patch Set 1 #
Total comments: 9
Patch Set 2 : #
Messages
Total messages: 14 (0 generated)
xians, ben: ping
On 2013/04/01 19:55:01, sergeyu wrote: > xians, ben: ping Serge, Sorry for the delay, we just came back from our Easter holiday. I am not sure if this change is safe, could you please do some simple tests, like opening multiple tabs with apprtc.appspot.com, allow the request, then right click on one tab, and select "Close other tabs", then close the browser, make sure you don't hit the DCHECK in ~MediaStreamCaptureIndicator(). If that works, please also try the option "Reopen closed tab", then close the browsers. Thanks, SX
On 2013/04/02 08:37:15, xians1 wrote: > On 2013/04/01 19:55:01, sergeyu wrote: > > xians, ben: ping > > Serge, SergeY > > Sorry for the delay, we just came back from our Easter holiday. > > I am not sure if this change is safe, could you please do some simple tests, > like opening multiple tabs with http://apprtc.appspot.com, allow the request, then > right click on one tab, and select "Close other tabs", then close the browser, > make sure you don't hit the DCHECK in ~MediaStreamCaptureIndicator(). > If that works, please also try the option "Reopen closed tab", then close the > browsers. Yes, it all works correctly. I'm not sure why you are concerned about that DCHECK - there is nothing in this CL that could break it. IsCapturingUserMedia() and IsBeingMirrored() are const, so these methods cannot affect what's in |usage_map_|. Removal of DoDevicesOpenedOnUIThread() and DoDevicesClosedOnUIThread is just a cosmetic change that doesn't affect how everything else works. > > Thanks, > SX
lgtm. miu, FYI on this refactoring. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:300: WebContents* const web_contents = I am trying to be careful here since I remember we were hit by a bug that some of the CaptureDevicesClosed() were not called in some special scenarios like several tabs went away at the same, for example. In those case we might leave the UI indicator while the tab has gone away.
lgtm Please address comments below before committing. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:300: WebContents* const web_contents = On 2013/04/03 19:19:11, xians1 wrote: > I am trying to be careful here since I remember we were hit by a bug that some > of the CaptureDevicesClosed() were not called in some special scenarios like > several tabs went away at the same, for example. In those case we might leave > the UI indicator while the tab has gone away. I'm concerned as well. Thanks for bringing me in on this code review, xians1@. FWICT, however, I don't think the changes to the IsXXX() methods will cause a problem. sergeyu: Please make sure this change doesn't regress http://crbug.com/171077. For more context, see the comments for the LookUpByKnownAlias() method and also https://codereview.chromium.org/12035046/. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.h:57: // Returns true if the render view is capturing user media (e.g., webcam s/render view/WebContents/ https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.h:61: // Returns true if the render view itself is being mirrored (e.g., a source of here too: s/render view/WebContents/
https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:300: WebContents* const web_contents = On 2013/04/03 19:19:11, xians1 wrote: > I am trying to be careful here since I remember we were hit by a bug that some > of the CaptureDevicesClosed() were not called in some special scenarios like > several tabs went away at the same, for example. In those case we might leave > the UI indicator while the tab has gone away. In this particular case |web_contents| must be still alive because calling code was getting render_process_id and render_view_id from WebContents. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:300: WebContents* const web_contents = On 2013/04/03 23:05:21, Yuri wrote: > On 2013/04/03 19:19:11, xians1 wrote: > > I am trying to be careful here since I remember we were hit by a bug that some > > of the CaptureDevicesClosed() were not called in some special scenarios like > > several tabs went away at the same, for example. In those case we might leave > > the UI indicator while the tab has gone away. > > I'm concerned as well. Thanks for bringing me in on this code review, xians1@. > > FWICT, however, I don't think the changes to the IsXXX() methods will cause a > problem. sergeyu: Please make sure this change doesn't regress > http://crbug.com/171077. I don't know how to repro it - do we have test extension for tab mirroring? It sucks that there are no tests for this case. If you are concerned that such a trivial CL can break something it's a sign that (1) there are not enough tests, and (2) this code is more complicated than it needs to be and must be refactored. > For more context, see the comments for the > LookUpByKnownAlias() method and also https://codereview.chromium.org/12035046/. Looking at this bug and the fix I don't think there is any chance that my CL breaks anything. The problem we have around this code is that in some cases WebContents* is used to identify renderer and in some other renderer ID pair is used for the same purpose. Mapping between these two identifiers breaks when tab is being destroyed. I'm working on another CL - it will remove |render_process_id| and |render_view_id| from this code, so WebContents* will be used to identify renderers. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.h:57: // Returns true if the render view is capturing user media (e.g., webcam On 2013/04/03 23:05:21, Yuri wrote: > s/render view/WebContents/ Done. https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.h:61: // Returns true if the render view itself is being mirrored (e.g., a source of On 2013/04/03 23:05:21, Yuri wrote: > here too: s/render view/WebContents/ Done.
ben: ping - please approve changes in chrome/browser/ui/tabs/tab_utils.cc .
https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/13355003/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:300: WebContents* const web_contents = On 2013/04/03 23:43:30, sergeyu wrote: > > FWICT, however, I don't think the changes to the IsXXX() methods will cause a > > problem. sergeyu: Please make sure this change doesn't regress > > http://crbug.com/171077. > > I don't know how to repro it - do we have test extension for tab mirroring? It's described in the bug. Yes, there are many test extensions. I would be happy to point you to ours and show you how to use them, or you can run TabCaptureApiTest.EndToEnd (in browser_tests). If you go the latter route, simply run browser_tests with the --run-manual flag, which IIRC will leave the browser window open for you with a tab capture session active so you can perform the repro steps. > It sucks that there are no tests for this case. If you are concerned that such a > trivial CL can break something it's a sign that (1) there are not enough tests, > and (2) this code is more complicated than it needs to be and must be > refactored. 1. It's on you to make a reasonable effort to test your changes regardless. 2. If you feel there are not enough tests, then please write them. I am not the original author of this code, but the ROI may be insufficient here. 3. Test cases do exist for tab capture. Yes there need to be more. We are very much aware of (and in fear of) this. Unfortunately, we are currently fighting uphill battles just to get the basic ones enabled. Example blocker: https://code.google.com/p/chromium/issues/detail?id=177163 > Looking at this bug and the fix I don't think there is any chance that my CL > breaks anything. Agreed. > Mapping between these two identifiers breaks when tab > is being destroyed. I'm working on another CL - it will remove > |render_process_id| and |render_view_id| from this code, so WebContents* will be > used to identify renderers. Nice! :) That would be very ideal, since this UI indicator is supposed to be 1:1 with a single tab (i.e., a single WebContents instance). Just be aware: You may be opening a huge refactoring "can of worms" here, since the render_view_ids run deep. Another possible gotcha: WebContents might be destroyed before you attempt to de-ref the pointer in MediaStreamCaptureIndicator on a CloseXXX() call. Best of luck, and feel free to use me as a resource for help/advice.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/13355003/9001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/13355003/9001
Message was sent while issue was closed.
Change committed as 192512 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
