Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25050005:
Refactored loading of applications / extensions icons. (Closed)
|
Created:
7 years, 2 months ago by dvh Modified:
7 years, 2 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, asargent_no_longer_on_chrome Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRefactored loading of applications / extensions icons.
Added ImageLoader::LoadExtensionIconAsync() and ImageLoader::LoadExtensionIconDataURLAsync().
Moved all apps / extension loading code to ImageLoader.
BUG=297298, 297301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226823
Patch Set 1 #Patch Set 2 : #
Total comments: 61
Patch Set 3 : Fixes based on the feedback of finnur@ #
Total comments: 10
Patch Set 4 : More feedback. #Patch Set 5 : Added unit tests. #
Total comments: 17
Patch Set 6 : Additional feedbacks. #
Total comments: 1
Patch Set 7 : Improved comment. #Patch Set 8 : Merge with HEAD #Patch Set 9 : Added unit test data. #Patch Set 10 : Added MatchType parameter to LoadExtensionIconAsync() and LoadExtensionIconDataURLAsync(). #Patch Set 11 : #
Total comments: 2
Patch Set 12 : #Messages
Total messages: 22 (0 generated)
finnur@ for chrome/browser/ui/webui/extensions/* asargent@ for chrome/browser/extensions/* PTAL. Thanks!
https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:511: // We will iterator over |item_list_| to request all the icons. s/iterator/iterate/ Or just: s/We will iterator/Iterate/ https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:530: developer::ItemInfo* info = &*item_list_[icon_to_load_]; Hmm: &* What's wrong with .get() ? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:534: // Loop to request the next icon. nit: This comment is redundant. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:537: // All icons have been loaded: we send the result. nit: The word 'we' is redundant. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:145: // List of extensions/apps items to return. This serves more of a purpose than returning data. It also keeps track of the data needed to determine which icon(s) to load, no? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:150: unsigned int icon_to_load_; Style guide says don't use unsigned types. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:177: // Request icon the the extension/app at the index |icon_to_load_| in A question from the quick-and-dirty-haiku-dude: When code this fall lands, is request icon the the true proper English? ;) https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:181: // Called the icon requested byRequestNextIcon() has been loaded. nit: This also needs a bit of rewording. Suggest: Called when each icon, has loaded by RequestNextIcon(). |url| is the data url containing the icon. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:134: } This function is now both here and in extension_icon_source.cc. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:419: } style: Single line body, no braces needed. But under what circumstances would |extension| be NULL? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:501: // to avoid unnecessary conversions. It seems we're not consistent in skipping the FinalizeImage call. See for example LoadExtensionIconLoaded and LoadDefaultImageDone. Why not always call FinalizeImage and let it determine (based on |grayscale|) whether it needs to jump to the file thread or just call the callback directly? It seems like these functions shouldn't need to make a judgement call based on |grayscale|, in case Finalize does some more processing. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:20: #include "url/gurl.h" nit: Gurl and chrome::FaviconBitmapResult can not be forward declared instead (and the .h moved to the .cc file)? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:74: explicit ImageLoader(Profile* profile); Shouldn't this be below the static function below (IsComponent...)? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:109: callback); nit: indent callback some more? It looks like the fifth parameter (same below). https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:111: // Load the data URL of the icon of the given extension. The pre-existing API was pretty clear. You had two "load image asynchronously" functions, one that loads many at a time and the other that loads only one. Pretty self-explanatory. Now we have four very similar functions that all do slightly different tasks and it is not as clear which to pick just from looking at the header file. I'd like to know why these four variations are provided and be able to determine (from the comments) which one to pick. Additional question: I take it we plan to keep them all around forever (that this is not a transition period)? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:119: virtual ~ImageLoader(); Umm... what's the point of moving this into a separate public section at the bottom? Did you mean to make this private? If so, why not move it to the already existing private section below? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:122: base::WeakPtrFactory<ImageLoader> weak_ptr_factory_; nit: The indentation for this member and the next four looks wrong (it went from 2 to 3 spaces, when it should have only two) https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:145: // 1) It loads the icon image using LoadImageAsync() nit: End comments with period. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:149: void LoadExtensionIconLoaded(const extensions::Extension* extension, Weird name: LoadFooLoaded. LoadFooDone is better, IMHO. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:158: // 1) The icons as listed in the extension / app manifests. Extension is a superset of apps, when it comes to Extension pointers but I don't think listing (in the comments) the fact that extensions can be apps helps to clarify the comment. In fact, I suspect it does the opposite. I would just drop the words (or app) in these comments. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:167: // Loads the favicon image for the given app/extension associated. If the ... for the given |extension|. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:176: // an other fallback. type: s/an other/another/ https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:193: callback); nit: indent? Same above and below. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:220: // Process on the "finalize" operation on the file thread. nit: 'on' is redundant. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:245: // URL in the file thread. nit: s/in/on/ https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... File chrome/browser/ui/webui/extensions/extension_icon_source.cc (left): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... chrome/browser/ui/webui/extensions/extension_icon_source.cc:139: if (icon.relative_path().empty()) { What was the purpose of this code? Was it to prevent that an extension could ask to load arbitrary images on disk? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... File chrome/browser/ui/webui/extensions/extension_icon_source.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... chrome/browser/ui/webui/extensions/extension_icon_source.h:92: // ImageLoader::LoadExtensionIconAsync(). OnIconLoaded OnIconLoaded... what? :)
Finnur's in the OWNERS file for chrome/browser/extensions, so he can review the whole thing. =) I'll move myself to CC - Finnur, let me know if you need a second opinion on anything.
Hi finnur@, I did all the changes. There's still an opened question about the API. PTAL. Thanks! https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:511: // We will iterator over |item_list_| to request all the icons. On 2013/09/30 15:11:21, Finnur wrote: > s/iterator/iterate/ > Or just: > s/We will iterator/Iterate/ Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:530: developer::ItemInfo* info = &*item_list_[icon_to_load_]; On 2013/09/30 15:11:21, Finnur wrote: > Hmm: &* > What's wrong with .get() ? Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:534: // Loop to request the next icon. On 2013/09/30 15:11:21, Finnur wrote: > nit: This comment is redundant. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:537: // All icons have been loaded: we send the result. On 2013/09/30 15:11:21, Finnur wrote: > nit: The word 'we' is redundant. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:145: // List of extensions/apps items to return. On 2013/09/30 15:11:21, Finnur wrote: > This serves more of a purpose than returning data. It also keeps track of the > data needed to determine which icon(s) to load, no? Actually, this class will return the list of extensions/apps: the information related to a specific app/extension, including its icon. Do you think we need more comment? https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:150: unsigned int icon_to_load_; On 2013/09/30 15:11:21, Finnur wrote: > Style guide says don't use unsigned types. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:177: // Request icon the the extension/app at the index |icon_to_load_| in On 2013/09/30 15:11:21, Finnur wrote: > A question from the quick-and-dirty-haiku-dude: > > When code this fall lands, > is request icon the the > true proper English? > > ;) I'll try to. Fixing it. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:181: // Called the icon requested byRequestNextIcon() has been loaded. On 2013/09/30 15:11:21, Finnur wrote: > nit: This also needs a bit of rewording. Suggest: Called when each icon, has > loaded by RequestNextIcon(). |url| is the data url containing the icon. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:134: } On 2013/09/30 15:11:21, Finnur wrote: > This function is now both here and in extension_icon_source.cc. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:419: } On 2013/09/30 15:11:21, Finnur wrote: > style: Single line body, no braces needed. > > But under what circumstances would |extension| be NULL? Added a comment. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.cc:501: // to avoid unnecessary conversions. On 2013/09/30 15:11:21, Finnur wrote: > It seems we're not consistent in skipping the FinalizeImage call. See for > example LoadExtensionIconLoaded and LoadDefaultImageDone. Why not always call > FinalizeImage and let it determine (based on |grayscale|) whether it needs to > jump to the file thread or just call the callback directly? It seems like these > functions shouldn't need to make a judgement call based on |grayscale|, in case > Finalize does some more processing. It was the behavior of the existing code in extension_icon_source. Though, it's a good idea to simplify this code. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:20: #include "url/gurl.h" On 2013/09/30 15:11:21, Finnur wrote: > nit: Gurl and chrome::FaviconBitmapResult can not be forward declared instead > (and the .h moved to the .cc file)? Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:74: explicit ImageLoader(Profile* profile); On 2013/09/30 15:11:21, Finnur wrote: > Shouldn't this be below the static function below (IsComponent...)? Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:109: callback); On 2013/09/30 15:11:21, Finnur wrote: > nit: indent callback some more? It looks like the fifth parameter (same below). Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:111: // Load the data URL of the icon of the given extension. On 2013/09/30 15:11:21, Finnur wrote: > The pre-existing API was pretty clear. You had two "load image asynchronously" > functions, one that loads many at a time and the other that loads only one. > Pretty self-explanatory. > > Now we have four very similar functions that all do slightly different tasks and > it is not as clear which to pick just from looking at the header file. I'd like > to know why these four variations are provided and be able to determine (from > the comments) which one to pick. - One loads the icon of an extension (it's a specific resource of an app/extension) and will return a image. - The other one loads the icon of an extension and will return a image data URL from it. Do you have better naming for them / suggestion? > Additional question: I take it we plan to keep them all around forever (that > this is not a transition period)? This is not a transition period. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:119: virtual ~ImageLoader(); On 2013/09/30 15:11:21, Finnur wrote: > Umm... what's the point of moving this into a separate public section at the > bottom? Did you mean to make this private? If so, why not move it to the already > existing private section below? I moved it at the previous location, below ImageLoader(). https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:122: base::WeakPtrFactory<ImageLoader> weak_ptr_factory_; On 2013/09/30 15:11:21, Finnur wrote: > nit: The indentation for this member and the next four looks wrong (it went from > 2 to 3 spaces, when it should have only two) Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:145: // 1) It loads the icon image using LoadImageAsync() On 2013/09/30 15:11:21, Finnur wrote: > nit: End comments with period. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:149: void LoadExtensionIconLoaded(const extensions::Extension* extension, On 2013/09/30 15:11:21, Finnur wrote: > Weird name: LoadFooLoaded. LoadFooDone is better, IMHO. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:158: // 1) The icons as listed in the extension / app manifests. On 2013/09/30 15:11:21, Finnur wrote: > Extension is a superset of apps, when it comes to Extension pointers but I don't > think listing (in the comments) the fact that extensions can be apps helps to > clarify the comment. In fact, I suspect it does the opposite. I would just drop > the words (or app) in these comments. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:167: // Loads the favicon image for the given app/extension associated. If the On 2013/09/30 15:11:21, Finnur wrote: > ... for the given |extension|. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:176: // an other fallback. On 2013/09/30 15:11:21, Finnur wrote: > type: s/an other/another/ Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:193: callback); On 2013/09/30 15:11:21, Finnur wrote: > nit: indent? Same above and below. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:220: // Process on the "finalize" operation on the file thread. On 2013/09/30 15:11:21, Finnur wrote: > nit: 'on' is redundant. Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:245: // URL in the file thread. On 2013/09/30 15:11:21, Finnur wrote: > nit: s/in/on/ Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... File chrome/browser/ui/webui/extensions/extension_icon_source.cc (left): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... chrome/browser/ui/webui/extensions/extension_icon_source.cc:139: if (icon.relative_path().empty()) { On 2013/09/30 15:11:21, Finnur wrote: > What was the purpose of this code? Was it to prevent that an extension could ask > to load arbitrary images on disk? The equivalent is performed in this method: ImageLoader::LoadImagesOnBlockingPool(). https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... File chrome/browser/ui/webui/extensions/extension_icon_source.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/ui/webui/ex... chrome/browser/ui/webui/extensions/extension_icon_source.h:92: // ImageLoader::LoadExtensionIconAsync(). OnIconLoaded On 2013/09/30 15:11:21, Finnur wrote: > OnIconLoaded... what? :) Done.
https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:530: developer::ItemInfo* info = &*item_list_[icon_to_load_]; You changed line 518, but not 530... https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:145: // List of extensions/apps items to return. Yes. My point was that you use this data structure not just to return data but also to keep track of work that needs to be done (with the help of icon_to_load_). https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:111: // Load the data URL of the icon of the given extension. I don't think these need better names, necessarily. What is needed is more guidance for people who want to use this API since the comments for these two new functions are extremely skimpy on the details. I don't know if there ever will be doubt (for consumers of this class) which of these four functions is the right one for them, but it would be great if the comments could help them (people looking at this API) choose the right function. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:167: // Loads the favicon image for the given app/extension associated. If the 'Associated' is redundant. On 2013/10/01 04:19:26, dvh-g wrote: > On 2013/09/30 15:11:21, Finnur wrote: > > ... for the given |extension|. > > Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:533: if (icon_to_load_ < (int) item_list_.size()) { Style guide also says to use static_cast for this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.cc:410: // |extension| is NULL when we force loading of the default icon. nit: s/is/can be/ https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:91: nit: delete this extra line break? https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:139: scoped_ptr<SkBitmap> default_extension_data_; Can you rename these as: default_app_icon_ default_extension_icon_ ? 'Data' is a bit too generic here. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:245: // the icon of the app/extension. The following method will be called when nit: app/extension -> extension
- Apply fixes. - Added unit tests. PTAL! Thanks. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.cc:530: developer::ItemInfo* info = &*item_list_[icon_to_load_]; On 2013/10/01 11:14:15, Finnur wrote: > You changed line 518, but not 530... Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:145: // List of extensions/apps items to return. On 2013/10/01 11:14:15, Finnur wrote: > Yes. My point was that you use this data structure not just to return data but > also to keep track of work that needs to be done (with the help of > icon_to_load_). Done. https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/image_loader.h:167: // Loads the favicon image for the given app/extension associated. If the On 2013/10/01 11:14:15, Finnur wrote: > 'Associated' is redundant. > > On 2013/10/01 04:19:26, dvh-g wrote: > > On 2013/09/30 15:11:21, Finnur wrote: > > > ... for the given |extension|. > > > > Done. > Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:533: if (icon_to_load_ < (int) item_list_.size()) { On 2013/10/01 11:14:16, Finnur wrote: > Style guide also says to use static_cast for this: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.cc:410: // |extension| is NULL when we force loading of the default icon. On 2013/10/01 11:14:16, Finnur wrote: > nit: s/is/can be/ Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:91: On 2013/10/01 11:14:16, Finnur wrote: > nit: delete this extra line break? Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:139: scoped_ptr<SkBitmap> default_extension_data_; On 2013/10/01 11:14:16, Finnur wrote: > Can you rename these as: > default_app_icon_ > default_extension_icon_ > ? > > 'Data' is a bit too generic here. Done. https://codereview.chromium.org/25050005/diff/13001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:245: // the icon of the app/extension. The following method will be called when On 2013/10/01 11:14:16, Finnur wrote: > nit: app/extension -> extension Done.
Almost there. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/extension_icon_image_unittest.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/extension_icon_image_unittest.cc:119: extensions::ImageLoader* image_loader_; nit: scoped_ptr<extensions::ImageLoader> image_loader_ would result in fewer lines of code and less chance of accidental leak. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.cc:410: // |extension| can be NULL. In this case, force loading of the default icon. Originally I asked when it can be NULL. That part is still missing. The latter part of this comment should probably read: In this case, fail the icon loading, which will result in the default icon being loaded. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:116: // Load the icon of the given |extension|. The size in pixels of the returned nit: Add to the first sentence: and return it via |callback| as a gfx::Image. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:117: // icon can be chosen with |icon_size|. The icon can also be converted to Can be? Looks to me like it must be chosen. Unless you can pass in 0x0 or something, but that should then be documented. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader_unittest.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:60: quit_in_image_loaded_ = false; Do you know why it is necessary to set this to false here? Actually, why is this variable necessary? https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:105: GURL url_; Document: // Holds the image loaded by the test. and // Holds the data url retrieved by the test. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:115: bool quit_in_image_loaded_; Please rename: quit_when_image_loaded_. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:341: EXPECT_EQ(1, image_loaded_count()); We could test against an actual data URL. Or is that not advisable?
Applied some fixes and comments. PTAL. Thanks! https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.cc:410: // |extension| can be NULL. In this case, force loading of the default icon. On 2013/10/02 09:52:15, Finnur wrote: > Originally I asked when it can be NULL. That part is still missing. > > The latter part of this comment should probably read: In this case, fail the > icon loading, which will result in the default icon being loaded. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:116: // Load the icon of the given |extension|. The size in pixels of the returned On 2013/10/02 09:52:15, Finnur wrote: > nit: Add to the first sentence: and return it via |callback| as a gfx::Image. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:117: // icon can be chosen with |icon_size|. The icon can also be converted to On 2013/10/02 09:52:15, Finnur wrote: > Can be? Looks to me like it must be chosen. Unless you can pass in 0x0 or > something, but that should then be documented. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader_unittest.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:60: quit_in_image_loaded_ = false; On 2013/10/02 09:52:15, Finnur wrote: > Do you know why it is necessary to set this to false here? > Actually, why is this variable necessary? I don't think that the case where the it's really needed actually happen. I removed it. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:105: GURL url_; On 2013/10/02 09:52:15, Finnur wrote: > Document: > // Holds the image loaded by the test. > and > // Holds the data url retrieved by the test. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:105: GURL url_; On 2013/10/02 09:52:15, Finnur wrote: > Document: > // Holds the image loaded by the test. > and > // Holds the data url retrieved by the test. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:115: bool quit_in_image_loaded_; On 2013/10/02 09:52:15, Finnur wrote: > Please rename: quit_when_image_loaded_. Done. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:341: EXPECT_EQ(1, image_loaded_count()); On 2013/10/02 09:52:15, Finnur wrote: > We could test against an actual data URL. Or is that not advisable? Even though lossless, I think that PNG compression is not consistent, depending on the algorithm chosen. It would make the test flacky. A second issue is that the resize algorithm can give different pixel results depending on the algorithm too.
LGTM, with nits. https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... File chrome/browser/extensions/image_loader_unittest.cc (right): https://codereview.chromium.org/25050005/diff/29001/chrome/browser/extensions... chrome/browser/extensions/image_loader_unittest.cc:341: EXPECT_EQ(1, image_loaded_count()); Fair enough. https://codereview.chromium.org/25050005/diff/40001/chrome/browser/extensions... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/40001/chrome/browser/extensions... chrome/browser/extensions/image_loader.h:122: // |icon_size| is set to -1 if no resize is requested. Suggest minor reordering: Load the icon of the given |extension| and return it via |callback| as a gfx::Image. |extension| can be NULL to request the default extension icon. The size in pixels of the returned icon can be chosen with |icon_size| or -1,-1 if no resize is requested. The icon can also be converted to grayscale by setting |grayscale| to true.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/25050005/48001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/25050005/69001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
On 2013/10/02 16:55:03, I haz the power (commit-bot) wrote: > Retried try job too often on linux_aura for step(s) unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Added unit tests data.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/25050005/45001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2013/10/02 19:21:44, I haz the power (commit-bot) wrote: > Retried try job too often on linux_rel for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Hi finnur@, I added a ExtensionIconSet::MatchType parameter to LoadExtensionIconAsync() and LoadExtensionIconDataURLAsync(). Could you have a quick look at see if everything looks fine? Thanks.
Still LGTM.
With one nit... https://codereview.chromium.org/25050005/diff/105001/chrome/browser/extension... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/105001/chrome/browser/extension... chrome/browser/extensions/image_loader.h:122: // it should fallback on smaller or bigger size when choosing the icon source nit: s/on/to/
https://codereview.chromium.org/25050005/diff/105001/chrome/browser/extension... File chrome/browser/extensions/image_loader.h (right): https://codereview.chromium.org/25050005/diff/105001/chrome/browser/extension... chrome/browser/extensions/image_loader.h:122: // it should fallback on smaller or bigger size when choosing the icon source On 2013/10/03 10:45:11, Finnur wrote: > nit: s/on/to/ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dvh@chromium.org/25050005/109001
Message was sent while issue was closed.
Change committed as 226823 |
