Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26563004:
Find Favicon in priority of icon_type. (Closed)
|
Created:
7 years, 2 months ago by michaelbai Modified:
7 years, 2 months ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFind Favicon in priority of icon_type.
This patch provided a way to find the favicon in the order of a list
of icon_types. if the best fit icon is found the rest of icon_types
will not be searched, otherwise, the largest icon of all list
icon_types is returned.
BUG=298446
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229760
Patch Set 1 #
Total comments: 17
Patch Set 2 : address comments #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : add a new method #
Total comments: 34
Patch Set 5 : address comments #
Total comments: 28
Patch Set 6 : #
Total comments: 22
Patch Set 7 : address comments #
Total comments: 3
Patch Set 8 : #
Total comments: 27
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #
Total comments: 2
Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 41 (0 generated)
+pkotwicz https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:58: const std::vector<int> icon_types_priority, const std::vector<int>& https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:69: std::vector<int> icon_types_priority; How come these take an int instead of IconType? Also, name it icon_types as order is implied by it being a vector. https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.cc:1711: GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors, Can we do one request with a bitmask of the types in icon_types_priority and then do the ordering? https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, icon_types_priority->icon_types https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_service.h:714: const std::vector<int>& icon_types_priority, Make this take FaviconForURLParams.
I will take a look at this CL on Wednesday. Michael, so that I can provide a better review, can you describe the other CLs which need to be written in order to resolve the bug
https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:65: threshold_for_next_icon_types(threshold_for_next_icon_types) {} I would expect to have a vector of thresholds with the same amount of elements as |icon_types_priority|. The constructor should also not have a |desired_size_in_dip| given that it is unclear how |threshold_for_next_icon_types| should be handled when |desired_size_in_dip| != 0 Some overarching questions: 1) How common are 32x32 touch icons? If they are rare, we could change the logic to prefer: i) Closest match to |desired_size_in_dip| and whichever scale factors are passed in ii) Favicons over touch icons in case that the two have the same size. 2) If we expect to always be passing in the same values for |icon_types_priority| and |threshold_for_new_icon_types| it may make sense to make those values an implementation detail of HistoryBackend.
Want to clarify one thing, the query is same as before except the query will be against different set of icon_types, the threshold_for_next_icon_types is used to determine whether the query should move on to next set of icon_types. I realized this change make it difficult to understand this API, as the parameter passed this function will always be same, it might be easier for reading to move the logic into HistoryBackend, like 1. Find the largest icon in favicon. 2. If the found icon is only normal favicon(16x16), find the largest icon in precomposed touch icon and touch icon. 3. If no touch icon was found, return the normal favicon. sky@ How about adding another method and having this logic in HistoryBackend. Note, HistoryBackend needs to know the normal favicon size. https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:65: threshold_for_next_icon_types(threshold_for_next_icon_types) {} On 2013/10/09 17:11:51, pkotwicz wrote: > I would expect to have a vector of thresholds with the same amount of elements > as |icon_types_priority|. > The constructor should also not have a |desired_size_in_dip| given that it is > unclear how |threshold_for_next_icon_types| should be handled when > |desired_size_in_dip| != 0 > > Some overarching questions: > 1) How common are 32x32 touch icons? If they are rare, we could change the logic > to prefer: > i) Closest match to |desired_size_in_dip| and whichever scale factors are > passed in > ii) Favicons over touch icons in case that the two have the same size. > > 2) If we expect to always be passing in the same values for > |icon_types_priority| and |threshold_for_new_icon_types| it may make sense to > make those values an implementation detail of HistoryBackend. Yes, currently, it will always be the same values, but I am not sure it is good to have this specific logic in HistoryBackend. https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:69: std::vector<int> icon_types_priority; Using int because The icon_types could be icon_types_prority.push_back(chrome::FAVICON); icon_types_prority.push_back(chrome::TOUCH_ICON|chrome::PRECOMPOSED_TOUCH_ICON); On 2013/10/08 23:47:52, sky wrote: > How come these take an int instead of IconType? > Also, name it icon_types as order is implied by it being a vector. https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.cc:1711: GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors, I see your point, as the icon_types could be a composition of icon_type, it might be a little complicated to sort the result and find the best one here. On 2013/10/08 23:47:52, sky wrote: > Can we do one request with a bitmask of the types in icon_types_priority and > then do the ordering?
I wonder whether: 1. Find the largest icon in favicon. 2. If the found icon is only normal favicon(16x16), find the largest icon in precomposed touch icon and touch icon. 3. If no touch icon was found, return the normal favicon. can be simplified even further to: 1. Find the largest icon of any type 2. Use the icon type to resolve conflicts when favicons of the same size of more than one icon type are available.
On 2013/10/09 19:01:09, pkotwicz wrote: > I wonder whether: > 1. Find the largest icon in favicon. > 2. If the found icon is only normal favicon(16x16), find the largest icon in > precomposed touch icon and touch icon. > 3. If no touch icon was found, return the normal favicon. > > can be simplified even further to: > 1. Find the largest icon of any type > 2. Use the icon type to resolve conflicts when favicons of the same size of more > than one icon type are available. of course, we can get all images from DB at one time, but the requirement is always use favicon if it is larger than 16x16, so, the priority is - largest favicon which is larger than 16x16, even it is smaller than touch icon. - largest touch icon or precomposed touch icon as long as it is larger than 16x16 favicon - 16x16 favicon.
https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.cc:1711: GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors, On 2013/10/09 18:35:09, michaelbai wrote: > I see your point, as the icon_types could be a composition of icon_type, it > might be a little complicated to sort the result and find the best one here. Why? It doesn't seems like it's going to be much more complicated than what you have here. Also, you have to assume going to the DB is a LOT slower than some sorting here. > > On 2013/10/08 23:47:52, sky wrote: > > Can we do one request with a bitmask of the types in icon_types_priority and > > then do the ordering? >
PTAL, I should address all comments https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/favicon/favico... chrome/browser/favicon/favicon_service.h:58: const std::vector<int> icon_types_priority, On 2013/10/08 23:47:52, sky wrote: > const std::vector<int>& Done. https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.cc:1711: GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors, I tried, it seemed it is complicated, GetFaviconsFromDB will only return one best fit icon, not all the icon which met the criteria. To make the GetFaviconsFromDB returns all icons requires change a lot of code including API's definition, like GetIconMappingsForPageURL needs to return all icon mappings instead of one and how to calculate best match icon in GetFaviconBitmapResultsForBestMatch. On 2013/10/09 20:33:24, sky wrote: > On 2013/10/09 18:35:09, michaelbai wrote: > > I see your point, as the icon_types could be a composition of icon_type, it > > might be a little complicated to sort the result and find the best one here. > > Why? It doesn't seems like it's going to be much more complicated than what you > have here. Also, you have to assume going to the DB is a LOT slower than some > sorting here. > > > > > On 2013/10/08 23:47:52, sky wrote: > > > Can we do one request with a bitmask of the types in icon_types_priority and > > > then do the ordering? > > > https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, As element of the list could be a composition of icon_type, this name might better than 'icon_types' On 2013/10/08 23:47:52, sky wrote: > icon_types_priority->icon_types https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_service.h:714: const std::vector<int>& icon_types_priority, On 2013/10/08 23:47:52, sky wrote: > Make this take FaviconForURLParams. Done.
https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.cc:1711: GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors, To be more specific, AFAIK - GetIconMappingsForPageURL needs to change to return all matched icons of given icon_types. - GetFaviconBitmapResultsForBestMatch needs return best matched icon for each requested icon_type I don't know how others depend on the output of above 2 functions. - GetFaviconsForURL needs to have exact same output as before when icon_types_priority.size() == 0. The change seems big, let me know what's your opinion. On 2013/10/10 05:51:43, michaelbai wrote: > I tried, it seemed it is complicated, GetFaviconsFromDB will only return one > best fit icon, not all the icon which met the criteria. To make the > GetFaviconsFromDB returns all icons requires change a lot of code including > API's definition, like GetIconMappingsForPageURL needs to return all icon > mappings instead of one and how to calculate best match icon in > GetFaviconBitmapResultsForBestMatch. > > > On 2013/10/09 20:33:24, sky wrote: > > On 2013/10/09 18:35:09, michaelbai wrote: > > > I see your point, as the icon_types could be a composition of icon_type, it > > > might be a little complicated to sort the result and find the best one here. > > > > Why? It doesn't seems like it's going to be much more complicated than what > you > > have here. Also, you have to assume going to the DB is a LOT slower than some > > sorting here. > > > > > > > > On 2013/10/08 23:47:52, sky wrote: > > > > Can we do one request with a bitmask of the types in icon_types_priority > and > > > > then do the ordering? > > > > > >
If I was doing the implementation, I would be tempted to modify ThumbnailDatabase::GetIconMappingsForPageURL() and change how the method selects which icon type to return.
https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, On 2013/10/10 05:51:43, michaelbai wrote: > As element of the list could be a composition of icon_type, this name might > better than 'icon_types' > > On 2013/10/08 23:47:52, sky wrote: > > icon_types_priority->icon_types > I don't get this. Why do we need the ability to support *both* an ordered list containing bitmasks and IconTypes. Can't we make it only an ordered list of IconTypes? If you say no, why? What specific use case do you have that requires a vector of bitmasks? https://codereview.chromium.org/26563004/diff/15001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:67: Profile* profile; Since the new args are not clear you need to point folks at where the documentation can be found. https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, icon_types_priority->icon_types here and every where. https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... chrome/browser/history/history_service.h:694: // icon_types in |params|.icon_types_in_priority which most closely match |params.icon_types| (|s around the whole thing).
PTAL https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/1/chrome/browser/history/histor... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, The use case is we want use favicon first if it is larger than normal size, if not, the largest icon of touch_icon or precomposed_touch_icon would work, if there is no touch_icon or precomposed_touch_icon, normal favicon will be used. In this case, icon_types the vector should be icon_types.push_back(FAVICON); icon_types.push_back(TOUCH_ICON|PRECOMPOSED_TOUCH_ICON); On 2013/10/10 20:35:12, sky wrote: > On 2013/10/10 05:51:43, michaelbai wrote: > > As element of the list could be a composition of icon_type, this name might > > better than 'icon_types' > > > > On 2013/10/08 23:47:52, sky wrote: > > > icon_types_priority->icon_types > > > > I don't get this. Why do we need the ability to support *both* an ordered list > containing bitmasks and IconTypes. Can't we make it only an ordered list of > IconTypes? If you say no, why? What specific use case do you have that requires > a vector of bitmasks? https://codereview.chromium.org/26563004/diff/15001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:67: Profile* profile; I did it at line 54-55. On 2013/10/10 20:35:12, sky wrote: > Since the new args are not clear you need to point folks at where the > documentation can be found. https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... chrome/browser/history/history_backend.h:247: const std::vector<int>& icon_types_priority, On 2013/10/10 20:35:12, sky wrote: > icon_types_priority->icon_types here and every where. Done. https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/15001/chrome/browser/history/hi... chrome/browser/history/history_service.h:694: // icon_types in |params|.icon_types_in_priority which most closely match On 2013/10/10 20:35:12, sky wrote: > |params.icon_types| (|s around the whole thing). Done.
I will look at this on Friday.
https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1700: void HistoryBackend::GetFaviconsForURL( I think it may be best to create a new method in HistoryBackend to deal with the extra functionality you are adding in. HistoryBackend::GetLargestFaviconForURL( GURL page_url, const std::vector<int> icon_types, const std::vector<int> thresholds_for_next_icon_type, chrome::FaviconBitmapresult* bitmap_result) The behavior you currently have when |icon_types| has size 1 and when |icon_types| has size > 1 is completely different. There is no reason in my mind that the two should be handled by the same method. Have you considered other ways of tackling this bug? For instance, not storing touch icons in the database at all if we do not want to retrieve them. This is a non trivial change, but you will need to change FaviconHandler anyways to store large favicons of type FAVICON. This change would not necessitate changing HistoryBackend at all because favicons expire within 7 days.
https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1700: void HistoryBackend::GetFaviconsForURL( I had discussed adding new method with sky offline, he preferred to use one method. They are not complete different, they actually share a lot of code. As you can see, the previous feature can be implemented easily by setting icon_types with one element, and we probably also still need the capability to select the icon by desired_size_in_dip and desired_scale_factors. I considered the solution to only download the icon that will be used before write this CL, it moves how we select the icon to the download time, probably will duplicate the logic in HistoryBackend, as you said this is not a trivial change, and we probably will not support touch icon in the future, it might not worthy to make such change right now. On 2013/10/11 16:14:19, pkotwicz wrote: > I think it may be best to create a new method in HistoryBackend to deal with the > extra functionality you are adding in. > HistoryBackend::GetLargestFaviconForURL( > GURL page_url, > const std::vector<int> icon_types, > const std::vector<int> thresholds_for_next_icon_type, > chrome::FaviconBitmapresult* bitmap_result) > > The behavior you currently have when |icon_types| has size 1 and when > |icon_types| has size > 1 is completely different. There is no reason in my mind > that the two should be handled by the same method. > > Have you considered other ways of tackling this bug? For instance, not storing > touch icons in the database at all if we do not want to retrieve them. This is a > non trivial change, but you will need to change FaviconHandler anyways to store > large favicons of type FAVICON. This change would not necessitate changing > HistoryBackend at all because favicons expire within 7 days.
https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1700: void HistoryBackend::GetFaviconsForURL( In looking at this bit more I agree with Peter. This is trying to shoe horn too much into a single function. Move new behavior into a separate function, maybe GetLargestRawFaviconForURL. Create a new structure (or just pass in parameters and no structure) for it as well. That is, don't overload FaviconForURLParams to handle both. On 2013/10/11 16:34:32, michaelbai wrote: > I had discussed adding new method with sky offline, he preferred to use one > method. They are not complete different, they actually share a lot of code. As > you can see, the previous feature can be implemented easily by > setting icon_types with one element, and we probably also still need the > capability to select the icon by desired_size_in_dip and desired_scale_factors. > > I considered the solution to only download the icon that will be used before > write this CL, it moves how we select the icon to the download time, probably > will duplicate the logic in HistoryBackend, as you said this is not a trivial > change, and we probably will not support touch icon in the future, it might not > worthy to make such change right now. > > > On 2013/10/11 16:14:19, pkotwicz wrote: > > I think it may be best to create a new method in HistoryBackend to deal with > the > > extra functionality you are adding in. > > HistoryBackend::GetLargestFaviconForURL( > > GURL page_url, > > const std::vector<int> icon_types, > > const std::vector<int> thresholds_for_next_icon_type, > > chrome::FaviconBitmapresult* bitmap_result) > > > > The behavior you currently have when |icon_types| has size 1 and when > > |icon_types| has size > 1 is completely different. There is no reason in my > mind > > that the two should be handled by the same method. > > > > Have you considered other ways of tackling this bug? For instance, not storing > > touch icons in the database at all if we do not want to retrieve them. This is > a > > non trivial change, but you will need to change FaviconHandler anyways to > store > > large favicons of type FAVICON. This change would not necessitate changing > > HistoryBackend at all because favicons expire within 7 days. > https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1711: i != icon_types.end(); i++) { always use pre-increment for iterators. https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1726: bitmap_results->clear(); Don't reuse the passed in vector, that's just confusing and error prone. https://codereview.chromium.org/26563004/diff/25001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1727: if (bitmap_size_in_pixel > threshold_for_next_icon_types) Why is threshold_for_next_icon_types applied to all and not a vector<int>? Given the usage a better name if desired_size.
PTAL Add the new method
ping
https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: CancelableTaskTracker::TaskId GetFaviconForChromeURL( Add description. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:60: return id; nit: indentation is off. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:328: } else { nit: no else here and 347. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:177: Profile* profile, Why do we need the profile here? Doesn't FaviconService have a way to know the Profile? https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:180: int minimal_size_in_pixel, minimum_size_in_pixels here and everywhere. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1710: // Time the query. nit: remove comment, just describes what the code is doing. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: if (icon_mappings.empty()) nit: merge with if on 1721. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1727: std::map<chrome::IconType, FaviconBitmap> largest_favicon_bitmaps; nit: add comments describing what you're doing here and 1745. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1732: FaviconBitmap largest = largest_favicon_bitmaps[i->icon_type]; This should be a ref. That way you don't need 1742. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1761: // Construct FaviconBitmapResults. nit: this comment isn't helpful, especially when bitmapresult isn't build until a ways down. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1764: if (!thumbnail_db_->GetFaviconHeader(largest_icon.icon_id, &icon_url, Might it be possible that you've never set largest_icon by the time you get here? https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:655: nit: only one newline. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:670: nit: only one newline. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... File chrome/browser/history/thumbnail_database.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... chrome/browser/history/thumbnail_database.cc:1088: bool ThumbnailDatabase::GetAllIconMappingsForPageURL( This is practically the same as GetIconMappingsForPageURL. Can you merge the two and add a bool that gives the different behavior.
A few extra comments. But looks good! https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1733: for (std::vector<FaviconBitmapIDSize>::const_iterator j = largest will not be populated if all of the pixel sizes are empty? For instance, FaviconSQLHandler::Insert() inserts favicons with an empty pixel size. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... File chrome/browser/history/thumbnail_database.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... chrome/browser/history/thumbnail_database.cc:1089: const GURL& page_url, Could you use the version of GetIconMappingsForPageURL() which does not take in a |required_icon_types| parameter directly? (There is only one caller of GetAllIconMappingsForPageURL() so far.
PTAL https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: CancelableTaskTracker::TaskId GetFaviconForChromeURL( On 2013/10/15 15:03:47, sky wrote: > Add description. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:60: return id; On 2013/10/15 15:03:47, sky wrote: > nit: indentation is off. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:328: } else { On 2013/10/15 15:03:47, sky wrote: > nit: no else here and 347. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:177: Profile* profile, Currently, it seemed that FaviconService are not supposed to get Profile by itself, the Profile is passed in in FaviconForURLParams, the HistoryService which only needs the profile to construct is also passed in. On 2013/10/15 15:03:47, sky wrote: > Why do we need the profile here? Doesn't FaviconService have a way to know the > Profile? https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:180: int minimal_size_in_pixel, On 2013/10/15 15:03:47, sky wrote: > minimum_size_in_pixels here and everywhere. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1710: // Time the query. On 2013/10/15 15:03:47, sky wrote: > nit: remove comment, just describes what the code is doing. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: if (icon_mappings.empty()) On 2013/10/15 15:03:47, sky wrote: > nit: merge with if on 1721. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1727: std::map<chrome::IconType, FaviconBitmap> largest_favicon_bitmaps; On 2013/10/15 15:03:47, sky wrote: > nit: add comments describing what you're doing here and 1745. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1732: FaviconBitmap largest = largest_favicon_bitmaps[i->icon_type]; On 2013/10/15 15:03:47, sky wrote: > This should be a ref. That way you don't need 1742. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1733: for (std::vector<FaviconBitmapIDSize>::const_iterator j = On 2013/10/15 16:44:33, pkotwicz wrote: > largest will not be populated if all of the pixel sizes are empty? For instance, > FaviconSQLHandler::Insert() inserts favicons with an empty pixel size. added if (largest_icon.pixel_size.width() == 0 ... https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1761: // Construct FaviconBitmapResults. On 2013/10/15 15:03:47, sky wrote: > nit: this comment isn't helpful, especially when bitmapresult isn't build until > a ways down. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1764: if (!thumbnail_db_->GetFaviconHeader(largest_icon.icon_id, &icon_url, On 2013/10/15 15:03:47, sky wrote: > Might it be possible that you've never set largest_icon by the time you get > here? added if (largest_icon.pixel_size.width() == 0 ... https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:655: On 2013/10/15 15:03:47, sky wrote: > nit: only one newline. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:670: On 2013/10/15 15:03:47, sky wrote: > nit: only one newline. Done. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... File chrome/browser/history/thumbnail_database.cc (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... chrome/browser/history/thumbnail_database.cc:1088: bool ThumbnailDatabase::GetAllIconMappingsForPageURL( Remove this method, move the logic to HistoryBackend, as I will got through the icon mappings anyway. On 2013/10/15 15:03:47, sky wrote: > This is practically the same as GetIconMappingsForPageURL. Can you merge the two > and add a bool that gives the different behavior. https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/th... chrome/browser/history/thumbnail_database.cc:1089: const GURL& page_url, On 2013/10/15 16:44:33, pkotwicz wrote: > Could you use the version of GetIconMappingsForPageURL() which does not take in > a |required_icon_types| parameter directly? (There is only one caller of > GetAllIconMappingsForPageURL() so far. Done.
https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: // Return the TaskId of the one to retreive the favicon from chrome specific How about: "Returns the TaskId to retrieve the favicon from a chrome-specific URL." https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:334: CancelableTaskTracker::TaskId FaviconService::GetLargestRawFaviconForURLImpl( Can you explain why you need both GetLargestRawFaviconForURL() and GetLargestRawFaviconForURLImpl() https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:344: std::vector<ui::ScaleFactor>(), callback, tracker); You should pass in a vector with ui::SCALE_FACTOR_100P. Passing in an empty vector is wrong given ChromeWebUIControllerFactory::GetFaviconForURL(). https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1699: void HistoryBackend::GetLargestFaviconForURL( Nit: Order method in same order as in header file https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1701: const std::vector<int>& icon_types, Given your current implementation, is there any reason that this cannot be: const std::vector<chrome::IconType>& icon_types https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1713: if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings)) { Nit: the braces are not necessary https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1716: if (icon_mappings.empty()) return; Nit: Return on a new line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: // Find the largest bitmap for each favicon in icon_types. How about: "Find the largest favicon bitmap for each icon type." https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1728: if (!(i->icon_type & required_icon_types)) continue; Nit: "continue" on a new line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1742: if (largest_favicon_bitmaps.empty()) return; Nit: return on a separate line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1762: largest_icon.pixel_size.height() == 0) { This is wrong. A pixel width and height in the database of 0 does not imply an invalid bitmap or a bitmap with width and height of 0. A pixel width and height in the database of 0 implies that the bitmap width and height in unknown. I would expect that if all of the results for GetFaviconBitmapIDSizes() had empty sizes, that the first result would be chosen.
thanks, PTAL https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: // Return the TaskId of the one to retreive the favicon from chrome specific On 2013/10/15 21:39:41, pkotwicz wrote: > How about: "Returns the TaskId to retrieve the favicon from a chrome-specific > URL." Done. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:334: CancelableTaskTracker::TaskId FaviconService::GetLargestRawFaviconForURLImpl( Removed On 2013/10/15 21:39:41, pkotwicz wrote: > Can you explain why you need both GetLargestRawFaviconForURL() and > GetLargestRawFaviconForURLImpl() https://codereview.chromium.org/26563004/diff/47001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:344: std::vector<ui::ScaleFactor>(), callback, tracker); On 2013/10/15 21:39:41, pkotwicz wrote: > You should pass in a vector with ui::SCALE_FACTOR_100P. Passing in an empty > vector is wrong given ChromeWebUIControllerFactory::GetFaviconForURL(). Thanks https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1699: void HistoryBackend::GetLargestFaviconForURL( Changed the header file order. On 2013/10/15 21:39:41, pkotwicz wrote: > Nit: Order method in same order as in header file https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1701: const std::vector<int>& icon_types, No, the icon_types could be FAVICON TOUCH_ICON | PRECOMPOSED_TOUCH_ICON. On 2013/10/15 21:39:41, pkotwicz wrote: > Given your current implementation, is there any reason that this cannot be: > > const std::vector<chrome::IconType>& icon_types https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1713: if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings)) { On 2013/10/15 21:39:41, pkotwicz wrote: > Nit: the braces are not necessary Done. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1716: if (icon_mappings.empty()) return; Sky want to use this style in his previous comment On 2013/10/15 21:39:41, pkotwicz wrote: > Nit: Return on a new line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: // Find the largest bitmap for each favicon in icon_types. On 2013/10/15 21:39:41, pkotwicz wrote: > How about: "Find the largest favicon bitmap for each icon type." Done. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1728: if (!(i->icon_type & required_icon_types)) continue; ditto On 2013/10/15 21:39:41, pkotwicz wrote: > Nit: "continue" on a new line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1742: if (largest_favicon_bitmaps.empty()) return; ditto On 2013/10/15 21:39:41, pkotwicz wrote: > Nit: return on a separate line https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1762: largest_icon.pixel_size.height() == 0) { In which situation, it is unknown? On 2013/10/15 21:39:41, pkotwicz wrote: > This is wrong. A pixel width and height in the database of 0 does not imply an > invalid bitmap or a bitmap with width and height of 0. > A pixel width and height in the database of 0 implies that the bitmap width and > height in unknown. > > I would expect that if all of the results for GetFaviconBitmapIDSizes() had > empty sizes, that the first result would be chosen.
https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:177: Profile* profile, On 2013/10/15 19:36:31, michaelbai wrote: > Currently, it seemed that FaviconService are not supposed to get Profile by > itself, the Profile is passed in in FaviconForURLParams, the HistoryService > which only needs the profile to construct is also passed in. > > On 2013/10/15 15:03:47, sky wrote: > > Why do we need the profile here? Doesn't FaviconService have a way to know the > > Profile? > I don't think there is a good reason for this. Make FaviconService take the Profile in its constructor and remove Profile from this method and others. If that change is too big, do it in a separate patch first. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1716: if (icon_mappings.empty()) return; On 2013/10/15 22:52:34, michaelbai wrote: > Sky want to use this style in his previous comment > > On 2013/10/15 21:39:41, pkotwicz wrote: > > Nit: Return on a new line > Not sure what you mean here. I do not like single life if statement. Maybe you mean my comment here https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... . By that I meant make 1713: if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings) || icon_mappings.empty()) return https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:195: nit: remove newline. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:180: int minimal_size_in_pixels, minimum_size_in_pixels. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: // Find the largest bitmap for in each icon_types. Find the largest bitmap for each IconType placing in |largest_favicon_bitmaps|. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1728: if (!(i->icon_type & required_icon_types)) continue; no single line if. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1743: if (largest_favicon_bitmaps.empty()) return; no single line if. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1784: favicon_bitmap_results->push_back(bitmap_result); As this only ever returns a single value, why the vector? https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.h:248: int minimal_size_in_pixels, minimum_size_in_pixels https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_service.h:710: // Used by the FaviconService to get largest icon of |page_url| in the order Used by FaviconService to find the first icon whose width and height are greater than or equal to that of |minimum_size_in_pixels|. This searches for icons by IconType. Each element of |icon_types| is a bitmask of IconTypes indicating the types to search for. I don't see you doing this part of your comment: "If none icon is larger than |minimal_size_in_pixel|, the largest one of all icon types in |icon_types| is returned." Am I missing something?
https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1701: const std::vector<int>& icon_types, I see your point. In practice we only ever save one of TOUCH_ICON and PRECOMPOSED_TOUCH_ICON into the database so there would be no difference either way (see HistoryBackend::SetFaviconMappingsForPage()) I suggested the change because it makes things simpler to think about. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1762: largest_icon.pixel_size.height() == 0) { FaviconSQLHandler::Insert() is an example. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: // Return the TaskId of to retreive the favicon from chrome specific URL. Nit: "of to retreive" -> "to retrieve" https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:175: // See history::HistoryService::GetLargestFaviconForURL. See HistoryService::GetLargestFaviconForURL() (HistoryService is not in the history namespace) https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:653: DCHECK(thread_checker_.CalledOnValidThread()); Nit: DCHECK should be indented with LoadBackendIfNecessary()
PTAL https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:177: Profile* profile, I will do it in separated patch. https://crbug/308153 On 2013/10/16 13:27:15, sky wrote: > On 2013/10/15 19:36:31, michaelbai wrote: > > Currently, it seemed that FaviconService are not supposed to get Profile by > > itself, the Profile is passed in in FaviconForURLParams, the HistoryService > > which only needs the profile to construct is also passed in. > > > > On 2013/10/15 15:03:47, sky wrote: > > > Why do we need the profile here? Doesn't FaviconService have a way to know > the > > > Profile? > > > > I don't think there is a good reason for this. Make FaviconService take the > Profile in its constructor and remove Profile from this method and others. If > that change is too big, do it in a separate patch first. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1701: const std::vector<int>& icon_types, Don't like this, the TOUCH_ICON and PRECOMPOSED_TOUCH_ICON is different type of icon, I want to keep that flexibility for any future requirement. On 2013/10/16 16:59:41, pkotwicz wrote: > I see your point. > In practice we only ever save one of TOUCH_ICON and PRECOMPOSED_TOUCH_ICON into > the database so there would be no difference either way (see > HistoryBackend::SetFaviconMappingsForPage()) > I suggested the change because it makes things simpler to think about. https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1762: largest_icon.pixel_size.height() == 0) { Is this the only case? On 2013/10/16 16:59:41, pkotwicz wrote: > FaviconSQLHandler::Insert() is an example. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:48: // Return the TaskId of to retreive the favicon from chrome specific URL. On 2013/10/16 16:59:41, pkotwicz wrote: > Nit: "of to retreive" -> "to retrieve" Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.cc:195: On 2013/10/16 13:27:16, sky wrote: > nit: remove newline. Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:175: // See history::HistoryService::GetLargestFaviconForURL. On 2013/10/16 16:59:41, pkotwicz wrote: > See HistoryService::GetLargestFaviconForURL() (HistoryService is not in the > history namespace) Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:180: int minimal_size_in_pixels, On 2013/10/16 13:27:16, sky wrote: > minimum_size_in_pixels. Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1724: // Find the largest bitmap for in each icon_types. On 2013/10/16 13:27:16, sky wrote: > Find the largest bitmap for each IconType placing in |largest_favicon_bitmaps|. Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1728: if (!(i->icon_type & required_icon_types)) continue; On 2013/10/16 13:27:16, sky wrote: > no single line if. Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1743: if (largest_favicon_bitmaps.empty()) return; On 2013/10/16 13:27:16, sky wrote: > no single line if. Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1784: favicon_bitmap_results->push_back(bitmap_result); It because ChromeWebUIControllerFactory::GetInstance()->GetFaviconForURL(), need vector as the return value. To make this change simple, I used the vector here. Removed vector. On 2013/10/16 13:27:16, sky wrote: > As this only ever returns a single value, why the vector? https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_backend.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_backend.h:248: int minimal_size_in_pixels, On 2013/10/16 13:27:16, sky wrote: > minimum_size_in_pixels Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:653: DCHECK(thread_checker_.CalledOnValidThread()); On 2013/10/16 16:59:41, pkotwicz wrote: > Nit: DCHECK should be indented with LoadBackendIfNecessary() Done. https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/59001/chrome/browser/history/hi... chrome/browser/history/history_service.h:710: // Used by the FaviconService to get largest icon of |page_url| in the order On 2013/10/16 13:27:16, sky wrote: > Used by FaviconService to find the first icon whose width and height are greater > than or equal to that of |minimum_size_in_pixels|. This searches for icons by > IconType. Each element of |icon_types| is a bitmask of IconTypes indicating the > types to search for. > > I don't see you doing this part of your comment: "If none icon is larger than > |minimal_size_in_pixel|, the largest one of all icon types in |icon_types| is > returned." Am I missing something? In the HistoryBackend::GetLargestFaviconForURL, the largest icon is stored in largest_icon local variable and this will always be returned no matter its size.
https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/47001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1762: largest_icon.pixel_size.height() == 0) { HistoryBackend::SetImportedFavicons() is another case
ping
https://codereview.chromium.org/26563004/diff/68001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/68001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1734: if ((largest.icon_id == 0 && largest.bitmap_id == 0) || Nit: Checking largest.bitmap_id == 0 should be sufficient https://codereview.chromium.org/26563004/diff/68001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1754: if (f->first & *t && Shouldn't you be checking largest_icon.bitmap_id == 0 here too?
PTAL https://codereview.chromium.org/26563004/diff/68001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/68001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1754: if (f->first & *t && On 2013/10/17 22:54:17, pkotwicz wrote: > Shouldn't you be checking largest_icon.bitmap_id == 0 here too? Correct, thanks
https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1766: GURL icon_url; Isn't it possible to get here and largest_icon not be > minimum_size_in_pixels? If that happens, do you really want to continue? https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:90: std::vector<chrome::FaviconBitmapResult> results; Again, why the vector here too?
LGTM with nits https://codereview.chromium.org/26563004/diff/81001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:175: // See HistoryService::GetLargestFaviconForURL. Nit: See HistoryService::GetLargestFaviconForURL() https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1702: int minimum_size_in_pixels, Nit: minimum_size_in_pixel (without the s) https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1712: if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings) || Nit: separate this into 2 if statements. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1746: // Find an icon which is larger than minimum_size_in_pixels in the order of Nit: |minimum_size_in_pixel| https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend_unittest.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:1925: // Tests GetFaviconsForURL with icon_types priority, GetFaviconForURL() -> GetLargestFaviconForURL() https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:1926: TEST_F(HistoryBackendTest, TestGetFaviconsForURLWithIconTypesPriority) { Rename test to GetLargestFaviconForURLPriority or something similar? https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:1952: Nit: No line break here https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:1967: TEST_F(HistoryBackendTest, TestGetFaviconsForURLReturnFavicon) { Rename test as well https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:1993: No line break here either https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend_unittest.cc:2029: // Verify 16x16 icon is returned, even it small than minimal_size. Nit: "even though it is smaller than |minimum_size_in_pixel|." https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:711: // greater than or equal to that of |minimum_size_in_pixels|. This searches Nit: Used by FaviconService to find the first favicon bitmap whose width and height are greater than or equal to |minimum_size_in_pixel|. I would rather use "favicon bitmap" instead of "icon" because "favicon bitmap" has a very specific meaning while "icon" does not. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:714: // If the largest icon of first icon types given by |icon_types| is not Nit: of |icon_types|[0] is not larger than ... https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:717: // If none icon is larger than |minimum_size_in_pixel|, the largest Nit: If no icon
PTAL https://codereview.chromium.org/26563004/diff/81001/chrome/browser/favicon/fa... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/favicon/fa... chrome/browser/favicon/favicon_service.h:175: // See HistoryService::GetLargestFaviconForURL. On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: See HistoryService::GetLargestFaviconForURL() Done. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1702: int minimum_size_in_pixels, This required by sky https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa... On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: minimum_size_in_pixel (without the s) https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1712: if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings) || This required by sky https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi... On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: separate this into 2 if statements. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1746: // Find an icon which is larger than minimum_size_in_pixels in the order of ditto On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: |minimum_size_in_pixel| https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1766: GURL icon_url; In this case, no icon is larger than minimum_size_in_pixels, return the largest one. On 2013/10/18 01:21:59, sky wrote: > Isn't it possible to get here and largest_icon not be > minimum_size_in_pixels? > If that happens, do you really want to continue? https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_service.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.cc:90: std::vector<chrome::FaviconBitmapResult> results; On 2013/10/18 01:21:59, sky wrote: > Again, why the vector here too? Done. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:711: // greater than or equal to that of |minimum_size_in_pixels|. This searches On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: Used by FaviconService to find the first favicon bitmap whose width and > height are greater than or equal to |minimum_size_in_pixel|. > > I would rather use "favicon bitmap" instead of "icon" because "favicon bitmap" > has a very specific meaning while "icon" does not. Done. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:711: // greater than or equal to that of |minimum_size_in_pixels|. This searches On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: Used by FaviconService to find the first favicon bitmap whose width and > height are greater than or equal to |minimum_size_in_pixel|. > > I would rather use "favicon bitmap" instead of "icon" because "favicon bitmap" > has a very specific meaning while "icon" does not. Done. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:714: // If the largest icon of first icon types given by |icon_types| is not On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: of |icon_types|[0] is not larger than ... Done. https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_service.h:717: // If none icon is larger than |minimum_size_in_pixel|, the largest On 2013/10/18 01:29:35, pkotwicz wrote: > Nit: If no icon Done.
https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1766: GURL icon_url; Then please update the docs. On 2013/10/18 04:37:02, michaelbai wrote: > In this case, no icon is larger than minimum_size_in_pixels, return the largest > one. > > > On 2013/10/18 01:21:59, sky wrote: > > Isn't it possible to get here and largest_icon not be > > minimum_size_in_pixels? > > If that happens, do you really want to continue? > https://codereview.chromium.org/26563004/diff/144001/chrome/browser/history/h... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/144001/chrome/browser/history/h... chrome/browser/history/history_service.h:725: const FaviconService::FaviconRawCallback& callback, Update docs for FaviconRawCallback too.
PTAL https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... File chrome/browser/history/history_backend.cc (right): https://codereview.chromium.org/26563004/diff/81001/chrome/browser/history/hi... chrome/browser/history/history_backend.cc:1766: GURL icon_url; The doc covers this case, did I miss something? On 2013/10/18 14:11:40, sky wrote: > Then please update the docs. > > On 2013/10/18 04:37:02, michaelbai wrote: > > In this case, no icon is larger than minimum_size_in_pixels, return the > largest > > one. > > > > > > On 2013/10/18 01:21:59, sky wrote: > > > Isn't it possible to get here and largest_icon not be > > > minimum_size_in_pixels? > > > If that happens, do you really want to continue? > > > https://codereview.chromium.org/26563004/diff/144001/chrome/browser/history/h... File chrome/browser/history/history_service.h (right): https://codereview.chromium.org/26563004/diff/144001/chrome/browser/history/h... chrome/browser/history/history_service.h:725: const FaviconService::FaviconRawCallback& callback, On 2013/10/18 14:11:40, sky wrote: > Update docs for FaviconRawCallback too. Done. https://codereview.chromium.org/26563004/diff/204001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/204001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:81: // Returns an invalid chrome::FaviconBitmapResult if there are no matches. Not sure whether we should have the detail description here, it seems duplicated, should user look for the method's document?
LGTM https://codereview.chromium.org/26563004/diff/204001/chrome/browser/favicon/f... File chrome/browser/favicon/favicon_service.h (right): https://codereview.chromium.org/26563004/diff/204001/chrome/browser/favicon/f... chrome/browser/favicon/favicon_service.h:81: // Returns an invalid chrome::FaviconBitmapResult if there are no matches. On 2013/10/18 17:31:49, michaelbai wrote: > Not sure whether we should have the detail description here, it seems > duplicated, should user look for the method's document? I agree. I was only thinking you need update the first sentence. Maybe the whole comment should be nuked and replaced with something like see function for details on value.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/26563004/254001
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=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/26563004/454001
Message was sent while issue was closed.
Change committed as 229760 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
