Add a class to replace ImageLoadingTracker with a nicer API.

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/utf_string_conversions.h"
 #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"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
@@ skipping 94 matching lines @@
     const MediaStreamCaptureIndicator::CaptureDeviceTab& tab) {
     return (render_process_id_ == tab.render_process_id &&
             render_view_id_ == tab.render_view_id);
 }
 
 MediaStreamCaptureIndicator::MediaStreamCaptureIndicator()
     : status_icon_(NULL),
       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() {
   // The user is responsible for cleaning up by closing all the opened devices.
   DCHECK(tabs_.empty());
 }
 
 bool MediaStreamCaptureIndicator::IsCommandIdChecked(
     int command_id) const {
   NOTIMPLEMENTED() << "There are no checked items in the MediaStream menu.";
@@ skipping 82 matching lines @@
   if (!g_browser_process)
     return;
 
   StatusTray* status_tray = g_browser_process->status_tray();
   if (!status_tray)
     return;
 
   status_icon_ = status_tray->CreateStatusIcon();
 
   EnsureStatusTrayIconResources();
-  EnsureImageLoadingTracker();
 }
 
 void MediaStreamCaptureIndicator::EnsureStatusTrayIconResources() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!mic_image_) {
     mic_image_ = ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
         IDR_INFOBAR_MEDIA_STREAM_MIC);
   }
   if (!camera_image_) {
     camera_image_ = ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
@@ skipping 18 matching lines @@
 
   int message_id = IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY_AUDIO_AND_VIDEO;
   if (audio && !video)
     message_id = IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY_AUDIO_ONLY;
   else if (!audio && video)
     message_id = IDS_MEDIA_STREAM_STATUS_TRAY_BALLOON_BODY_VIDEO_ONLY;
 
   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;
   }
 
   string16 title = l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
   string16 body = l10n_util::GetStringFUTF16(message_id,
       GetSecurityOrigin(render_process_id, render_view_id));
   status_icon_->DisplayBalloon(*balloon_image_, title, body);
 }
 
 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);
   status_icon_->DisplayBalloon(*image_skia, string16(), message);
 }
 
 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;
 
   // If there is no browser process, we should not do anything.
   if (!g_browser_process)
     return;
 
   StatusTray* status_tray = g_browser_process->status_tray();
   if (status_tray != NULL) {
@@ skipping 145 matching lines @@
 
 bool MediaStreamCaptureIndicator::IsProcessCapturing(int render_process_id,
                                                      int render_view_id) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   CaptureDeviceTabs::const_iterator iter = std::find_if(
       tabs_.begin(), tabs_.end(), TabEquals(render_process_id, render_view_id));
   if (iter == tabs_.end())
     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;
-}