Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
835903005:
[Favicon] Add new fallback icon rendering flow. (Closed)
|
Created:
5 years, 11 months ago by huangs Modified:
5 years, 9 months ago Reviewers:
stevenjb, calamity, sky, pkotwicz, kmadhusu, beaudoin, James Hawkins, Pam (message me for reviews), Jered CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Favicon] Add new fallback icon rendering flow.
Design: go/chrome-fallback-icons
This implements the "explicit" flow to get a fallback icon by
specifying the URL directly. For example:
chrome://fallback-icon/32,00f,ccf,0.5,0.8/http://www.google.com/
Otherwise there's no change to existing UI.
Details:
- Adding FallbackIconSource to host chrome://fallback-icon/
- Adding FaviconService::GetRawFallbackFaviconImage() to return image.
- Adding ParseFallbackIconPath() to parse chrome://fallback-icon/ styles.
- Unit tests for parser.
This CL has been sliced into smaller pieces:
https://codereview.chromium.org/886163003/
https://codereview.chromium.org/924063003/
https://codereview.chromium.org/973883002/
BUG=455063
Patch Set 1 #Patch Set 2 : Refactor by extracting FallbackIconSpecsBuilder. #
Total comments: 10
Patch Set 3 : Adding new host chrome://fallback-icon. #
Total comments: 67
Patch Set 4 : Style fixes, removing pure-cleanup cleanges. #Patch Set 5 : Removing FallbackIconStyleBuilder; trimming down chrome://fallback-icon syntax. #
Total comments: 69
Patch Set 6 : Cleanup and simplification. #
Total comments: 4
Patch Set 7 : Comment and style fix. #Patch Set 8 : Sync and merge. #
Total comments: 12
Patch Set 9 : Style and comment fixes. #Patch Set 10 : Sync. #Patch Set 11 : Sync and split off components/favicon_base piece (https://codereview.chromium.org/886163003/). #Patch Set 12 : Getting code to work again after merge. #Patch Set 13 : Sync and split off FallbackIconUrlParser piece (https://codereview.chromium.org/924063003/). #Patch Set 14 : Getting code to work again after merge. #
Messages
Total messages: 48 (8 generated)
huangs@chromium.org changed reviewers: + beaudoin@chromium.org, calamity@chromium.org, pkotwicz@chromium.org
PTAL.
A few preliminary comments. I want to keep the code for fallback icons separate from the favicon code till the design for fallback icons has stabilized. I am hoping that once the design for fallback icons has stabilized: - There will be less extra parameters which need to be passed via the chrome:// URL (I am hoping for none but that may be unrealistic) - The only way to request a fallback icon would be by requesting a normal favicon. The fallback icon will only be returned if a normal favicon was not found in the history database. https://codereview.chromium.org/835903005/diff/20001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/835903005/diff/20001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:91: // beyond the function call. My hope is that this function will never be needed. Ideally, the only way to get a fallback icon would be by requesting a normal favicon for icon URL if: - a favicon for |icon_url| was not stored at the desired size in the favicon database. For now FallbackFaviconSource (not FaviconSource) can call directly into FallbackIconService https://codereview.chromium.org/835903005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/favicon_source.cc (right): https://codereview.chromium.org/835903005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/favicon_source.cc:93: if (parsed.fallback_specs_builder) { Can you move the code for parsing the URLs for fallback icons to a new class which inherits from content::URLDataSource? Once we are ready to have fallback icons used everywhere that favicons are used, you can move the parsing logic to this class. https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:49: fallback_icon_font_list_.push_back(font_name); You should cache the gfx::FontList. Creating the gfx::FontList object is very expensive for some fonts. (e.g. "ui-sans" on ChromeOS) https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/favicon_types.h:94: double corner_radius_ratio; I think that a pixel corner radius is more intuitive (Alternatively you can rename this member to "roundness". 0.0f -> square, 1.0f -> circle)
https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:94: FaviconRawBitmapResult result; Having the fallback icon codepath separate from the main favicon codepathallows you to: - avoid using FaviconRawBitmapResult - make this function synchronous
On 2015/01/19 04:32:02, pkotwicz wrote: > A few preliminary comments. > > I want to keep the code for fallback icons separate from the favicon code till > the design for fallback icons has stabilized. I am hoping that once the design > for fallback icons has stabilized: > - There will be less extra parameters which need to be passed via the chrome:// > URL (I am hoping for none but that may be unrealistic) > - The only way to request a fallback icon would be by requesting a normal > favicon. The fallback icon will only be returned if a normal favicon was not > found in the history database. Will address code details tomorrow. I should clarify that the "explicit" flow from this CL is useful for testing, server NTP, and the new App Launcher. For existing usage by local NTP, bookmarks, and history, we keep the old interface; fallback rendering will not need the parameters, and will use default values (though the background will be grey). But before making such an "implicit" flow (thus changing the looks of local NTP, bookmarks, and history), we should go through UI review. I plan to initiate this this coming week. As for having GetRawFallbackFaviconImage() where it is, the reason is that in a future CL, I plan to change this so if background color is unspecified and we are requesting a high-res fallback, then we attempt to fetch a low-res favicon, and extract the dominant color as the background color. This will be the default behavior for high-res fallback icons (which will also be the fallback for high-res icons).
I don't mind if FallbackIconService is in chrome/browser/favicon as opposed to in components/favicon_base If FallbackIconService is in chrome/browser/favicon can't it download the low-res favicon to extract the dominant color? Please ping me if you have any questions (pinging is more time-efficient)
Updated, PTAL. Also renamed FallbackIconSpecs* to FallbackIconStyle*. https://codereview.chromium.org/835903005/diff/20001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/835903005/diff/20001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:91: // beyond the function call. Simplified, and named everything "FallbackIcon" or "fallback-icon". https://codereview.chromium.org/835903005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/favicon_source.cc (right): https://codereview.chromium.org/835903005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/favicon_source.cc:93: if (parsed.fallback_specs_builder) { Done. https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:49: fallback_icon_font_list_.push_back(font_name); Would this add extra memory footprint, and would this be premature optimization? Since this feature is not used yet, I'd rather keep it simple. Adding TODO though. https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:94: FaviconRawBitmapResult result; Done. https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/20001/components/favicon_base/... components/favicon_base/favicon_types.h:94: double corner_radius_ratio; "Roundness" is a good idea, updating.
Sorry, I didn't see your new CL. I will take a look on Wednesday morning
Thank you for separating the fallback icon code into its own class A few comments https://codereview.chromium.org/835903005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1076: } Will there be any extensions with permission to load chrome://fallback-icon? Is this required for instant NTP? If not, can you please remove this change? https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.cc:24: #include "url/gurl.h" Thank you for making this change. However, can you move "cleanup" changes like this one to a separate CL? https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:8: #include <memory> Is this include needed? https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size&scalefactor' Optional For simplicity, make all of the parameters mandatory: the size&scalefactor parameter: - can become just a size parameter (we should remove the scale factor parameter from FaviconSource too) - can be merged into the style&specs parameter. For instance: chrome://fallback-icon/000,fff,0.5,1,16/http://www.google.com will correspond to a fallback icon with: black foreground white text font 50% of icon height 100% roundness 16px size for http://www.google.com the style&specs parameter: - make all 5 parameters (size will be a parameter too) mandatory. I don't think you gain much from making an URL with just the first style parameter (e.g. chrome://fallback-icon/000/http://www.google.com) valid. This change should decrease the number of required unit tests. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/favicon_source.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/favicon_source.cc:88: favicon_service->GetRawFavicon( Can you move this change to a separate CL https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/history_ui.cc:432: content::URLDataSource::Add(profile, new FallbackIconSource()); Remove this. The history page does not use fallback icons (and when it does, it will make use of the implicit path) https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/ntp/most_visited_handler.cc:84: content::URLDataSource::Add(profile, new FallbackIconSource()); I think that this is the only place (not sure about InstantService) that you need to register FallbackIconSource. (For now, it will only be possible to get fallback icons from Web UI via the NTP) https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:860: Remove this too https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/suggestions_internals/suggestions_internals_ui.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/suggestions_internals/suggestions_internals_ui.cc:31: content::URLDataSource::Add(profile, new FallbackIconSource()); I don't think that this class is used for the NTP. If it isn't, then you should probably not add FallbackIconSource. https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:31: ParsedFallbackIconPath* parsed) { Can you move the parsing logic to FallbackIconSource? There shouldn't be much left here once you merge the size parameter into the style parameter. https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... File chrome/common/favicon/favicon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/favicon_url_parser.cc:38: DCHECK(parsed); Nit: Remove the DCHECK. We will crash on the next line anyways if parsed is nullptr https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/favicon_url_parser.cc:100: } else if (HasSubstringAt(path, parsed_index, kOriginParameter)) { Thanks! Can you move this change to a separate CL? https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:26: #include "url/gurl.h" Nit: Remove the includes that you do not use https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:7: Nit: Given that eventually you want to make use of FaviconService to determine the background color, I suggest moving this class to chrome/browser/favicon (I am fine if you leave the class here and do the move in an upcoming CL when you want to determine the dominant color from the low res favicon) https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:8: #include <vector> #include <string> https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:13: #include "components/favicon_base/favicon_callback.h" Remove cancelable_task_tracker.h and favicon_callback.h includes https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:19: } // namespace gfx Nit: Remove namespace comment. We do not do namespace comments for forward declarations https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:31: // Renders a fallback icon on a given |canvas| at position (|x|, |y|). Nits: - Can you make this method private? - "a given |canvas|" -> "|canvas|" Optional: Merge this method with RenderFallbackIconBitmap(). In terms of future compatibility with bookmark_app_helper.cc, you could have GetFallbackIconBitmap() which returns a SkBitmap and GetFallbackIconPNGBytes() which returns raw PNG data. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:33: const FallbackIconStyle& style, gfx::Canvas* canvas); Style: Each parameter on its own line https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:38: const GURL& icon_url, int size_in_pixels, const FallbackIconStyle& style); Style: Each parameter on its own line https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_style_builder.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:80: for (size_t i = 0; i < len; ++i) { Nit: can you use base::HexStringToInt() ? https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:104: bool FallbackIconStyleBuilder::Parse(const std::string& specs_str) { Can this method return a FallbackIconStyle? (And change the default FallbackIconStyle() constructor to create a FallbackIconStyle with the correct defaults) https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:108: if (tokens.size() > 4) // Too many fields. I think that it makes more sense to make all of the fields mandatory. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:115: return Reset(); How about leaving the background color as the default if parsing the color failed? https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/favicon_types.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.cc:33: roundness(0.0) { Can the constructor initialize FallbackIconStyle with the defaults? https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:9: #include "ui/gfx/color_utils.h" Nit: Please include "third_party/skia/include/core/SkColor.h" Why are you including "ui/gfx/color_utils.h"? https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:78: // excludes icon URL and size, since these are only specified when we render. How about: "Specification for a fallback icon. The icon is composed of a solid rounded square containing a single letter. The specification excludes the icon URL which is specified when the icon is rendered." No reason to exclude the size :) https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:89: // Ratio in [0.0, 1.0] from text font size (pixels) to icon size. Nits: "from text font size" -> "of the text font size" "to icon size" -> "to the icon size" https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:93: // adjusted so 0.0 => square and 0.5 => circle. How about: "The roundness of the icon's corners. 0.0 => square icon, 1.0 => circle icon. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:96: // If any member change, also update FallbackIconStyleBuilder. Nit: change -> changes
Updated. I think we should keep size separate. PTAL. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1076: } On 2015/01/21 19:44:14, pkotwicz wrote: > Will there be any extensions with permission to load chrome://fallback-icon? Is > this required for instant NTP? > > If not, can you please remove this change? Done. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.cc:24: #include "url/gurl.h" Done, including putting back the removal of redundant #include "components/favicon_base/favicon_types.h" https://codereview.chromium.org/861333002 https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:8: #include <memory> Oops remnants from now-removed use of std::unique_ptr<>. Removed. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size&scalefactor' Optional I really think size should be separate from styles. For Favicon size stands out as a separate parameter since an icon can have multiple sizes. FallbackIcon resembles Favicon, so size should be a separate parameter. Next, chrome://favicon names all the parameters except the last URL, so we should keep this the same. As for scale factors aa@bx, this seems to be an addition since the parsing cod has comments on "legacy format" of aa. If you don't like the code duplication, we can extract common code between ParseFaviconPath() and ParseFallbackIconPath() into a common utility. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/favicon_source.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/favicon_source.cc:88: favicon_service->GetRawFavicon( On 2015/01/21 19:44:15, pkotwicz wrote: > Can you move this change to a separate CL Done. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/history_ui.cc:432: content::URLDataSource::Add(profile, new FallbackIconSource()); On 2015/01/21 19:44:15, pkotwicz wrote: > Remove this. The history page does not use fallback icons (and when it does, it > will make use of the implicit path) Done. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/ntp/most_visited_handler.cc:84: content::URLDataSource::Add(profile, new FallbackIconSource()); Acknowledged. I want to be able to type in the chrome://fallback-icon URL and see it working though -- for testing. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/browser_options_handler.cc:860: On 2015/01/21 19:44:15, pkotwicz wrote: > Remove this too Done. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/suggestions_internals/suggestions_internals_ui.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/suggestions_internals/suggestions_internals_ui.cc:31: content::URLDataSource::Add(profile, new FallbackIconSource()); On 2015/01/21 19:44:15, pkotwicz wrote: > I don't think that this class is used for the NTP. If it isn't, then you should > probably not add FallbackIconSource. Done. https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:31: ParsedFallbackIconPath* parsed) { Per other comment, I think size should be a separate parameters, and all parameter should be named. Again I won't mind doing some refactoring so common code between this and ParseFaviconPath() can refer to code in a common library -- but perhaps this should be done in a separate CL? https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... File chrome/common/favicon/favicon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/favicon_url_parser.cc:38: DCHECK(parsed); I'd rather keep this. Controlled crash is better than uncontrolled crash, and this also serves as documentation. Meanwhile this is cleanup, so moved to the other CL. https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/favicon_url_parser.cc:100: } else if (HasSubstringAt(path, parsed_index, kOriginParameter)) { On 2015/01/21 19:44:15, pkotwicz wrote: > Thanks! Can you move this change to a separate CL? Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:26: #include "url/gurl.h" Removed: #include "base/bind.h" #include "base/location.h" #include "base/message_loop/message_loop_proxy.h" #include "ui/gfx/favicon_size.h" #include "ui/gfx/image/image_skia.h" Note: #include "grit/platform_locale_settings.h" is for IDS_SANS_SERIF_FONT_FAMILY https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:7: Leaving the file here. However, FallbackIconService is supposed to be unaware of the icon dominant color background color stuff. For the implicit flow it will be up to FaviconService to call this. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:8: #include <vector> On 2015/01/21 19:44:15, pkotwicz wrote: > #include <string> Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:13: #include "components/favicon_base/favicon_callback.h" On 2015/01/21 19:44:15, pkotwicz wrote: > Remove cancelable_task_tracker.h and favicon_callback.h includes Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:19: } // namespace gfx On 2015/01/21 19:44:15, pkotwicz wrote: > Nit: Remove namespace comment. We do not do namespace comments for forward > declarations Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:31: // Renders a fallback icon on a given |canvas| at position (|x|, |y|). bookmark_app_helper.cc has the comment // Overlays a shortcut icon over the bottom left corner of a given image. so I provided this interface. Making it private to the .cc file for now and will update when this is needed. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:33: const FallbackIconStyle& style, gfx::Canvas* canvas); I thought this is okay if things fit on 2 lines. Changed anyway. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:38: const GURL& icon_url, int size_in_pixels, const FallbackIconStyle& style); On 2015/01/21 19:44:15, pkotwicz wrote: > Style: Each parameter on its own line Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_style_builder.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:80: for (size_t i = 0; i < len; ++i) { Note that I wish to avoid memory allocation in this routine, and I need to validate that |str| is valid. I thought of using HexStringToInt(). There are 3 possible cases: (1) Depending on length, we use this to extract substrings of |str| and get RGB through multiple calls. That's kinda bad. (2) Call HexStringToInt() once to convert str (into uint32), then shift out the pieces. But that's kinda silly since the routine packs the digits into a number, so we have to unpack it again. (3) Call HexStringToInt() for each character: same problem with substrings, and lots of overhead. What would be nice is to have base::BaseCharToDigit::Convert(), but that's not exposed in the interface. So I figured screw it, this won't compile to that much code anyway. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:104: bool FallbackIconStyleBuilder::Parse(const std::string& specs_str) { I consider Parse() to be a sibling of SetBackgroundColor() etc, so it should allow fields to be unspecified if necessary -- although a failed parse will kill everything. A use case is to allow the explicit flow to allow background to be unspecified, so we can test the flow to grab favicon and extract dominant color. Though this would require FallbackIconSource to grab FaviconService (and then implicit flow requires FaviconSource to grab FallbackIconService). I guess that's an argument for having one service... Will revisit this later. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:108: if (tokens.size() > 4) // Too many fields. As mentioned above, Parse() can still leave blanks field for more complex defaults (dominant color). https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:115: return Reset(); Note we impose strict limit for FontSizeRatio and Roundness (cannot be negative), it's better to make everything strict, and only assign defaults when intended via AssignDefaults() and Build(). https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/favicon_types.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.cc:33: roundness(0.0) { The default text color depends on the background color: it's black for bright background and white for dark background. If we initialize other fields then we'll have scattered default values. Therefore I'm letting FallbackIconStyleBuilder handle all the defaults, and just initialize them to useless values in the constructor. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:9: #include "ui/gfx/color_utils.h" On 2015/01/21 19:44:16, pkotwicz wrote: > Nit: Please include "third_party/skia/include/core/SkColor.h" > > Why are you including "ui/gfx/color_utils.h"? Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:78: // excludes icon URL and size, since these are only specified when we render. Rephrased, but kept size excluded still. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:89: // Ratio in [0.0, 1.0] from text font size (pixels) to icon size. On 2015/01/21 19:44:16, pkotwicz wrote: > Nits: "from text font size" -> "of the text font size" > "to icon size" -> "to the icon size" Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:93: // adjusted so 0.0 => square and 0.5 => circle. On 2015/01/21 19:44:16, pkotwicz wrote: > How about: "The roundness of the icon's corners. 0.0 => square icon, 1.0 => > circle icon. Done. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/favicon_types.h:96: // If any member change, also update FallbackIconStyleBuilder. On 2015/01/21 19:44:16, pkotwicz wrote: > Nit: change -> changes Done.
https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size&scalefactor' Optional I don't think that making chrome://fallback-icon similar to chrome://favicon should be a goal. Eventually everything will use the implicit path. (except bookmark apps Bookmark apps will not use favicons from WebUI though) My hope is that even the server NTP will use the implicit path. My understanding (and hope) is that the explicit path is temporary. If it is temporary, the biggest goal is to make the explicit path code as simple as possible. I think that the things that I have suggested make the code simpler. FYI: I have filed crbug.com/451042 to remove the scale factor parameter from chrome://favicon (However, even if crbug.com/451042 is never fixed, chrome://fallback-icon should not need a scale factor parameter) https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_style_builder.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:104: bool FallbackIconStyleBuilder::Parse(const std::string& specs_str) { Ok, I am fine with allowing empty arguments https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:108: if (tokens.size() > 4) // Too many fields. I think we can make all of the fields mandatory and allow blank values. I think that this is simplest. My understanding is that the goals are: - Have a temporary way for JS in the NTP to display fallback icons. Eventually, the NTP will switch to using implicit icons. - Have a temporary way for UX folks to try out different parameters for fallback icons. In light of these goals, I think that code compexity should dictate the parameter format, not user friendliness.
Updated, PTAL. https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size&scalefactor' Optional Done, removed scale factor, and always have dummy commas. Per discussion, size and style will be kept separate in the subroutines, but will be bundled in the explicit flow URL. https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/40001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:31: ParsedFallbackIconPath* parsed) { Moved to ParseFallbackIconPath(), per discussion. https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... File components/favicon_base/fallback_icon_style_builder.cc (right): https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:104: bool FallbackIconStyleBuilder::Parse(const std::string& specs_str) { Done, how we always have ",". https://codereview.chromium.org/835903005/diff/40001/components/favicon_base/... components/favicon_base/fallback_icon_style_builder.cc:108: if (tokens.size() > 4) // Too many fields. Done.
Thanks for bearing with me. I think that sky@ can do the next review https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:11: // FaviconSource is the gateway between network-level chrome: FaviconSource -> FallbackIconSource https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:16: // Some parameters are optional as described below. However, the order of the How about: "All of the parameters except for the url are optional." https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size' Optional Nit: With my suggested comment you can remove the Optional / Permanent tags https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:23: // Color in RRGBBBAA, RRGGBB, or RGB hex format, to specify the fallback RRGBBAA -> RRGGBBAA https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:27: // icon's text color. RRGBBAA -> RRGGBBAA https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:30: // (pixels) as a ratio to the icon's size. "request fallback" -> "fallback" https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:32: // Number in [0.0, 1.0] to specify the request fallback icon's roundness. "request fallback" -> "fallback" https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:36: // String to specify the page URL of the fallback icon. Nit: Add a new line https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:59: // Sends the 16x16 DIP 1x default favicon. How about: "Sends the default fallback icon." https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:68: content::URLDataSource::Add(profile, new FallbackIconSource()); I think that SuggestionsHandler is no longer used in Chrome because NewTabUI::IsDiscoveryInNTPEnabled() always returns false. If this is the case, please remove this change. I have filed http://crbug.com/450679 about killing this class entirely. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:9: #include "ui/base/webui/web_ui_util.h" You no longer need the above include. Remove it. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:33: // Translate and validate each digits. Nit: digits -> digit https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:86: return style->is_valid();; Nit: Remove extra semicolon https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:99: size_t parsed_index = 0; Nit: Can you replace the places where |parsed_index| is used with either: "0" or "slash + 1" https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:9: Nit: No need to include favicon_types.h https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:16: // The URL from which the fallback icon is being requested. How about: "The page URL the fallback icon is requested for." Your current comment suggests that the URL is the URL of the page requesting the fallback icon (i.e. the URL of the new tab page) https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:22: // Specifications of fallback icon. This is nullptr if no fallback. Remove the part about the nullptr because it is not true https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:34: FallbackIconUrlParserTest() { Setting the supported scale factors should no longer be necessary :) https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:171: "16,000,fff,0.75,0.25,junk", The above test case belongs in the "exactly 5 params" category? https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:208: Nit: Remove extra blank line https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:210: // Empty URL.. Nit: Remove extra period https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... File chrome/common/url_constants.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... chrome/common/url_constants.h:43: extern const char kChromeUIFallbackIconURL[]; This constant isn't used? https://codereview.chromium.org/835903005/diff/80001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/835903005/diff/80001/components/BUILD.gn#newc... components/BUILD.gn:243: "//components/favicon_base:unit_tests", Remove this because there are no longer any unit tests https://codereview.chromium.org/835903005/diff/80001/components/components_te... File components/components_tests.gyp (right): https://codereview.chromium.org/835903005/diff/80001/components/components_te... components/components_tests.gyp:384: 'components.gyp:favicon_base', Remove this because there are no longer any unit tests https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/BUILD.gn (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/BUILD.gn:10: "fallback_icon_style.h", I don't see fallback_icon_style.* in the diff https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/BUILD.gn:29: sources = [ The unit test now longer exists, remove it https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:31: // not handle non-ASCII domain names. Nit: fallback URL -> page URL https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:69: Nit: Skip |x| and |y| for now. Btw, I think the |display_rect| parameter in DrawStringRectWithFlags() needs to take into account |x| and |y| https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:73: int size_in_pixels, Nit: |size_in_pixels| -> |size| https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:11: #include "base/callback.h" Remove the above include because you no longer use it https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:42: int size_in_pixels, Nit: |size_in_pixels| -> |size| https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/favicon_types.cc (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:25: double kMaxRoundness = 1.0; Nit: Remove these. It does not make sense to have a constant for the maximum but not for the minimum https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:59: void FallbackIconStyle::MatchTextColorWithBackgroundColor() { Optional: color_utils::GetReadableColor(SK_ColorBLACK, background_color) yields the same result. It is probably better to use that https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:67: && roundness >= 0.0 && roundness <= kMaxRoundness; Style: && goes on the previous line
Updated, PTAL. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/fallback_icon_source.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:11: // FaviconSource is the gateway between network-level chrome: On 2015/01/23 16:00:47, pkotwicz wrote: > FaviconSource -> FallbackIconSource Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:16: // Some parameters are optional as described below. However, the order of the On 2015/01/23 16:00:47, pkotwicz wrote: > How about: "All of the parameters except for the url are optional." Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:20: // 'size' Optional On 2015/01/23 16:00:47, pkotwicz wrote: > Nit: With my suggested comment you can remove the Optional / Permanent tags Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:23: // Color in RRGBBBAA, RRGGBB, or RGB hex format, to specify the fallback On 2015/01/23 16:00:47, pkotwicz wrote: > RRGBBAA -> RRGGBBAA Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:27: // icon's text color. On 2015/01/23 16:00:47, pkotwicz wrote: > RRGBBAA -> RRGGBBAA Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:30: // (pixels) as a ratio to the icon's size. On 2015/01/23 16:00:47, pkotwicz wrote: > "request fallback" -> "fallback" Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:32: // Number in [0.0, 1.0] to specify the request fallback icon's roundness. On 2015/01/23 16:00:47, pkotwicz wrote: > "request fallback" -> "fallback" Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:36: // String to specify the page URL of the fallback icon. On 2015/01/23 16:00:47, pkotwicz wrote: > Nit: Add a new line Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/fallback_icon_source.h:59: // Sends the 16x16 DIP 1x default favicon. On 2015/01/23 16:00:47, pkotwicz wrote: > How about: "Sends the default fallback icon." Done. https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:68: content::URLDataSource::Add(profile, new FallbackIconSource()); On 2015/01/23 16:00:47, pkotwicz wrote: > I think that SuggestionsHandler is no longer used in Chrome because > NewTabUI::IsDiscoveryInNTPEnabled() always returns false. If this is the case, > please remove this change. > > I have filed http://crbug.com/450679 about killing this class entirely. Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:9: #include "ui/base/webui/web_ui_util.h" On 2015/01/23 16:00:47, pkotwicz wrote: > You no longer need the above include. Remove it. Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:33: // Translate and validate each digits. On 2015/01/23 16:00:47, pkotwicz wrote: > Nit: digits -> digit Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:86: return style->is_valid();; On 2015/01/23 16:00:47, pkotwicz wrote: > Nit: Remove extra semicolon Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.cc:99: size_t parsed_index = 0; Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:9: On 2015/01/23 16:00:47, pkotwicz wrote: > Nit: No need to include favicon_types.h Need this for favicon_base::FallbackIconStyle , which is a non-pointer member. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:16: // The URL from which the fallback icon is being requested. On 2015/01/23 16:00:47, pkotwicz wrote: > How about: "The page URL the fallback icon is requested for." > > Your current comment suggests that the URL is the URL of the page requesting the > fallback icon (i.e. the URL of the new tab page) Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser.h:22: // Specifications of fallback icon. This is nullptr if no fallback. On 2015/01/23 16:00:47, pkotwicz wrote: > Remove the part about the nullptr because it is not true Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:34: FallbackIconUrlParserTest() { Removed, also got rid of #include "ui/base/layout.h", the class and TEST_F. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:171: "16,000,fff,0.75,0.25,junk", On 2015/01/23 16:00:48, pkotwicz wrote: > The above test case belongs in the "exactly 5 params" category? Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:208: On 2015/01/23 16:00:48, pkotwicz wrote: > Nit: Remove extra blank line Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/favicon/fa... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:210: // Empty URL.. On 2015/01/23 16:00:48, pkotwicz wrote: > Nit: Remove extra period Done. https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... File chrome/common/url_constants.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... chrome/common/url_constants.h:43: extern const char kChromeUIFallbackIconURL[]; On 2015/01/23 16:00:48, pkotwicz wrote: > This constant isn't used? Comment at line 18 says we should sync with components below (the .cc file has more detain on line 20). https://codereview.chromium.org/835903005/diff/80001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/835903005/diff/80001/components/BUILD.gn#newc... components/BUILD.gn:243: "//components/favicon_base:unit_tests", On 2015/01/23 16:00:48, pkotwicz wrote: > Remove this because there are no longer any unit tests Done. https://codereview.chromium.org/835903005/diff/80001/components/components_te... File components/components_tests.gyp (right): https://codereview.chromium.org/835903005/diff/80001/components/components_te... components/components_tests.gyp:384: 'components.gyp:favicon_base', On 2015/01/23 16:00:48, pkotwicz wrote: > Remove this because there are no longer any unit tests Done. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/BUILD.gn (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/BUILD.gn:10: "fallback_icon_style.h", On 2015/01/23 16:00:48, pkotwicz wrote: > I don't see fallback_icon_style.* in the diff Done. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/BUILD.gn:29: sources = [ On 2015/01/23 16:00:48, pkotwicz wrote: > The unit test now longer exists, remove it Done. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:31: // not handle non-ASCII domain names. Done, updated comment to have TODO(huangs) for non-ASCII domain names. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:69: Done. Will add them later if needed. Nice catch on the DrawStringRectWithFlags(). Made |x| and |y| const 0 to show relation. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.cc:73: int size_in_pixels, On 2015/01/23 16:00:48, pkotwicz wrote: > Nit: |size_in_pixels| -> |size| Done. Note I updated RenderFallbackIconBitmap() as well. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:11: #include "base/callback.h" On 2015/01/23 16:00:48, pkotwicz wrote: > Remove the above include because you no longer use it Done. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/fallback_icon_service.h:42: int size_in_pixels, Done, updated comment to reflect this. Also updated for RenderFallbackIconBitmap() above. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... File components/favicon_base/favicon_types.cc (right): https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:25: double kMaxRoundness = 1.0; Removed. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:59: void FallbackIconStyle::MatchTextColorWithBackgroundColor() { I just tried this, and got background #757575 or darker => white text background #767676 or lighter => black text In particular, we want default background #808080 to have *white* text. So we need to use the old wan and control the threshold ourselves. :( No change in code. https://codereview.chromium.org/835903005/diff/80001/components/favicon_base/... components/favicon_base/favicon_types.cc:67: && roundness >= 0.0 && roundness <= kMaxRoundness; On 2015/01/23 16:00:48, pkotwicz wrote: > Style: && goes on the previous line Done.
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
LGTM +sky@ for OWNERS review. I am unsure whether the format of FallbackIconStyle is legal but couldn't find anything in the Google style guide https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... File chrome/common/url_constants.h (right): https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... chrome/common/url_constants.h:43: extern const char kChromeUIFallbackIconURL[]; Sorry, I didn't see that comment https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:80: // Draw a filled, colored rounded square. Not limiting |size|. I am confused as to what "Not limiting |size|" means. If it means that DrawFallbackIcon() doesn't check if |size| is below kMaxFallbackFaviconSize, I think it is ok to remove the comment
https://codereview.chromium.org/835903005/diff/100001/chrome/common/favicon/f... File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right): https://codereview.chromium.org/835903005/diff/100001/chrome/common/favicon/f... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:18: // /components/favicon_base/favicon_types.h Nit: fix whitespace
On 2015/01/23 20:43:39, pkotwicz wrote: > LGTM > > +sky@ for OWNERS review. I am unsure whether the format of FallbackIconStyle is > legal but couldn't find anything in the Google style guide You mean that it has public members? That is against the style guide: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... . What files do I need to review here? Also, did this go through the eng review? > > https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... > File chrome/common/url_constants.h (right): > > https://codereview.chromium.org/835903005/diff/80001/chrome/common/url_consta... > chrome/common/url_constants.h:43: extern const char kChromeUIFallbackIconURL[]; > Sorry, I didn't see that comment > > https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... > File components/favicon_base/fallback_icon_service.cc (right): > > https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... > components/favicon_base/fallback_icon_service.cc:80: // Draw a filled, colored > rounded square. Not limiting |size|. > I am confused as to what "Not limiting |size|" means. If it means that > DrawFallbackIcon() doesn't check if |size| is below kMaxFallbackFaviconSize, I > think it is ok to remove the comment
Can I change FallbackIconStyle back to struct then? It has a couple of function functions though. The idea and shapes of fallback icons came from UI designers (stevet@ and sgabriel@). I added some flexibility for testing and to accommodate possible tweaks in the near future. This should be trimmed down once design is finalized. Meanwhile the planned steps are in go/chrome-fallback-icons. The specific owners show support of the idea, but it would be helpful if you comment there as well. As for code, please focus on the interfaces, and specifically, the proposed new host chrome://fallback-icon/size,bc,tc,fsr,r/URL where bc = background color, tc = text color, fsr = font size ratio, and r = roundness.
Converting to a struct is fine. -Scott On Fri, Jan 23, 2015 at 4:01 PM, <huangs@chromium.org> wrote: > Can I change FallbackIconStyle back to struct then? It has a couple of > function > functions though. > > The idea and shapes of fallback icons came from UI designers (stevet@ and > sgabriel@). I added some flexibility for testing and to accommodate > possible > tweaks in the near future. This should be trimmed down once design is > finalized. > > Meanwhile the planned steps are in go/chrome-fallback-icons. The specific > owners show support of the idea, but it would be helpful if you comment > there as > well. > > As for code, please focus on the interfaces, and specifically, the > proposed new > host > > chrome://fallback-icon/size,bc,tc,fsr,r/URL > > where bc = background color, tc = text color, fsr = font size ratio, and r > = > roundness. > > > https://chromiumcodereview.appspot.com/835903005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Updated, PTAL. https://codereview.chromium.org/835903005/diff/100001/chrome/common/favicon/f... File chrome/common/favicon/fallback_icon_url_parser_unittest.cc (right): https://codereview.chromium.org/835903005/diff/100001/chrome/common/favicon/f... chrome/common/favicon/fallback_icon_url_parser_unittest.cc:18: // /components/favicon_base/favicon_types.h On 2015/01/23 20:44:08, pkotwicz wrote: > Nit: fix whitespace Done. https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/100001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:80: // Draw a filled, colored rounded square. Not limiting |size|. On 2015/01/23 20:43:38, pkotwicz wrote: > I am confused as to what "Not limiting |size|" means. If it means that > DrawFallbackIcon() doesn't check if |size| is below kMaxFallbackFaviconSize, I > think it is ok to remove the comment Done.
huangs@chromium.org changed reviewers: + erikwright@chromium.org, jhawkins@chromium.org
OWNER reviews: erikwright@: components\favicon_base.gypi components\favicon_base\* jhawkins@: chrome\browser\search\instant_service.cc chrome\browser\ui\webui\* chrome\common\favicon\* sky@: chrome\*.gypi chrome\common\url_constants.* PTAL. Thanks!
requested files LGTM
On 2015/01/28 21:13:06, sky wrote: > requested files LGTM Please select an OWNER from components/favicon_base: ---- sky@chromium.org stevenjb@chromium.org # Temporary for the duration of the favicon componentization. sdefresne@chromium.org ---- Note that the same OWNERs have per-file OWNERship of favicon_base.gypi too.
"git cl owner" claims you're an owner; will investigate this. Meanwhile, I'll bug stevenjb@. Thanks!
huangs@chromium.org changed reviewers: + stevenjb@chromium.org - erikwright@chromium.org
OWNER review to stevenjb@: components\favicon_base.gypi components\favicon_base\* PTAL. Thanks!
On 2015/01/29 16:17:51, huangs1 wrote: > OWNER review to stevenjb@: > components\favicon_base.gypi > components\favicon_base\* > > PTAL. Thanks! I am an OWNER of components/ but that's a very big directory with many distinct subdirectories, most of which I know nothing about. In general it is best to choose the most specific OWNERs possible (i.e., those from deepest, most specific OWNERS files in the hierarchy who will be most familiar with the modified code) while still attempting to have a reasonable number of reviewers giving a meaningful review of the CL. If the two goals come into conflict, your CL is probably too big.
https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:43: l10n_util::GetStringUTF8(IDS_SANS_SERIF_FONT_FAMILY); Pt this in #else to avoid duplicate assignment on Chrome OS https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:75: const int y = 0; Either name these meaningfully as constants, e.g. kOffsetX, or just use 0 in the code. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:84: corner_radius, paint); Align (better yet, use git cl format) https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.h:6: #define COMPONENTS_FAVICON_BASE_FAVICON_FALLBACK_ICON_SERVICE_H_ extra _FAVICON_ https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.h:30: // empty std::vector on failure. |size| is in pixels. Could you explain what exactly |size| refers to? https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/favicon_types.h:101: // If any member changes, also update FallbackIconStyleBuilder. Move this comment above the members.
huangs@chromium.org changed reviewers: + pam@chromium.org
Updated, PTAL. Adding pam@ for change to DEPS: +net/base/registry_controlled_domains/registry_controlled_domain.h' Also, ping jhawkins@. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/fallback_icon_service.cc (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:43: l10n_util::GetStringUTF8(IDS_SANS_SERIF_FONT_FAMILY); On 2015/01/29 21:41:08, stevenjb wrote: > Pt this in #else to avoid duplicate assignment on Chrome OS Done. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:75: const int y = 0; Taking kOffsetX/Y. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.cc:84: corner_radius, paint); Moved content to following line. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/fallback_icon_service.h (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.h:6: #define COMPONENTS_FAVICON_BASE_FAVICON_FALLBACK_ICON_SERVICE_H_ On 2015/01/29 21:41:08, stevenjb wrote: > extra _FAVICON_ Done. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/fallback_icon_service.h:30: // empty std::vector on failure. |size| is in pixels. On 2015/01/29 21:41:08, stevenjb wrote: > Could you explain what exactly |size| refers to? Done. https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/835903005/diff/140001/components/favicon_base... components/favicon_base/favicon_types.h:101: // If any member changes, also update FallbackIconStyleBuilder. This was placed here since adding new member usually occurs at the end. But then again change may also mean removing... Moved above the members.
lgtm
On 2015/01/28 18:08:24, huangs1 wrote: > OWNER reviews: > > erikwright@: > components\favicon_base.gypi > components\favicon_base\* > > jhawkins@: > chrome\browser\search\instant_service.cc Please pick an owner from c\b\search. > chrome\browser\ui\webui\* > chrome\common\favicon\* > > sky@: > chrome\*.gypi > chrome\common\url_constants.* > > PTAL. Thanks!
This is a rather large CL. Is there a (mini-) design doc for this?
It's here: go/chrome-fallback-icons
huangs@chromium.org changed reviewers: + kmadhusu@chromium.org
OWNER review to kmadhusu@ for chrome/browser/search/instant_service.cc PTAL.
On 2015/01/30 00:25:45, huangs1 wrote: > It's here: go/chrome-fallback-icons Thanks. I don't see anything about a lot of the code I'm reviewing in webui and common. Can you update the doc? Also, please consider breaking out some of these pieces into separate CLs for the sake of reviewability and rollback-ability.
The FallbackIconSource stuff is described by the fallback icon specs part, and the fact that we're running a new host chrome://fallback-icon. Otherwise it's really just implementation detail since most of the code there is boilerplate.
Since this has been split into two other reviews, please close this issue. Thanks, - Pam
This CL is the third piece: making the feature accessible via chrome://fallback-favicon . Once the second piece is in, I'll merge and subtract the changes from this CL.
jered@chromium.org changed reviewers: + jered@chromium.org
jered@chromium.org changed reviewers: + jered@chromium.org
lgtm
lgtm
On 2015/03/03 19:08:23, Jered wrote: > lgtm Thanks. Could you PTAL https://codereview.chromium.org/973883002/ ? I'm going to just close this CL. Thanks!
Message was sent while issue was closed.
Closing CL as all the pieces have been committed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
