Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20728002:
Fix issue: icons for the apps are flickering while launching. (Closed)
|
Created:
7 years, 5 months ago by zhchbin Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix issue: icons for the apps are flickering while launching.
The bug is introduced by the crrev.com/18369003, which fix issue 159302.
BUG=264645, 159302
TEST=When launching the unpacked app, the icons of the others shouldn't flicker.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215244
Patch Set 1 #Patch Set 2 : Nit fix. #
Total comments: 3
Patch Set 3 : Polish as comments. #
Total comments: 6
Patch Set 4 : Add comments. #Messages
Total messages: 24 (0 generated)
Please review.
https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... chrome/browser/resources/extensions/extension_list.js:197: new Date().getTime(); if the icon changes will this assignment happen before or after it completes? because if it's after.. this won't work.
On 2013/07/26 16:48:07, kalman wrote: > https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... > chrome/browser/resources/extensions/extension_list.js:197: new Date().getTime(); > if the icon changes will this assignment happen before or after it completes? > because if it's after.. this won't work. This line happen when the user click the reload link. And if the icon changes after this line, I think it's the responsibility of next click.
https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... chrome/browser/resources/extensions/extension_list.js:197: new Date().getTime(); On 2013/07/26 16:48:07, kalman wrote: > if the icon changes will this assignment happen before or after it completes? > because if it's after.. this won't work. However, I couldn't make sure I got your meaning of these words exactly. Could you please offer me more information about your case?
https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... chrome/browser/resources/extensions/extension_list.js:197: new Date().getTime(); On 2013/07/29 15:43:59, zhchbin wrote: > On 2013/07/26 16:48:07, kalman wrote: > > if the icon changes will this assignment happen before or after it completes? > > because if it's after.. this won't work. > > However, I couldn't make sure I got your meaning of these words exactly. Could > you please offer me more information about your case? Like this: 1. Extension changes its icon URL in the manifest. 2. Click reload 3. extensionReloadedIconURL caches the URL, but that includes the icon url. 4. extension is reloaded but uses the old value for extension.icon so it doesn't change can you try these steps with this patch?
On 2013/07/29 17:26:24, kalman wrote: > https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/20728002/diff/3001/chrome/browser/resources/e... > chrome/browser/resources/extensions/extension_list.js:197: new Date().getTime(); > On 2013/07/29 15:43:59, zhchbin wrote: > > On 2013/07/26 16:48:07, kalman wrote: > > > if the icon changes will this assignment happen before or after it > completes? > > > because if it's after.. this won't work. > > > > However, I couldn't make sure I got your meaning of these words exactly. Could > > you please offer me more information about your case? > > Like this: > > 1. Extension changes its icon URL in the manifest. > 2. Click reload > 3. extensionReloadedIconURL caches the URL, but that includes the icon url. > 4. extension is reloaded but uses the old value for extension.icon so it doesn't > change > > can you try these steps with this patch? The icon url is something like: chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1. It didn't change when the icon URL changed in the manifest. This is why it's cached in the issue 159302, the url changed in the manifest, but the icon url in the page "chrome://extensions" kept like: chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1. Exactly speaking, this cl is to fix issue 159302, also fix the issue 264645. Please help me confirm it on local.
icon.url is literally chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1? This would all be moot if we were caching the random get-string rather than the icon URL itself. Perhaps we should do that.
On 2013/07/30 01:53:40, kalman wrote: > icon.url is literally > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1? > > This would all be moot if we were caching the random get-string rather than the > icon URL itself. Perhaps we should do that. I don't think so. I think chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1 is by designed. Note that 48 is something related to the icon size. And there are a lot of code using the icon url like it.
On 2013/07/30 02:01:19, zhchbin wrote: > On 2013/07/30 01:53:40, kalman wrote: > > icon.url is literally > > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1? > > > > This would all be moot if we were caching the random get-string rather than > the > > icon URL itself. Perhaps we should do that. > > I don't think so. I think > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1 is by designed. > Note that 48 is something related to the icon size. And there are a lot of code > using the icon url like it. Yeah I had assumed that the icon URL was the... URL of the icon within the extension's directory, not the magical URL that chrome invents to pick a size. But either would make sense. Nevertheless are you sure that icon.url is *always* constant across reloads? Seems like probably, but is it documented as such?
On 2013/07/30 02:10:13, kalman wrote:
> On 2013/07/30 02:01:19, zhchbin wrote:
> > On 2013/07/30 01:53:40, kalman wrote:
> > > icon.url is literally
> > > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1?
> > >
> > > This would all be moot if we were caching the random get-string rather
than
> > the
> > > icon URL itself. Perhaps we should do that.
> >
> > I don't think so. I think
> > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1 is by
designed.
> > Note that 48 is something related to the icon size. And there are a lot of
> code
> > using the icon url like it.
>
> Yeah I had assumed that the icon URL was the... URL of the icon within the
> extension's directory, not the magical URL that chrome invents to pick a size.
> But either would make sense.
>
> Nevertheless are you sure that icon.url is *always* constant across reloads?
> Seems like probably, but is it documented as such?
Yes, I can. The icon URL comes from the ExtensionIconSource::GetIconURL. The
code are:
GURL icon_url(base::StringPrintf("%s%s/%d/%d%s",
chrome::kChromeUIExtensionIconURL,
extension->id().c_str(),
icon_size,
match,
grayscale ? "?grayscale=true" : ""));
So the icon.url is *always* constant across reloads.
On 2013/07/30 02:28:00, zhchbin wrote:
> On 2013/07/30 02:10:13, kalman wrote:
> > On 2013/07/30 02:01:19, zhchbin wrote:
> > > On 2013/07/30 01:53:40, kalman wrote:
> > > > icon.url is literally
> > > > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1?
> > > >
> > > > This would all be moot if we were caching the random get-string rather
> than
> > > the
> > > > icon URL itself. Perhaps we should do that.
> > >
> > > I don't think so. I think
> > > chrome://extension-icon/omdkgkgnnakfiojpcjdobjgdlpimkcbp/48/1 is by
> designed.
> > > Note that 48 is something related to the icon size. And there are a lot of
> > code
> > > using the icon url like it.
> >
> > Yeah I had assumed that the icon URL was the... URL of the icon within the
> > extension's directory, not the magical URL that chrome invents to pick a
size.
> > But either would make sense.
> >
> > Nevertheless are you sure that icon.url is *always* constant across reloads?
> > Seems like probably, but is it documented as such?
>
> Yes, I can. The icon URL comes from the ExtensionIconSource::GetIconURL. The
> code are:
>
> GURL icon_url(base::StringPrintf("%s%s/%d/%d%s",
> chrome::kChromeUIExtensionIconURL,
> extension->id().c_str(),
> icon_size,
> match,
> grayscale ? "?grayscale=true" : ""));
>
> So the icon.url is *always* constant across reloads.
I'm wondering if we want to tackle this in the C++. I think the application is
refreshing the page because it is creating a RenderViewHost, causing it to run
MaybeUpdateAfterNotification() and update the images [1]. Now, I don't think we
don't want to take out the refreshing on RenderViewHost creation entirely
because it handles "navigating from a non-extension page to an extension page
... as extension content being shown in popups and balloons" [2] which sounds
like valid functionality (this comment looks like it was blasted when they took
out the NAV_ENTRY_COMMITTED event being listened for).
@kalman
Should the page even be receiving a refresh request when an app is launched?
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
[2]
https://codereview.chromium.org/8353022/diff/12001/chrome/browser/ui/webui/op...
dnishi - good catch. That code looks suspicious to me, and not just because it's redundantly checking the correct profile all over the place. I just don't understand why it cares about render views in the *current webui page* from being deleted. Maybe the revision history would give a clue. In any case, it does look like this notification is the root of the problem: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... we appear to be refreshing the page whenever an extension host is created, which would include when an extension is launched (... or popup opened, etc). Presumably this is for the "inspect views" button in the devtools. Seems like a blunt hammer, a neater fix would be to have a look at what is going on there and refresh just what needs to be refreshed (e.g. in that case re-create the views). However, that's unfortunately rather complicated. Probably not worth it, despite being ideal. It still really irks me that we can't solve this properly, in the first place. E.g. extensions can request these favicon/icon URLs and I wonder whether they would have bugs similar to what the extension settings page is setting. Why can't we get the cache control right? I think I'm ok with this hack, as long as it's well documented. As I said I'd still rather be storing that Date.now() string in the map rather than the whole icon URL.
@kalman It does look like it is a combination of RenderViewCreated and NOTIFICATION_EXTENSION_HOST_CREATED causing refreshes on launch. I just tried out a build where I commented out line 473, 928-929 to stop it from updating on NOTIFICATION_EXTENSION_HOST_CREATED. It launched the application, causing the icons to flicker for the all launches including the first. When I commented out line 362 to stop it from updating on RenderViewCreated, it flickered once for the first launch and then didn't for subsequent launches. With lines 362, 473, and 928-929 commented out to stop it from updating on both, there is no flickering at all for the first or all subsequent launches. As for cache control, Jeffrey and I had a discussion about it in the comments of this code review link if you wanted to give that a read [1]. [1] https://codereview.chromium.org/16831012
> I think I'm ok with this hack, as long as it's well documented. As I said > I'd still rather be storing that Date.now() string in the map rather than > the whole icon URL. Has polished as @kalman's comment. I also felt vexed about this issue. You can find that several months ago I also want to fix the issue 159302... To sum up, AFAICS, this problem will only appear in the extensions/app developer side, and now we can fix it with a simple js hack. As you said, finding a neater fix is unfortunately rather complicated, can we fix the problem first of all and then fire a new issue to track the neater fix? By the way, the problem of issue 159302 can also be reproduced while using App Developer Tool.
lgtm https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:108: if (extensionReloadedTimestamp[extension.id]) { reference bug number(s) in a comment here https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:196: extensionReloadedTimestamp[extension.id] = new Date().getTime(); Date.now() should do it.
lgtm
https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:196: extensionReloadedTimestamp[extension.id] = new Date().getTime(); Actually, I just had a thought. Suppose an extension reloaded itself using the management API and the extension settings handler updates the information on the page. Technically, we never clicked the reload buttons, so the timestamp query string never gets updated. Wouldn't this mean that, in this case, we've reloaded the extension without sending a GET for its potentially updated image?
https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:196: extensionReloadedTimestamp[extension.id] = new Date().getTime(); On 2013/08/01 18:30:19, Daniel Nishi wrote: > Actually, I just had a thought. > > Suppose an extension reloaded itself using the management API and the extension > settings handler updates the information on the page. Technically, we never > clicked the reload buttons, so the timestamp query string never gets updated. > Wouldn't this mean that, in this case, we've reloaded the extension without > sending a GET for its potentially updated image? The same would apply for the developerPrivate API I believe. Yes that would imply that the image doesn't get updated properly. Maybe you could add a note for this; we could store and access that data in C++ from there (via the EXTENSION_FOO_NOTIFICATION, whichever one it is). In fact that would be more robust; just more effort. Since the user isn't *active* only the chrome://extensions page at that point (probably) I'm happy with just documenting the issue there.
On 2013/08/01 18:46:18, kalman wrote: > https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... > File chrome/browser/resources/extensions/extension_list.js (right): > > https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_list.js:196: > extensionReloadedTimestamp[extension.id] = new Date().getTime(); > On 2013/08/01 18:30:19, Daniel Nishi wrote: > > Actually, I just had a thought. > > > > Suppose an extension reloaded itself using the management API and the > extension > > settings handler updates the information on the page. Technically, we never > > clicked the reload buttons, so the timestamp query string never gets updated. > > Wouldn't this mean that, in this case, we've reloaded the extension without > > sending a GET for its potentially updated image? > > The same would apply for the developerPrivate API I believe. > > Yes that would imply that the image doesn't get updated properly. Maybe you > could add a note for this; we could store and access that data in C++ from there > (via the EXTENSION_FOO_NOTIFICATION, whichever one it is). In fact that would be > more robust; just more effort. > > Since the user isn't *active* only the chrome://extensions page at that point > (probably) I'm happy with just documenting the issue there. Sounds good. lgtm with that small issue documented.
https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:108: if (extensionReloadedTimestamp[extension.id]) { On 2013/08/01 15:22:01, kalman wrote: > reference bug number(s) in a comment here Done. https://codereview.chromium.org/20728002/diff/18001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_list.js:196: extensionReloadedTimestamp[extension.id] = new Date().getTime(); On 2013/08/01 15:22:01, kalman wrote: > Date.now() should do it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/20728002/28001
Retried try job too often on linux_aura for step(s) browser_tests, content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/20728002/28001
Message was sent while issue was closed.
Change committed as 215244 |
