Minor cleanups in MediaStreamCaptureIndicator.

chrome/browser/media/media_stream_capture_indicator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/media_stream_capture_indicator.h"
 
 #include "base/bind.h"
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 256 matching lines @@
 void MediaStreamCaptureIndicator::CaptureDevicesOpened(
     int render_process_id,
     int render_view_id,
     const content::MediaStreamDevices& devices,
     const base::Closure& close_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!devices.empty());
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&MediaStreamCaptureIndicator::DoDevicesOpenedOnUIThread,
+      base::Bind(&MediaStreamCaptureIndicator::AddCaptureDevices,
                  this, render_process_id, render_view_id, devices,
                  close_callback));
 }
 
 void MediaStreamCaptureIndicator::CaptureDevicesClosed(
     int render_process_id,
     int render_view_id,
     const content::MediaStreamDevices& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!devices.empty());
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&MediaStreamCaptureIndicator::DoDevicesClosedOnUIThread,
+      base::Bind(&MediaStreamCaptureIndicator::RemoveCaptureDevices,
                  this, render_process_id, render_view_id, devices));
 }
 
 
 bool MediaStreamCaptureIndicator::IsCapturingUserMedia(
-    int render_process_id, int render_view_id) const {
+    content::WebContents* web_contents) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  WebContents* const web_contents =

[no longer working on chromium, 2013/04/03 19:19:11]

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.

[miu, 2013/04/03 23:05:21]

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/.

[Sergey Ulanov, 2013/04/03 23:43:30]

In this particular case |web_contents| must be still alive because calling code
was getting render_process_id and render_view_id from WebContents.

[Sergey Ulanov, 2013/04/03 23:43:30]

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.
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.

[miu, 2013/04/04 00:33:06]

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.
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
Agreed.
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.

-      LookUpByKnownAlias(render_process_id, render_view_id);
-  if (!web_contents)
-    return false;
-
   UsageMap::const_iterator it = usage_map_.find(web_contents);
   return (it != usage_map_.end() &&
           (it->second->IsCapturingAudio() || it->second->IsCapturingVideo()));
 }
 
 bool MediaStreamCaptureIndicator::IsBeingMirrored(
-    int render_process_id, int render_view_id) const {
+    content::WebContents* web_contents) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  WebContents* const web_contents =
-      LookUpByKnownAlias(render_process_id, render_view_id);
-  if (!web_contents)
-    return false;
-
   UsageMap::const_iterator it = usage_map_.find(web_contents);
   return it != usage_map_.end() && it->second->IsMirroring();
 }
 
-void MediaStreamCaptureIndicator::DoDevicesOpenedOnUIThread(
-    int render_process_id,
-    int render_view_id,
-    const content::MediaStreamDevices& devices,
-    const base::Closure& close_callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  AddCaptureDevices(render_process_id, render_view_id, devices, close_callback);
-}
-
-void MediaStreamCaptureIndicator::DoDevicesClosedOnUIThread(
-    int render_process_id,
-    int render_view_id,
-    const content::MediaStreamDevices& devices) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  RemoveCaptureDevices(render_process_id, render_view_id, devices);
-}
-
 void MediaStreamCaptureIndicator::MaybeCreateStatusTrayIcon() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (status_icon_)
     return;
 
   // If there is no browser process, we should not create the status tray.
   if (!g_browser_process)
     return;
 
   StatusTray* status_tray = g_browser_process->status_tray();
@@ skipping 265 matching lines @@
   }
 
   UpdateNotificationUserInterface();
 }
 
 void MediaStreamCaptureIndicator::OnStopScreenCapture(
     const base::Closure& stop) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   stop.Run();
 }