Use ImageLoader instead of ImageLoadingTracker (Part 12)

Use ImageLoader instead of ImageLoadingTracker (Part 12)

Update extensions::TabHelper class to use new ImageLoader class.

BUG=163929
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178981

[rpaquay, 2013-01-16 20:18:31]

Last CL related to removing uses of deprecated ImageLoadingTracker class.

The funny business with "call_id" was needed to ensure we would handle
consecutive calls to "UpdateExtensionAppIcon". In the previous version of the
code, each call to "UpdateExtensionAppIcon" would create a new instance of the
loading tracker to make sure only the last OnImageLoaded was processed.

For example:
 UpdateExtensionAppIcon(Ext1);
 UpdateExtensionAppIcon(Ext2);
 UpdateExtensionAppIcon(NULL);

When the OnImageLoaded callbacks are called, only the last one should do actual
work.

It is not clear to me why/if this behavior is really needed given the current
set of callers, which is why the new code preserves the old behavior.

[asargent_no_longer_on_chrome, 2013-01-18 23:52:20]

Marijn had a good idea for how to handle this - add an additional WeakPtrFactory
member variable that you just use for the ImageLoader:

class TabHelper  { ....  

 private:
  // Used to help us cancel outstanding image loads.
  WeakPtrFactor<TabHelper> images_ptr_factory_;

...

}


const Extension* TabHelper::GetExtension(...) {
  ...
  if (extension) {
    images_ptr_factory_.InvalidateWeakPtrs();
    // Now use images_ptr_factory_.GetWeakPtr() instead of AsWeakPtr()
    // when base::bind'ing your call to loader->LoadImageAsync
    ...
  }


It's still an extra member variable, but it has the nice property of
"cancelling" any outstanding image load so you don't have to manually ignore
them in OnImageLoaded yourself.

[rpaquay, 2013-01-19 00:59:26]

On 2013/01/18 23:52:20, Antony Sargent wrote:
> Marijn had a good idea for how to handle this - add an additional
WeakPtrFactory
> member variable that you just use for the ImageLoader:
> 
> class TabHelper  { ....  
> 
>  private:
>   // Used to help us cancel outstanding image loads.
>   WeakPtrFactor<TabHelper> images_ptr_factory_;
> 
> ...
> 
> }
> 
> 
> const Extension* TabHelper::GetExtension(...) {
>   ...
>   if (extension) {
>     images_ptr_factory_.InvalidateWeakPtrs();
>     // Now use images_ptr_factory_.GetWeakPtr() instead of AsWeakPtr()
>     // when base::bind'ing your call to loader->LoadImageAsync
>     ...
>   }
> 
> 
> It's still an extra member variable, but it has the nice property of
> "cancelling" any outstanding image load so you don't have to manually ignore
> them in OnImageLoaded yourself.

Nice, yes, that works. Nit: I think the "Invalidate" call needs to be outside
the "if" to ensure 100% similar behavior as the previous code.

[rpaquay, 2013-01-22 20:16:19]

Address code review feedback...

[asargent_no_longer_on_chrome, 2013-01-23 01:02:30]

LGTM

https://codereview.chromium.org/11886073/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/11886073/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/tab_helper.h:264: base::WeakPtrFactory<TabHelper>
image_loader_ptr_factory_;
It might be worth adding a brief comment here that this is used to vend weak
pointers for image loading purposes which need to be invalidated when we want to
stop in-progress images loads.

[commit-bot: I haz the power, 2013-01-24 22:29:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/11886073/10002

[commit-bot: I haz the power, 2013-01-25 01:42:35]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-01-25 18:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/11886073/10002

[commit-bot: I haz the power, 2013-01-25 21:08:12]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-01-25 22:16:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/11886073/10002

[commit-bot: I haz the power, 2013-01-26 01:39:55]

Change committed as 178981