Eliminate all UI thread decoding of extension images....

Eliminate all UI thread decoding of extension images. 

Except one, that is. We have one location we need to 
take a look at (I've added a comment). This changelist 
converts UI usage of DecodeImage on the UI thread to 
a revamped and simplified ImageLoadingTracker class. 

I plan on adding to GetFilePath a DCHECK for the File 
thread but decided to do so in another changelist, 
since it has a high likelyhood of flushing something 
out and be backed out because of that. This started 
out as issue 38521 (make infobar use cached icons) 
but grew in scope to just eliminate all UI thread 
access to DecodeImage and GetFilePath. 

BUG=http://crbug.com/38521 
TEST=None (extensions should work as before)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42471

[Finnur, 2010-03-19 04:29:58]

Looks scary but a lot is just plumbing after adding a param to a virtual pure
interface function.

[Aaron Boodman, 2010-03-19 05:00:36]

http://codereview.chromium.org/1075006/diff/1/4
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/1075006/diff/1/4#newcode118
chrome/browser/extensions/extension_action_context_menu_model.cc:118: tracker_ =
new ImageLoadingTracker(this, 1);
Could you share more code by moving this loading logic into ExtensionInstallUI
and passing an ExtensionResource instead of the bitmap?

That would presumably make ExtensionInstallUI a bit less janky too, since it
wouldn't be waiting for this read to complete before popping up.

http://codereview.chromium.org/1075006/diff/1/10
File chrome/browser/views/infobars/extension_infobar.cc (right):

http://codereview.chromium.org/1075006/diff/1/10#newcode95
chrome/browser/views/infobars/extension_infobar.cc:95: tracker_ = NULL;  // The
tracker only loads one image, so it is deleted after
There is a pattern I discovered recently that can make this less error-prone.

ImageLoadingTracker could use ScopedRunnableMethodFactory to send methods to the
file thread. Then you make the people who use ImageLoadingTracker have them as
class members instead of pointers.

SRMF guarantees that you won't get called back after destruction. So it would go
like this:

class ImageLoadingTracker {
 public:
  void PostTask() {
   ChromeThread::PostTask(ChromeThread::FILE,
factory_.NewRunnableMethod(&ImageLoadingTracker::Run, this, ...));
  }
 private:
  ScopedRunnableMethodFactory<ImageLoadingTracker> factory_;
};

class SomeClientClass : public ImageLoadingTracker::Observer {
 private:
  ImageLoadingTracker tracker_;
}

You don't need to do any bookkeeping to reset tracker_ to NULL or check
NULL-ness. SRMF does all that internally.

[asargent_no_longer_on_chrome, 2010-03-19 17:38:21]

http://codereview.chromium.org/1075006/diff/20001/21003
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/1075006/diff/20001/21003#newcode153
chrome/browser/extensions/extension_action_context_menu_model.cc:153:
ExtensionInstallUI client(BrowserList::GetLastActive()->profile());
Any chance that BrowserList::GetLastActive() will return a different Browser*
than it did in ExtensionActionContextMenuModel::ExecuteCommand ? Would anything
go wrong if it did?

http://codereview.chromium.org/1075006/diff/20001/21006
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/1075006/diff/20001/21006#newcode538
chrome/browser/extensions/extensions_ui.cc:538: }
Would it be worth adding an " else { NOTREACHED(); }" here?

http://codereview.chromium.org/1075006/diff/20001/21005
File chrome/browser/extensions/extensions_ui.h (right):

http://codereview.chromium.org/1075006/diff/20001/21005#newcode239
chrome/browser/extensions/extensions_ui.h:239: ImageLoadingTracker*
incognite_enable_image_tracker_;
nit: typo "incognite"

http://codereview.chromium.org/1075006/diff/20001/21008
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1075006/diff/20001/21008#newcode240
chrome/browser/views/browser_actions_container.h:240: friend class
ShowFolderMenuTask;
Wait, shouldn't friend declarations be in the protected or private section of a
class declaration?

[Finnur, 2010-03-23 18:25:42]

So, I've uploaded the latest version. I plan on resubmitting to the try bots
when LastKnownGood value catches up with the right version and start a final
test on the functionality that I changed, just to be sure I didn't regress
during cleanup. :)

As for the changes, some big ones (simplification) to the ImageLoadingTracker
and its API and the usage of ExtensionInstallUI changed a bit, due to the
simplification.

http://codereview.chromium.org/1075006/diff/20001/21003
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/1075006/diff/20001/21003#newcode153
chrome/browser/extensions/extension_action_context_menu_model.cc:153:
ExtensionInstallUI client(BrowserList::GetLastActive()->profile());
Doubt it, this triggers when you select a menu item (you are interacting with
the browser). We could pass in the browser pointer, but it seems like some of
the callers (gtk) would then just have to call BrowserList::GetLastActive().

http://codereview.chromium.org/1075006/diff/20001/21008
File chrome/browser/views/browser_actions_container.h (right):

http://codereview.chromium.org/1075006/diff/20001/21008#newcode240
chrome/browser/views/browser_actions_container.h:240: friend class
ShowFolderMenuTask;
Sure. Moved this down.

On 2010/03/19 17:38:21, Antony Sargent wrote:
> Wait, shouldn't friend declarations be in the protected or private section of
a
> class declaration?

[Aaron Boodman, 2010-03-23 19:10:55]

http://codereview.chromium.org/1075006/diff/64001/65017
File chrome/browser/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/1075006/diff/64001/65017#newcode45
chrome/browser/cocoa/extensions/browser_action_button.mm:45: tracker_(this) {
Does this need an ALLOW_THIS_IN_INITIALIZER ?

http://codereview.chromium.org/1075006/diff/64001/65026
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/1075006/diff/64001/65026#newcode184
chrome/browser/extensions/extension_install_ui.cc:184:
extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE);
Looks like you could share this code in a LoadImage() helper.

http://codereview.chromium.org/1075006/diff/64001/65026#newcode268
chrome/browser/extensions/extension_install_ui.cc:268: if (index ==
IDS_EXTENSION_INSTALL_PROMPT_TITLE) {
whoa there cowboy. Overloading a param named "index" this way to be an opaque
identifer is ... a bit unexpected.

I think it would be simpler to store the type of dialog as a member of this
class in each of the Confirm*() methods, and then check that here.

http://codereview.chromium.org/1075006/diff/64001/65007
File chrome/browser/extensions/image_loading_tracker.cc (right):

http://codereview.chromium.org/1075006/diff/64001/65007#newcode119
chrome/browser/extensions/image_loading_tracker.cc:119: void
ImageLoadingTracker::PostLoadImageTask(const ExtensionResource& resource,
Nit: I think you could simply this name to just LoadImage. From the caller's
point of view the posting and the tasking are implementation details. I also
think this class should just be called ExtensionImageLoader, but I understand
that is a larger change and shouldn't be done here.

http://codereview.chromium.org/1075006/diff/64001/65002
File chrome/browser/extensions/image_loading_tracker.h (right):

http://codereview.chromium.org/1075006/diff/64001/65002#newcode20
chrome/browser/extensions/image_loading_tracker.h:20: // track of whether the
view still cares about the icon loading.
It would be good to have an example usage here.

http://codereview.chromium.org/1075006/diff/64001/65002#newcode27
chrome/browser/extensions/image_loading_tracker.h:27: // if they need to access
it after this call.
You should make it clear that if the image could not be loaded for some reason
it might be NULL.

http://codereview.chromium.org/1075006/diff/64001/65002#newcode39
chrome/browser/extensions/image_loading_tracker.h:39: int index);
Recommend removing this param and changing OnImageLoaded(SkBitmap*, int index)
to OnImageLoaded(ExtensionResource, SkBitmap*, index).

ImageTrackingLoader can keep a counter it increments to provide index, which
allows the existing callers that rely on index to keep working.

But I think that ExtensionResource is a more useful thing for the client to get
in the callback in general.

http://codereview.chromium.org/1075006/diff/64001/65014
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/1075006/diff/64001/65014#newcode72
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:72: tracker_(this),
allow_this_in_initalizer ?

http://codereview.chromium.org/1075006/diff/64001/65023
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/1075006/diff/64001/65023#newcode192
chrome/common/extensions/extension.h:192: // ImageLoadingTracker, which uses the
File thread to decode.
Do we still have callers for these?

[Finnur, 2010-03-23 21:19:48]

http://codereview.chromium.org/1075006/diff/64001/65017
File chrome/browser/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/1075006/diff/64001/65017#newcode45
chrome/browser/cocoa/extensions/browser_action_button.mm:45: tracker_(this) {
Don't think we need ALLOW_THIS here (for the same reason as I stated in gtk
file).

http://codereview.chromium.org/1075006/diff/64001/65014
File chrome/browser/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/1075006/diff/64001/65014#newcode72
chrome/browser/gtk/browser_actions_toolbar_gtk.cc:72: tracker_(this),
The ALLOW_THIS macro is for working around a VC compiler warning and compiles to
nothing in other compilers. Given that this is a gtk specific class, I think we
don't need it.

http://codereview.chromium.org/1075006/diff/64001/65023
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/1075006/diff/64001/65023#newcode192
chrome/common/extensions/extension.h:192: // ImageLoadingTracker, which uses the
File thread to decode.
There are a few callers out there, all on the file thread. There is a mac class
calling it that I suspect can be replaced wholesale using this new scheme, but
that's better for a mac person to decide, implement and test.

On 2010/03/23 19:10:55, Aaron Boodman wrote:
> Do we still have callers for these?

[Aaron Boodman, 2010-03-23 21:29:15]

lgtm!

[asargent_no_longer_on_chrome, 2010-03-23 21:51:15]

lgtm too