Add a class to replace ImageLoadingTracker with a nicer API.

chrome/browser/media/media_stream_capture_indicator.cc

Index: chrome/browser/media/media_stream_capture_indicator.cc
diff --git a/chrome/browser/media/media_stream_capture_indicator.cc b/chrome/browser/media/media_stream_capture_indicator.cc
index 30c196eaa440a59ca31748b3f564502991cee317..bed47589f227533e9a7b73e1b9e7f49a90c891e7 100644
--- a/chrome/browser/media/media_stream_capture_indicator.cc
+++ b/chrome/browser/media/media_stream_capture_indicator.cc
@@ -11,6 +11,7 @@
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/image_loader.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/status_icons/status_icon.h"
 #include "chrome/browser/status_icons/status_tray.h"
@@ -125,7 +126,7 @@ MediaStreamCaptureIndicator::MediaStreamCaptureIndicator()
       mic_image_(NULL),
       camera_image_(NULL),
       balloon_image_(NULL),
-      request_index_(0) {
+      icon_tracker_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {

[no longer working on chromium, 2012/11/09 19:07:10]

Without looking the details, just want to double check if this is an issue:
MediaStreamCaptureIndicator is created on the main thread, and it will destruct
the icon_tracker_ in UI thread, will this be a problem?

You can try it out by go to https://apprtc.appspot.com, click allow, then close
the browser.

[Marijn Kruisselbrink, 2012/11/10 00:19:19]

From trying this, it seems creation/usage is on the UI thread, but destruction
is on some other thread. And that can indeed be a problem. I guess I'll have to
come up with a solution without weak pointers then. I'm also not sure how
important it is that the DisplayBalloon in OnImageLoaded doesn't get called if
Hide() has been called before OnImageLoaded is invoked? If this happening is not
a problem I can just remove all the weak pointer stuff, otherwise I'll have to
add some "invalidated" flag (which should be thread safe, since all places where
this flag could be used already ensure they are on the UI thread).

[no longer working on chromium, 2012/11/13 15:17:29]

After Hide() is called, it means no tab is using the capture devices, so it
should not show any UI indicators like balloon or image.

[Marijn Kruisselbrink, 2012/11/14 17:35:27]

Okay, replaced the weak_ptr_factory (which would have had threading issues) with
a simple flag (which is safe since every method accessing it is always executed
on the UI thread).

 }
 
 MediaStreamCaptureIndicator::~MediaStreamCaptureIndicator() {
@@ -228,7 +229,6 @@ void MediaStreamCaptureIndicator::CreateStatusTray() {
   status_icon_ = status_tray->CreateStatusIcon();
 
   EnsureStatusTrayIconResources();
-  EnsureImageLoadingTracker();
 }
 
 void MediaStreamCaptureIndicator::EnsureStatusTrayIconResources() {
@@ -267,14 +267,22 @@ void MediaStreamCaptureIndicator::ShowBalloon(
   const extensions::Extension* extension =
       GetExtension(render_process_id, render_view_id);
   if (extension) {
-    pending_messages_[request_index_++] =
+    string16 message =
         l10n_util::GetStringFUTF16(message_id,
                                    UTF8ToUTF16(extension->name()));
-    tracker_->LoadImage(
+
+    WebContents* web_contents = tab_util::GetWebContentsByID(
+        render_process_id, render_view_id);
+
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());

[no longer working on chromium, 2012/11/09 19:07:10]

Check the web_contents before using it, in some cases it can be NULL.

[Marijn Kruisselbrink, 2012/11/10 00:19:19]

If the web_contents would be NULL, GetExtension would already have returned NULL
so we wouldn't even get to this code. But you're right that it would be good to
make that more explicit.

[no longer working on chromium, 2012/11/13 15:17:29]

True, extension should be enough to safe guard it, I just missed it. Please just
ignore my comment.

+
+    extensions::ImageLoader::Get(profile)->LoadImageAsync(
         extension,
         extension->GetIconResource(32, ExtensionIconSet::MATCH_BIGGER),
         gfx::Size(32, 32),
-        ImageLoadingTracker::CACHE);
+        base::Bind(&MediaStreamCaptureIndicator::OnImageLoaded,
+                   icon_tracker_.GetWeakPtr(), message));
     return;
   }
 
@@ -285,13 +293,8 @@ void MediaStreamCaptureIndicator::ShowBalloon(
 }
 
 void MediaStreamCaptureIndicator::OnImageLoaded(
-    const gfx::Image& image,
-    const std::string& extension_id,
-    int index) {
-  string16 message;
-  message.swap(pending_messages_[index]);
-  pending_messages_.erase(index);
-
+    const string16& message,
+    const gfx::Image& image) {
   const gfx::ImageSkia* image_skia = !image.IsEmpty() ? image.ToImageSkia() :
       ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
           IDR_APP_DEFAULT_ICON);
@@ -302,8 +305,8 @@ void MediaStreamCaptureIndicator::Hide() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(tabs_.empty());
 
-  // We have to destroy |tracker_| on the UI thread.
-  tracker_.reset();
+  // Make sure no more image loaded messages get delivered.
+  icon_tracker_.InvalidateWeakPtrs();
 
   if (!status_icon_)
     return;
@@ -469,13 +472,3 @@ bool MediaStreamCaptureIndicator::IsProcessCapturing(int render_process_id,
     return false;
   return (iter->audio_ref_count > 0 || iter->video_ref_count > 0);
 }
-
-void MediaStreamCaptureIndicator::EnsureImageLoadingTracker() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (tracker_.get())
-    return;
-
-  tracker_.reset(new ImageLoadingTracker(this));
-  pending_messages_.clear();
-  request_index_ = 0;
-}