Fix a bug where an extension's icon wasn't being properly reloaded if it changes

Fix a bug where an extension's icon wasn't being properly reloaded if it
changes

A query string, which is incremented whenever the extension
Reload button is clicked, is appended to the end of the
extension icon url in order to prevent caching.

Patch from Evan Peterson <evan.peterson.ep@gmail.com>;
BUG=159302
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182625

[cduvall, 2013-02-12 22:49:26]

This patch is from Evan, one of the new hires.

[not at google - send to devlin, 2013-02-12 22:57:52]

I think this might be symptomatic of a larger problem, where reloading an
extension doesn't always update its resources properly. I noticed when I was
writing an extension that the content script source wasn't always being picked
up.

Also: does this bug exist with the refresh-extensions-page-to-reload-extensions
feature? Does this fix it (well, I wouldn't expect it to fix it, but it might
not be a problem in the refresh case).

It seems like the general case might be to purge Chrome's cache of the extension
resources when the extension is reloaded, but I don't know how easy that would
be. Could you look into it briefly?

Sorry to complicate things...

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:39: // Increments each
time an extension is reloaded.
use jsdoc style

/**
 * @type {number} Appended to icon URL...
 */

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:105: (++this.iconQueryId_)
+ ')';
I think () around ++this.iconQueryId_ are unnecessary, also postincrement is
fine.

[epeterson, 2013-02-12 23:52:41]

Extension icons do appear to be updated properly: following the icon's URL
(chrome://extension-icon/<extension-id>/48/1) reveals the correct image after
reloading. Since this URL is never changed, the cached icon is reused.

There isn't a problem when refreshing the extensions page to reload extensions;
the icon is successfully updated.

The JS on the extensions page sends messages to the C++ using chrome.send (i.e.
chrome.send('extensionSettingsReload', [extension.id])).  A specialized message
could potentially be created to tell the C++ to clear the browser cache of the
extension's data, although I would have to look into exactly how to clear the
cache from within the C++.

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:39: // Increments each
time an extension is reloaded.
On 2013/02/12 22:57:52, kalman wrote:
> use jsdoc style
> 
> /**
>  * @type {number} Appended to icon URL...
>  */

Done.

https://codereview.chromium.org/12225147/diff/2001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:105: (++this.iconQueryId_)
+ ')';
On 2013/02/12 22:57:52, kalman wrote:
> I think () around ++this.iconQueryId_ are unnecessary, also postincrement is
> fine.

Done.

[not at google - send to devlin, 2013-02-13 01:59:31]

I didn't mean the specialized message thing.

However, see comment, perhaps it isn't a caching issue - if it were a caching
issue then I'd expect accessing it in other places not to work.

By the way, the patch description should be more like:

Fix a bug where an extension's icon wasn't being properly reloaded if it
changes.

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:107: this.iconQueryId_++ +
')';
Ah I see - is the problem just that the background image isn't changing, so the
DOM doesn't know that it need to reload?

Would something like this work:

var iconUrl = 'url(' + extension.icon + ')';
if (iconUrl == item.style.backgroundImage)
  delete item.style.backgroundImage;
item.style.backgroundImage = iconUrl;

if not, something like

var iconUrl = 'url(' + extension.icon + ')';
if (iconUrl == item.style.backgroundImage) {
  delete item.style.backgroundImage;
  setTimeout(function() {
    item.style.backgroundImage = iconUrl;
  }, 0);
} else {
  item.style.backgroundImage = iconUrl;
}

etc...

[not at google - send to devlin, 2013-02-13 02:05:27]

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:107: this.iconQueryId_++ +
')';
> 
> var iconUrl = 'url(' + extension.icon + ')';
> if (iconUrl == item.style.backgroundImage)
>   delete item.style.backgroundImage;
> item.style.backgroundImage = iconUrl;
> 

> 
> var iconUrl = 'url(' + extension.icon + ')';
> if (iconUrl == item.style.backgroundImage) {
>   delete item.style.backgroundImage;
>   setTimeout(function() {
>     item.style.backgroundImage = iconUrl;
>   }, 0);
> } else {
>   item.style.backgroundImage = iconUrl;
> }
> 

Ok I straight away realised this was dumb, since the iconUrl isn't going to
change, that's the point. What I *mean't* was like

delete item.style.backgroundImage;
item.style.backgroundImage = '(' + extension.icon + ')';

or

delete item.style.backgroundImage;
setTimeout(function() {
  item.style.backgroundImage = '(' + extension.icon + ')';
}, 0);

[epeterson, 2013-02-14 01:09:22]

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/12225147/diff/1002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:107: this.iconQueryId_++ +
')';
I've just tried these suggestions, and, unfortunately, neither of them appear to
be convincing the DOM that it needs to reload.

[not at google - send to devlin, 2013-02-14 01:13:34]

lgtm

[commit-bot: I haz the power, 2013-02-14 20:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12225147/1002

[commit-bot: I haz the power, 2013-02-14 22:22:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12225147/1002

[commit-bot: I haz the power, 2013-02-15 05:51:31]

Change committed as 182625