Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1119163003:
Save large icons to a new LARGE_ICON type
|
Created:
5 years, 7 months ago by Roger McFarlane (Chromium) Modified:
5 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSave large icons to a new LARGE_ICON type
This CL adds a new LARGE_ICON value to the favicon_base::IconType
enum. On platforms where large icons are not the preferred favicon
image (and are not currently download by default), the largest
icon declared by a URL, sourced from its favicons and/or touch
icons, is saved to the favicon database under this new icon type
value.
This allow the large icon to co-exist along side the current small
favicon population on desktop. On mobile, large favicon and touch
icons are already collected by default, so this additional functionality
is not enabled on mobile.
Notes:
- The enabling of this functionality is currently behind a
flag/experiment.
- A placeholder TODO denotes the location where the favicon
record could be added (and the URL processing skipped) to
transition to download-on-demand for large icon bitmaps.
Design Doc:
https://docs.google.com/document/d/1rv4x3goTWFJzQu5a_h94xfNhSUG3sU98lfxa1Prys1w/edit
BUG=467712
Patch Set 1 #Patch Set 2 : fix try bots #
Total comments: 10
Patch Set 3 : Address Sam's initial feedback. #
Total comments: 5
Patch Set 4 : reformat a comment #
Total comments: 13
Patch Set 5 : more clarifying comments. #Patch Set 6 : yet more clarifying comments #
Total comments: 6
Messages
Total messages: 38 (5 generated)
I revisited the "new icon type" design alternative. This is a MUCH simpler way to add the download of a single large icon for each mapped icon URL (it chooses the largest icon up front instead of capturing all of the sizes for later use). The large icons coexist alongside the favicons and fit into the current mapping management and bookmark-respecting garbage collection schemes. I've also marked the location where one could save the favicon record early and defer the download until demanded. Please take a look. I still need to add some LARGE_ICON specific unit tests, but I'll hold off until I have some feedback re: the approach.
rogerm@chromium.org changed reviewers: + beaudoin@chromium.org, huangs@chromium.org, pkotwicz@chromium.org
(adding reviewers for realz this time) I revisited the "new icon type" design alternative. This is a MUCH simpler way to add the download of a single large icon for each mapped icon URL (it chooses the largest icon up front instead of capturing all of the sizes for later use). The large icons coexist alongside the favicons and fit into the current mapping management and bookmark-respecting garbage collection schemes. I've also marked the location where one could save the favicon record early and defer the download until demanded. Please take a look. I still need to add some LARGE_ICON specific unit tests, but I'll hold off until I have some feedback re: the approach.
Oops forgot about this. We've discussed this result off line, and should continue with this on Monday. Here are some minor suggesitions. Also the bug fix in thumbnail_database.cc should be a separate CL. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon/core/favicon_handler.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/favicon_handler.cc:354: // TODO(rogerm): To use fetch on demand for large icons: just add the Favicon NIT: Perhaps the comment should be in ProcessCurrentUrl(), and replace "processing" with "downloading"? https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/favicon_handler.cc:552: return 192; I'd prefer return icon_type == favicon_base::INVALID_ICON ? 0 : 192; to keep flow with LARGE separate with the rest. Also, #if defined(OS_ANDROID) NOT_REACHED(); #endif https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:15: large_icon_types_.push_back(favicon_base::IconType::LARGE_ICON); NIT: Move this after TOUCH_PRECOMPOSED_ICON for consistency, or comment on why this comes first. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon_base/favicon_types.h:25: enum IconType { Need more comment here regarding real (?) icon types vs. virtual icon types. Specifically, which layers do these appear in. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/histo... File components/history/core/browser/thumbnail_database.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/histo... components/history/core/browser/thumbnail_database.cc:663: touch_icon_count.BindInt64(1, favicon_base::TOUCH_PRECOMPOSED_ICON); This looks like an independent bug fix. I'd recommend having this as a separate CL, and having it sped through the review process. Also please see how this changes UMA, and ping michaelbai@ (or whoever wrote this).
Thanks. Another look? https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon/core/favicon_handler.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/favicon_handler.cc:354: // TODO(rogerm): To use fetch on demand for large icons: just add the Favicon On 2015/05/04 05:17:30, huangs wrote: > NIT: Perhaps the comment should be in ProcessCurrentUrl(), and replace > "processing" with "downloading"? Done. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/favicon_handler.cc:552: return 192; On 2015/05/04 05:17:30, huangs wrote: > I'd prefer > return icon_type == favicon_base::INVALID_ICON ? 0 : 192; > to keep flow with LARGE separate with the rest. Done. > Also, > #if defined(OS_ANDROID) > NOT_REACHED(); > #endif Why? https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon/core/large_icon_service.cc:15: large_icon_types_.push_back(favicon_base::IconType::LARGE_ICON); On 2015/05/04 05:17:30, huangs wrote: > NIT: Move this after TOUCH_PRECOMPOSED_ICON for consistency, or comment on why > this comes first. Done. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/favic... components/favicon_base/favicon_types.h:25: enum IconType { On 2015/05/04 05:17:30, huangs wrote: > Need more comment here regarding real (?) icon types vs. virtual icon types. > Specifically, which layers do these appear in. Done. https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/histo... File components/history/core/browser/thumbnail_database.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/20001/components/histo... components/history/core/browser/thumbnail_database.cc:663: touch_icon_count.BindInt64(1, favicon_base::TOUCH_PRECOMPOSED_ICON); On 2015/05/04 05:17:30, huangs wrote: > This looks like an independent bug fix. I'd recommend having this as a separate > CL, and having it sped through the review process. Also please see how this > changes UMA, and ping michaelbai@ (or whoever wrote this). Moved to a separate CL. This is a new histogram I added a couple of weeks back. This was just an error that made it through review. No one else is consuming this histogram yet.
Note: there were a rebase-update in there that picked up some changes not related to this CL.
Looks good, but anticipating further discussion from pkotwicz@. https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... components/favicon/core/large_icon_service.cc:19: large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON); Please wrap TOUCH_ICON and TOUCH_PRECOMPOSED_ICON inside #if defined(OS_ANDROID) #endif Also, test with clean profile to ensure LARGE_ICON is indeed captured and used? https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... components/favicon_base/favicon_types.h:28: // favicons are small by default and larger touch icons are not the norm, NIT: the indent's a bit unusual, but your call.
https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... components/favicon/core/large_icon_service.cc:19: large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON); On 2015/05/04 20:00:44, huangs wrote: > Please wrap TOUCH_ICON and TOUCH_PRECOMPOSED_ICON inside > #if defined(OS_ANDROID) > #endif There's no need for platform specific behavior here. > Also, test with clean profile to ensure LARGE_ICON is indeed captured and used? That's how I tested. https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... components/favicon_base/favicon_types.h:28: // favicons are small by default and larger touch icons are not the norm, On 2015/05/04 20:00:44, huangs wrote: > NIT: the indent's a bit unusual, but your call. Done.
LGTM. https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... File components/favicon/core/large_icon_service.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/40001/components/favic... components/favicon/core/large_icon_service.cc:19: large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON); Ah I see, since LARGE_ICON would have higher priority anyway. Any existing TOUCH_ICON on desktop (which should only exist for early experimenters) would serve as fallback.
https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:456: // No more icons to request, set the favicon from the candidate. Augment the comment to indicate what save_type means? https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:551: return icon_type == favicon_base::INVALID_ICON ? 0 : 192; Comment to explain this logic? https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:566: return 192; Strange to return such a specific value behind a NOTREACHED()... https://codereview.chromium.org/1119163003/diff/60001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1119163003/diff/60001/components/favicon_base... components/favicon_base/favicon_types.h:29: // denotes the largest available icon the determined to be available. The NIT: icon determined to be available https://codereview.chromium.org/1119163003/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1119163003/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:1849: (favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON)); I didn't check carefully but noticed above you're using: icon_types = favicon_base::FAVICON | favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON Could it be possible for this DCHECK to fail?
Patchset #5 (id:80001) has been deleted
Thanks. Peter? https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:456: // No more icons to request, set the favicon from the candidate. On 2015/05/05 00:54:00, beaudoin wrote: > Augment the comment to indicate what save_type means? Done. https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:551: return icon_type == favicon_base::INVALID_ICON ? 0 : 192; On 2015/05/05 00:54:00, beaudoin wrote: > Comment to explain this logic? Done. https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:566: return 192; On 2015/05/05 00:54:00, beaudoin wrote: > Strange to return such a specific value behind a NOTREACHED()... We don't expect that GetMaximalIconSize will be called with LARGE_ICON because this method is used to determine the size of a "real" icon to be downloaded; hence the NOTREACHED. But, the correct value to return, should this ever occur is 192. So, I've set up the code to do the "right" thing, and added an assertion so that we're alerted if this every unexpectedly called in this way. https://codereview.chromium.org/1119163003/diff/60001/components/favicon_base... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/1119163003/diff/60001/components/favicon_base... components/favicon_base/favicon_types.h:29: // denotes the largest available icon the determined to be available. The On 2015/05/05 00:54:00, beaudoin wrote: > NIT: icon determined to be available Done. https://codereview.chromium.org/1119163003/diff/60001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1119163003/diff/60001/components/history/core... components/history/core/browser/history_backend.cc:1849: (favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON)); On 2015/05/05 00:54:00, beaudoin wrote: > I didn't check carefully but noticed above you're using: > > icon_types = favicon_base::FAVICON | favicon_base::TOUCH_ICON | > favicon_base::TOUCH_PRECOMPOSED_ICON > > Could it be possible for this DCHECK to fail? If this DCHECK fails, it is catching a true error. in favicion_handler.cc: The usage of FAVICON | TOUCH_ICON | TOUCH_PRECOMPOSED is a mask where the (LARGE) favicon handler selects which types of icons it should consider as candidates. The icons do not get tagged with the ORed mask. in history_backend.cc: the usage is to update mappings for a given stored icon. An icon is stored using a single type, and the mask above is to select which types of icons to update with the given mapping. The final bitwise ORed mask is to treat the two touch icon types are equivalent.
ping?
LGTM with one optional improvement to save future readers a WTF moment. :) https://chromiumcodereview.appspot.com/1119163003/diff/60001/components/favic... File components/favicon/core/favicon_handler.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/60001/components/favic... components/favicon/core/favicon_handler.cc:566: return 192; On 2015/05/05 15:09:47, Roger McFarlane (Chromium) wrote: > On 2015/05/05 00:54:00, beaudoin wrote: > > Strange to return such a specific value behind a NOTREACHED()... > > We don't expect that GetMaximalIconSize will be called with LARGE_ICON because > this method is used to determine the size of a "real" icon to be downloaded; > hence the NOTREACHED. > > But, the correct value to return, should this ever occur is 192. > > So, I've set up the code to do the "right" thing, and added an assertion so that > we're alerted if this every unexpectedly called in this way. Acknowledged, although a quick comment to explain this might save a WTF moment to a future reader. https://chromiumcodereview.appspot.com/1119163003/diff/60001/components/histo... File components/history/core/browser/history_backend.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/60001/components/histo... components/history/core/browser/history_backend.cc:1849: (favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON)); On 2015/05/05 15:09:47, Roger McFarlane (Chromium) wrote: > On 2015/05/05 00:54:00, beaudoin wrote: > > I didn't check carefully but noticed above you're using: > > > > icon_types = favicon_base::FAVICON | favicon_base::TOUCH_ICON | > > favicon_base::TOUCH_PRECOMPOSED_ICON > > > > Could it be possible for this DCHECK to fail? > > If this DCHECK fails, it is catching a true error. > > in favicion_handler.cc: The usage of FAVICON | TOUCH_ICON | TOUCH_PRECOMPOSED is > a mask where the (LARGE) favicon handler selects which types of icons it should > consider as candidates. The icons do not get tagged with the ORed mask. > > in history_backend.cc: the usage is to update mappings for a given stored icon. > An icon is stored using a single type, and the mask above is to select which > types of icons to update with the given mapping. The final bitwise ORed mask is > to treat the two touch icon types are equivalent. > Acknowledged.
rogerm@chromium.org changed reviewers: + sky@chromium.org
Thanks. +sky@ for OWNERS https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:566: return 192; On 2015/05/05 19:47:52, beaudoin wrote: > On 2015/05/05 15:09:47, Roger McFarlane (Chromium) wrote: > > On 2015/05/05 00:54:00, beaudoin wrote: > > > Strange to return such a specific value behind a NOTREACHED()... > > > > We don't expect that GetMaximalIconSize will be called with LARGE_ICON because > > this method is used to determine the size of a "real" icon to be downloaded; > > hence the NOTREACHED. > > > > But, the correct value to return, should this ever occur is 192. > > > > So, I've set up the code to do the "right" thing, and added an assertion so > that > > we're alerted if this every unexpectedly called in this way. > > Acknowledged, although a quick comment to explain this might save a WTF moment > to a future reader. Done.
On 2015/05/06 16:06:16, Roger McFarlane (Chromium) wrote: > Thanks. > > +sky@ for OWNERS > > https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... > File components/favicon/core/favicon_handler.cc (right): > > https://codereview.chromium.org/1119163003/diff/60001/components/favicon/core... > components/favicon/core/favicon_handler.cc:566: return 192; > On 2015/05/05 19:47:52, beaudoin wrote: > > On 2015/05/05 15:09:47, Roger McFarlane (Chromium) wrote: > > > On 2015/05/05 00:54:00, beaudoin wrote: > > > > Strange to return such a specific value behind a NOTREACHED()... > > > > > > We don't expect that GetMaximalIconSize will be called with LARGE_ICON > because > > > this method is used to determine the size of a "real" icon to be downloaded; > > > hence the NOTREACHED. > > > > > > But, the correct value to return, should this ever occur is 192. > > > > > > So, I've set up the code to do the "right" thing, and added an assertion so > > that > > > we're alerted if this every unexpectedly called in this way. > > > > Acknowledged, although a quick comment to explain this might save a WTF moment > > to a future reader. > > Done. I have a couple of questions on https://codereview.chromium.org/1010293002/ that I posted there, could you make sure they get answered?
I'm not convinced we need a new type at the db level. Also, I thought the design was to download on demand. With this patch don't you download always? https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... File components/favicon/core/favicon_handler.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon/core/favicon_handler.cc:231: return favicon_base::FAVICON | favicon_base::TOUCH_ICON | Why do we need FAVICON here too? If you use FAVICON here won't that mean you'll have two FaviconHandlers attempting to download FAVICON? https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon/core/favicon_handler.cc:570: // type, so we're not expecting a call to get a maximal size to downlaod. downlaod->download https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 Why do we need the new type?
Re: Download on demand. This is a precursor CL to add large favicon handling to the existing download flow. Philippe is working on the CL to wire in download on demand once the flow to capture the URLs of large icons is locked down (hopefully by this CL, plus one to only save the favicon record and skip the download). The near-term goal is to have the user-visible icons-on-the-NTP functionality behind a flag so chrome leadership can live with the icon based NTP for awhile, so we can get consensus on whether this is the UI direction Chrome desktop should take. (Mobile already downloads large icons as you browse) https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... File components/favicon/core/favicon_handler.cc (right): https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon/core/favicon_handler.cc:231: return favicon_base::FAVICON | favicon_base::TOUCH_ICON | On 2015/05/06 19:50:42, sky wrote: > Why do we need FAVICON here too? If you use FAVICON here won't that mean you'll > have two FaviconHandlers attempting to download FAVICON? Yes and no. Attempts to get the existing FAVICON and TOUCH handlers to do all the work were unsuccessful (see https://codereview.chromium.org/1118073002/) because the handler seems to only capture a single representation of the icon. I.e., if you tell the regular favicon handler to capture a large icon, it forgoes the 16x16 version which is needed for tab rendering. If you have any suggestions on how to fix that, I'd be glad to hear them and go back to that approach. But, it's not quite as bad as it may seem... The issue you've brought up occurs when the small and large icon are in the same file (e.g., an ICO file that stores both the small and the large images or a single one-size fits all icon). In that case, the downside is that the file will be downloaded twice (because the image fetcher bypasses the cache) but the handler will (potentially) extract different representations from it. In the more general case, the small and large icon are in different files, possibly across different icon types. In this case, multiple downloads and handlers are exactly what is needed. In both cases, you want the bitmaps and the mappings to be updated indepedently for the small and large icons, hence their treatment as different icon types by the backend. https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon/core/favicon_handler.cc:570: // type, so we're not expecting a call to get a maximal size to downlaod. On 2015/05/06 19:50:42, sky wrote: > downlaod->download Will do. https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... File components/favicon_base/favicon_types.h (right): https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 On 2015/05/06 19:50:42, sky wrote: > Why do we need the new type? The way the url-to-icon mappings are managed causes multiple representations of the same IconType to step all over one another. For example, if there two link-rel-icon entries in an html file one for 16x16 and one for 192x192, both get mapped to the FAVICON type in the DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, there is no good way to determine whether or not it should replace the 16x16 or be added alongside it. They both claim to be the FAVICON for the page. The current mapping update logic causes the 16x16 would be unmapped and deleted.
I'm confused about this patch. You say download on demand but this isn't download on demand, right? -Scott On Wed, May 6, 2015 at 2:22 PM, <rogerm@chromium.org> wrote: > Re: Download on demand. > > This is a precursor CL to add large favicon handling to the existing > download > flow. Philippe is working on the CL to wire in download on demand once the > flow > to capture the URLs of large icons is locked down (hopefully by this CL, > plus > one to only save the favicon record and skip the download). > > The near-term goal is to have the user-visible icons-on-the-NTP > functionality > behind a flag so chrome leadership can live with the icon based NTP for > awhile, > so we can get consensus on whether this is the UI direction Chrome desktop > should take. (Mobile already downloads large icons as you browse) > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > File components/favicon/core/favicon_handler.cc (right): > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon/core/favicon_handler.cc:231: return > favicon_base::FAVICON | favicon_base::TOUCH_ICON | > On 2015/05/06 19:50:42, sky wrote: >> >> Why do we need FAVICON here too? If you use FAVICON here won't that > > mean you'll >> >> have two FaviconHandlers attempting to download FAVICON? > > > Yes and no. > > Attempts to get the existing FAVICON and TOUCH handlers to do all the > work were unsuccessful (see https://codereview.chromium.org/1118073002/) > because the handler seems to only capture a single representation of the > icon. I.e., if you tell the regular favicon handler to capture a large > icon, it forgoes the 16x16 version which is needed for tab rendering. > > If you have any suggestions on how to fix that, I'd be glad to hear them > and go back to that approach. > > But, it's not quite as bad as it may seem... > > The issue you've brought up occurs when the small and large icon are in > the same file (e.g., an ICO file that stores both the small and the > large images or a single one-size fits all icon). In that case, the > downside is that the file will be downloaded twice (because the image > fetcher bypasses the cache) but the handler will (potentially) extract > different representations from it. > > In the more general case, the small and large icon are in different > files, possibly across different icon types. In this case, multiple > downloads and handlers are exactly what is needed. > > In both cases, you want the bitmaps and the mappings to be updated > indepedently for the small and large icons, hence their treatment as > different icon types by the backend. > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon/core/favicon_handler.cc:570: // type, so we're not > expecting a call to get a maximal size to downlaod. > On 2015/05/06 19:50:42, sky wrote: >> >> downlaod->download > > > Will do. > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > File components/favicon_base/favicon_types.h (right): > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > On 2015/05/06 19:50:42, sky wrote: >> >> Why do we need the new type? > > > The way the url-to-icon mappings are managed causes multiple > representations of the same IconType to step all over one another. > > For example, if there two link-rel-icon entries in an html file one for > 16x16 and one for 192x192, both get mapped to the FAVICON type in the > DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, > there is no good way to determine whether or not it should replace the > 16x16 or be added alongside it. They both claim to be the FAVICON for > the page. > > The current mapping update logic causes the 16x16 would be unmapped and > deleted. > > https://chromiumcodereview.appspot.com/1119163003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/07 15:42:11, sky wrote: > I'm confused about this patch. You say download on demand but this > isn't download on demand, right? > > -Scott > > On Wed, May 6, 2015 at 2:22 PM, <mailto:rogerm@chromium.org> wrote: > > Re: Download on demand. > > > > This is a precursor CL to add large favicon handling to the existing > > download > > flow. Philippe is working on the CL to wire in download on demand once the > > flow > > to capture the URLs of large icons is locked down (hopefully by this CL, > > plus > > one to only save the favicon record and skip the download). > > > > The near-term goal is to have the user-visible icons-on-the-NTP > > functionality > > behind a flag so chrome leadership can live with the icon based NTP for > > awhile, > > so we can get consensus on whether this is the UI direction Chrome desktop > > should take. (Mobile already downloads large icons as you browse) > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > > File components/favicon/core/favicon_handler.cc (right): > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > > components/favicon/core/favicon_handler.cc:231: return > > favicon_base::FAVICON | favicon_base::TOUCH_ICON | > > On 2015/05/06 19:50:42, sky wrote: > >> > >> Why do we need FAVICON here too? If you use FAVICON here won't that > > > > mean you'll > >> > >> have two FaviconHandlers attempting to download FAVICON? > > > > > > Yes and no. > > > > Attempts to get the existing FAVICON and TOUCH handlers to do all the > > work were unsuccessful (see https://codereview.chromium.org/1118073002/) > > because the handler seems to only capture a single representation of the > > icon. I.e., if you tell the regular favicon handler to capture a large > > icon, it forgoes the 16x16 version which is needed for tab rendering. > > > > If you have any suggestions on how to fix that, I'd be glad to hear them > > and go back to that approach. > > > > But, it's not quite as bad as it may seem... > > > > The issue you've brought up occurs when the small and large icon are in > > the same file (e.g., an ICO file that stores both the small and the > > large images or a single one-size fits all icon). In that case, the > > downside is that the file will be downloaded twice (because the image > > fetcher bypasses the cache) but the handler will (potentially) extract > > different representations from it. > > > > In the more general case, the small and large icon are in different > > files, possibly across different icon types. In this case, multiple > > downloads and handlers are exactly what is needed. > > > > In both cases, you want the bitmaps and the mappings to be updated > > indepedently for the small and large icons, hence their treatment as > > different icon types by the backend. > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > > components/favicon/core/favicon_handler.cc:570: // type, so we're not > > expecting a call to get a maximal size to downlaod. > > On 2015/05/06 19:50:42, sky wrote: > >> > >> downlaod->download > > > > > > Will do. > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > > File components/favicon_base/favicon_types.h (right): > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > > On 2015/05/06 19:50:42, sky wrote: > >> > >> Why do we need the new type? > > > > > > The way the url-to-icon mappings are managed causes multiple > > representations of the same IconType to step all over one another. > > > > For example, if there two link-rel-icon entries in an html file one for > > 16x16 and one for 192x192, both get mapped to the FAVICON type in the > > DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, > > there is no good way to determine whether or not it should replace the > > 16x16 or be added alongside it. They both claim to be the FAVICON for > > the page. > > > > The current mapping update logic causes the 16x16 would be unmapped and > > deleted. > > > > https://chromiumcodereview.appspot.com/1119163003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. This is a precursor to download-on-demand. I've marked the place where the change needs to be in order to turn off the large icon download while saving the url of the icon you need to download when/if the demand arrives. With this CL, the goal is to review/approve that this is the scheme we want to use to capture this information (i.e., the LARGE_ICON type).
My concern is that this is downloading an saving right now, not what was discussed. If you disable creation of FaviconHandler for LARGE, then I'm ok with it. Well, I still have to look through the rest. On Thu, May 7, 2015 at 8:54 AM, <rogerm@chromium.org> wrote: > On 2015/05/07 15:42:11, sky wrote: >> >> I'm confused about this patch. You say download on demand but this >> isn't download on demand, right? > > >> -Scott > > >> On Wed, May 6, 2015 at 2:22 PM, <mailto:rogerm@chromium.org> wrote: >> > Re: Download on demand. >> > >> > This is a precursor CL to add large favicon handling to the existing >> > download >> > flow. Philippe is working on the CL to wire in download on demand once >> > the >> > flow >> > to capture the URLs of large icons is locked down (hopefully by this CL, >> > plus >> > one to only save the favicon record and skip the download). >> > >> > The near-term goal is to have the user-visible icons-on-the-NTP >> > functionality >> > behind a flag so chrome leadership can live with the icon based NTP for >> > awhile, >> > so we can get consensus on whether this is the UI direction Chrome >> > desktop >> > should take. (Mobile already downloads large icons as you browse) >> > >> > >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> > File components/favicon/core/favicon_handler.cc (right): >> > >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> > components/favicon/core/favicon_handler.cc:231: return >> > favicon_base::FAVICON | favicon_base::TOUCH_ICON | >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> Why do we need FAVICON here too? If you use FAVICON here won't that >> > >> > mean you'll >> >> >> >> have two FaviconHandlers attempting to download FAVICON? >> > >> > >> > Yes and no. >> > >> > Attempts to get the existing FAVICON and TOUCH handlers to do all the >> > work were unsuccessful (see https://codereview.chromium.org/1118073002/) >> > because the handler seems to only capture a single representation of the >> > icon. I.e., if you tell the regular favicon handler to capture a large >> > icon, it forgoes the 16x16 version which is needed for tab rendering. >> > >> > If you have any suggestions on how to fix that, I'd be glad to hear them >> > and go back to that approach. >> > >> > But, it's not quite as bad as it may seem... >> > >> > The issue you've brought up occurs when the small and large icon are in >> > the same file (e.g., an ICO file that stores both the small and the >> > large images or a single one-size fits all icon). In that case, the >> > downside is that the file will be downloaded twice (because the image >> > fetcher bypasses the cache) but the handler will (potentially) extract >> > different representations from it. >> > >> > In the more general case, the small and large icon are in different >> > files, possibly across different icon types. In this case, multiple >> > downloads and handlers are exactly what is needed. >> > >> > In both cases, you want the bitmaps and the mappings to be updated >> > indepedently for the small and large icons, hence their treatment as >> > different icon types by the backend. >> > >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> > components/favicon/core/favicon_handler.cc:570: // type, so we're not >> > expecting a call to get a maximal size to downlaod. >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> downlaod->download >> > >> > >> > Will do. >> > >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> > File components/favicon_base/favicon_types.h (right): >> > >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> Why do we need the new type? >> > >> > >> > The way the url-to-icon mappings are managed causes multiple >> > representations of the same IconType to step all over one another. >> > >> > For example, if there two link-rel-icon entries in an html file one for >> > 16x16 and one for 192x192, both get mapped to the FAVICON type in the >> > DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, >> > there is no good way to determine whether or not it should replace the >> > 16x16 or be added alongside it. They both claim to be the FAVICON for >> > the page. >> > >> > The current mapping update logic causes the 16x16 would be unmapped and >> > deleted. >> > >> > https://chromiumcodereview.appspot.com/1119163003/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > This is a precursor to download-on-demand. I've marked the place where the > change needs to be in order to turn off the large icon download while saving > the > url of the icon you need to download when/if the demand arrives. > > With this CL, the goal is to review/approve that this is the scheme we want > to > use to capture this information (i.e., the LARGE_ICON type). > > https://chromiumcodereview.appspot.com/1119163003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The FaviconHandler for LARGE is only enabled behind a flag, which is only manually specified (i.e., is not the target of any planned end-user experiments). See https://code.google.com/p/chromium/codesearch#chromium/src/components/favicon... On Thu, May 7, 2015 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: > My concern is that this is downloading an saving right now, not what > was discussed. If you disable creation of FaviconHandler for LARGE, > then I'm ok with it. Well, I still have to look through the rest. > > On Thu, May 7, 2015 at 8:54 AM, <rogerm@chromium.org> wrote: > > On 2015/05/07 15:42:11, sky wrote: > >> > >> I'm confused about this patch. You say download on demand but this > >> isn't download on demand, right? > > > > > >> -Scott > > > > > >> On Wed, May 6, 2015 at 2:22 PM, <mailto:rogerm@chromium.org> wrote: > >> > Re: Download on demand. > >> > > >> > This is a precursor CL to add large favicon handling to the existing > >> > download > >> > flow. Philippe is working on the CL to wire in download on demand once > >> > the > >> > flow > >> > to capture the URLs of large icons is locked down (hopefully by this > CL, > >> > plus > >> > one to only save the favicon record and skip the download). > >> > > >> > The near-term goal is to have the user-visible icons-on-the-NTP > >> > functionality > >> > behind a flag so chrome leadership can live with the icon based NTP > for > >> > awhile, > >> > so we can get consensus on whether this is the UI direction Chrome > >> > desktop > >> > should take. (Mobile already downloads large icons as you browse) > >> > > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> > >> > File components/favicon/core/favicon_handler.cc (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> > >> > components/favicon/core/favicon_handler.cc:231: return > >> > favicon_base::FAVICON | favicon_base::TOUCH_ICON | > >> > On 2015/05/06 19:50:42, sky wrote: > >> >> > >> >> Why do we need FAVICON here too? If you use FAVICON here won't that > >> > > >> > mean you'll > >> >> > >> >> have two FaviconHandlers attempting to download FAVICON? > >> > > >> > > >> > Yes and no. > >> > > >> > Attempts to get the existing FAVICON and TOUCH handlers to do all the > >> > work were unsuccessful (see > https://codereview.chromium.org/1118073002/) > >> > because the handler seems to only capture a single representation of > the > >> > icon. I.e., if you tell the regular favicon handler to capture a large > >> > icon, it forgoes the 16x16 version which is needed for tab rendering. > >> > > >> > If you have any suggestions on how to fix that, I'd be glad to hear > them > >> > and go back to that approach. > >> > > >> > But, it's not quite as bad as it may seem... > >> > > >> > The issue you've brought up occurs when the small and large icon are > in > >> > the same file (e.g., an ICO file that stores both the small and the > >> > large images or a single one-size fits all icon). In that case, the > >> > downside is that the file will be downloaded twice (because the image > >> > fetcher bypasses the cache) but the handler will (potentially) extract > >> > different representations from it. > >> > > >> > In the more general case, the small and large icon are in different > >> > files, possibly across different icon types. In this case, multiple > >> > downloads and handlers are exactly what is needed. > >> > > >> > In both cases, you want the bitmaps and the mappings to be updated > >> > indepedently for the small and large icons, hence their treatment as > >> > different icon types by the backend. > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> > >> > components/favicon/core/favicon_handler.cc:570: // type, so we're not > >> > expecting a call to get a maximal size to downlaod. > >> > On 2015/05/06 19:50:42, sky wrote: > >> >> > >> >> downlaod->download > >> > > >> > > >> > Will do. > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> > >> > File components/favicon_base/favicon_types.h (right): > >> > > >> > > > > > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> > >> > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > >> > On 2015/05/06 19:50:42, sky wrote: > >> >> > >> >> Why do we need the new type? > >> > > >> > > >> > The way the url-to-icon mappings are managed causes multiple > >> > representations of the same IconType to step all over one another. > >> > > >> > For example, if there two link-rel-icon entries in an html file one > for > >> > 16x16 and one for 192x192, both get mapped to the FAVICON type in the > >> > DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, > >> > there is no good way to determine whether or not it should replace the > >> > 16x16 or be added alongside it. They both claim to be the FAVICON for > >> > the page. > >> > > >> > The current mapping update logic causes the 16x16 would be unmapped > and > >> > deleted. > >> > > >> > https://chromiumcodereview.appspot.com/1119163003/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > This is a precursor to download-on-demand. I've marked the place where > the > > change needs to be in order to turn off the large icon download while > saving > > the > > url of the icon you need to download when/if the demand arrives. > > > > With this CL, the goal is to review/approve that this is the scheme we > want > > to > > use to capture this information (i.e., the LARGE_ICON type). > > > > https://chromiumcodereview.appspot.com/1119163003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yep. And I'm saying you remove that check and never create LARGE until this doesn't save. -Scott On Thu, May 7, 2015 at 9:09 AM, Roger McFarlane <rogerm@chromium.org> wrote: > The FaviconHandler for LARGE is only enabled behind a flag, which is only > manually specified (i.e., is not the target of any planned end-user > experiments). > > See > https://code.google.com/p/chromium/codesearch#chromium/src/components/favicon... > > > > On Thu, May 7, 2015 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: >> >> My concern is that this is downloading an saving right now, not what >> was discussed. If you disable creation of FaviconHandler for LARGE, >> then I'm ok with it. Well, I still have to look through the rest. >> >> On Thu, May 7, 2015 at 8:54 AM, <rogerm@chromium.org> wrote: >> > On 2015/05/07 15:42:11, sky wrote: >> >> >> >> I'm confused about this patch. You say download on demand but this >> >> isn't download on demand, right? >> > >> > >> >> -Scott >> > >> > >> >> On Wed, May 6, 2015 at 2:22 PM, <mailto:rogerm@chromium.org> wrote: >> >> > Re: Download on demand. >> >> > >> >> > This is a precursor CL to add large favicon handling to the existing >> >> > download >> >> > flow. Philippe is working on the CL to wire in download on demand >> >> > once >> >> > the >> >> > flow >> >> > to capture the URLs of large icons is locked down (hopefully by this >> >> > CL, >> >> > plus >> >> > one to only save the favicon record and skip the download). >> >> > >> >> > The near-term goal is to have the user-visible icons-on-the-NTP >> >> > functionality >> >> > behind a flag so chrome leadership can live with the icon based NTP >> >> > for >> >> > awhile, >> >> > so we can get consensus on whether this is the UI direction Chrome >> >> > desktop >> >> > should take. (Mobile already downloads large icons as you browse) >> >> > >> >> > >> >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >> >> > File components/favicon/core/favicon_handler.cc (right): >> >> > >> >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >> >> > components/favicon/core/favicon_handler.cc:231: return >> >> > favicon_base::FAVICON | favicon_base::TOUCH_ICON | >> >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> >> >> Why do we need FAVICON here too? If you use FAVICON here won't that >> >> > >> >> > mean you'll >> >> >> >> >> >> have two FaviconHandlers attempting to download FAVICON? >> >> > >> >> > >> >> > Yes and no. >> >> > >> >> > Attempts to get the existing FAVICON and TOUCH handlers to do all the >> >> > work were unsuccessful (see >> >> > https://codereview.chromium.org/1118073002/) >> >> > because the handler seems to only capture a single representation of >> >> > the >> >> > icon. I.e., if you tell the regular favicon handler to capture a >> >> > large >> >> > icon, it forgoes the 16x16 version which is needed for tab rendering. >> >> > >> >> > If you have any suggestions on how to fix that, I'd be glad to hear >> >> > them >> >> > and go back to that approach. >> >> > >> >> > But, it's not quite as bad as it may seem... >> >> > >> >> > The issue you've brought up occurs when the small and large icon are >> >> > in >> >> > the same file (e.g., an ICO file that stores both the small and the >> >> > large images or a single one-size fits all icon). In that case, the >> >> > downside is that the file will be downloaded twice (because the image >> >> > fetcher bypasses the cache) but the handler will (potentially) >> >> > extract >> >> > different representations from it. >> >> > >> >> > In the more general case, the small and large icon are in different >> >> > files, possibly across different icon types. In this case, multiple >> >> > downloads and handlers are exactly what is needed. >> >> > >> >> > In both cases, you want the bitmaps and the mappings to be updated >> >> > indepedently for the small and large icons, hence their treatment as >> >> > different icon types by the backend. >> >> > >> >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >> >> > components/favicon/core/favicon_handler.cc:570: // type, so we're not >> >> > expecting a call to get a maximal size to downlaod. >> >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> >> >> downlaod->download >> >> > >> >> > >> >> > Will do. >> >> > >> >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >> >> > File components/favicon_base/favicon_types.h (right): >> >> > >> >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >> >> > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >> >> > On 2015/05/06 19:50:42, sky wrote: >> >> >> >> >> >> Why do we need the new type? >> >> > >> >> > >> >> > The way the url-to-icon mappings are managed causes multiple >> >> > representations of the same IconType to step all over one another. >> >> > >> >> > For example, if there two link-rel-icon entries in an html file one >> >> > for >> >> > 16x16 and one for 192x192, both get mapped to the FAVICON type in the >> >> > DB. Say the 16x16 gets inserted first. When the 192x192 gets mapped, >> >> > there is no good way to determine whether or not it should replace >> >> > the >> >> > 16x16 or be added alongside it. They both claim to be the FAVICON for >> >> > the page. >> >> > >> >> > The current mapping update logic causes the 16x16 would be unmapped >> >> > and >> >> > deleted. >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > This is a precursor to download-on-demand. I've marked the place where >> > the >> > change needs to be in order to turn off the large icon download while >> > saving >> > the >> > url of the icon you need to download when/if the demand arrives. >> > >> > With this CL, the goal is to review/approve that this is the scheme we >> > want >> > to >> > use to capture this information (i.e., the LARGE_ICON type). >> > >> > https://chromiumcodereview.appspot.com/1119163003/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
My understanding of what we agreed upon is download on demand. To do download on demand you need to know the set of urls images are at, and potentially download one of them. We currently aren't recording this information. That is, FaviconHandler selects the best image to use, downloads it, and saves the image for later use, dropping the list of potential urls. I don't think an additional FaviconHandler for LARGE is what we want. Rather we want one of the FaviconHandlers (FAVICON) to record all the possible urls. When the NTP (or whoever) wants a different icon it can get the set of possible urls, with already downloaded images and then make the decision as to whether it wants to download something knew. It may be possible to use FaviconHandler for this, but I suspect it would be easier not to. On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: > Re: Download on demand. > > This is a precursor CL to add large favicon handling to the existing download > flow. Philippe is working on the CL to wire in download on demand once the flow > to capture the URLs of large icons is locked down (hopefully by this CL, plus > one to only save the favicon record and skip the download). > > The near-term goal is to have the user-visible icons-on-the-NTP functionality > behind a flag so chrome leadership can live with the icon based NTP for awhile, > so we can get consensus on whether this is the UI direction Chrome desktop > should take. (Mobile already downloads large icons as you browse) > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > File components/favicon/core/favicon_handler.cc (right): > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon/core/favicon_handler.cc:231: return favicon_base::FAVICON | > favicon_base::TOUCH_ICON | > On 2015/05/06 19:50:42, sky wrote: > > Why do we need FAVICON here too? If you use FAVICON here won't that mean > you'll > > have two FaviconHandlers attempting to download FAVICON? > > Yes and no. > > Attempts to get the existing FAVICON and TOUCH handlers to do all the work were > unsuccessful (see https://codereview.chromium.org/1118073002/) because the > handler seems to only capture a single representation of the icon. I.e., if you > tell the regular favicon handler to capture a large icon, it forgoes the 16x16 > version which is needed for tab rendering. > > If you have any suggestions on how to fix that, I'd be glad to hear them and go > back to that approach. > > But, it's not quite as bad as it may seem... > > The issue you've brought up occurs when the small and large icon are in the same > file (e.g., an ICO file that stores both the small and the large images or a > single one-size fits all icon). In that case, the downside is that the file will > be downloaded twice (because the image fetcher bypasses the cache) but the > handler will (potentially) extract different representations from it. > > In the more general case, the small and large icon are in different files, > possibly across different icon types. In this case, multiple downloads and > handlers are exactly what is needed. > > In both cases, you want the bitmaps and the mappings to be updated indepedently > for the small and large icons, hence their treatment as different icon types by > the backend. > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon/core/favicon_handler.cc:570: // type, so we're not expecting > a call to get a maximal size to downlaod. > On 2015/05/06 19:50:42, sky wrote: > > downlaod->download > > Will do. > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > File components/favicon_base/favicon_types.h (right): > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > On 2015/05/06 19:50:42, sky wrote: > > Why do we need the new type? > > The way the url-to-icon mappings are managed causes multiple representations of > the same IconType to step all over one another. > > For example, if there two link-rel-icon entries in an html file one for 16x16 > and one for 192x192, both get mapped to the FAVICON type in the DB. Say the > 16x16 gets inserted first. When the 192x192 gets mapped, there is no good way to > determine whether or not it should replace the 16x16 or be added alongside it. > They both claim to be the FAVICON for the page. > > The current mapping update logic causes the 16x16 would be unmapped and deleted.
Yes, download on demand is what we agreed to and what we're moving towards. At it's simplest, for download on demand, we don't need to save the full set of URLs, we only need to save the one most appropriate URL. This CL is recording that information AND downloading the image, the latter part not being what we want. But I was trying to make incremental changes to get there, and the download-on-demand part depends on the URL info being in the DB, so this came first. We don't want the FAVICON handler to record URLs, specifically because that would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, eventually manifest, etc). Neither do we want the TOUCH handler, because that excludes the FAVICON. Instead we need a handler that considers all of the icon candidates and selects the best one for a large icon. Saving the complete list of URLs and their sizes is still a viable approach. It just means that the TODO I added is in the wrong place and that the URLs need to be saved and the process aborted just before the list would have been sorted and pruned. But, we would still need a way to keep those favicon records distinct from those managed as regular FAVICONs and TOUCH*ICONS, or they end up getting clobbered by the regular favicon download and mapping process. In short, we'd still need something like LARGE_ICON ... or... we need a pretty large overhaul of the history backend's handling of favicons ... or ... we need some new place to store this info ... or ... there's something we've missed (which is entirely possible)? On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: > My understanding of what we agreed upon is download on demand. To do > download on > demand you need to know the set of urls images are at, and potentially > download > one of them. We currently aren't recording this information. That is, > FaviconHandler selects the best image to use, downloads it, and saves the > image > for later use, dropping the list of potential urls. I don't think an > additional > FaviconHandler for LARGE is what we want. Rather we want one of the > FaviconHandlers (FAVICON) to record all the possible urls. When the NTP (or > whoever) wants a different icon it can get the set of possible urls, with > already downloaded images and then make the decision as to whether it > wants to > download something knew. It may be possible to use FaviconHandler for > this, but > I suspect it would be easier not to. > > > On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: > >> Re: Download on demand. >> > > This is a precursor CL to add large favicon handling to the existing >> download >> flow. Philippe is working on the CL to wire in download on demand once the >> > flow > >> to capture the URLs of large icons is locked down (hopefully by this CL, >> plus >> one to only save the favicon record and skip the download). >> > > The near-term goal is to have the user-visible icons-on-the-NTP >> functionality >> behind a flag so chrome leadership can live with the icon based NTP for >> > awhile, > >> so we can get consensus on whether this is the UI direction Chrome desktop >> should take. (Mobile already downloads large icons as you browse) >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> File components/favicon/core/favicon_handler.cc (right): >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> components/favicon/core/favicon_handler.cc:231: return >> favicon_base::FAVICON | >> favicon_base::TOUCH_ICON | >> On 2015/05/06 19:50:42, sky wrote: >> > Why do we need FAVICON here too? If you use FAVICON here won't that mean >> you'll >> > have two FaviconHandlers attempting to download FAVICON? >> > > Yes and no. >> > > Attempts to get the existing FAVICON and TOUCH handlers to do all the work >> > were > >> unsuccessful (see https://codereview.chromium.org/1118073002/) because >> the >> handler seems to only capture a single representation of the icon. I.e., >> if >> > you > >> tell the regular favicon handler to capture a large icon, it forgoes the >> 16x16 >> version which is needed for tab rendering. >> > > If you have any suggestions on how to fix that, I'd be glad to hear them >> and >> > go > >> back to that approach. >> > > But, it's not quite as bad as it may seem... >> > > The issue you've brought up occurs when the small and large icon are in >> the >> > same > >> file (e.g., an ICO file that stores both the small and the large images >> or a >> single one-size fits all icon). In that case, the downside is that the >> file >> > will > >> be downloaded twice (because the image fetcher bypasses the cache) but the >> handler will (potentially) extract different representations from it. >> > > In the more general case, the small and large icon are in different files, >> possibly across different icon types. In this case, multiple downloads and >> handlers are exactly what is needed. >> > > In both cases, you want the bitmaps and the mappings to be updated >> > indepedently > >> for the small and large icons, hence their treatment as different icon >> types >> > by > >> the backend. >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> components/favicon/core/favicon_handler.cc:570: // type, so we're not >> > expecting > >> a call to get a maximal size to downlaod. >> On 2015/05/06 19:50:42, sky wrote: >> > downlaod->download >> > > Will do. >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> File components/favicon_base/favicon_types.h (right): >> > > > > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >> On 2015/05/06 19:50:42, sky wrote: >> > Why do we need the new type? >> > > The way the url-to-icon mappings are managed causes multiple >> representations >> > of > >> the same IconType to step all over one another. >> > > For example, if there two link-rel-icon entries in an html file one for >> 16x16 >> and one for 192x192, both get mapped to the FAVICON type in the DB. Say >> the >> 16x16 gets inserted first. When the 192x192 gets mapped, there is no good >> way >> > to > >> determine whether or not it should replace the 16x16 or be added >> alongside it. >> They both claim to be the FAVICON for the page. >> > > The current mapping update logic causes the 16x16 would be unmapped and >> > deleted. > > > > https://codereview.chromium.org/1119163003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I favor recording all image urls so that features that want to download (and potentially save) have the information available to them without needing to change the core favicon code. For example, you want LARGE to be 192, mobile wants a bigger large. What if desktop wants the mobile one in some place too, does that mean we need LARGER? Ugh! I have no doubt this will necessitate changes, that's what happens when new features change requirements. -Scott On Thu, May 7, 2015 at 11:37 AM, Roger McFarlane <rogerm@chromium.org> wrote: > > Yes, download on demand is what we agreed to and what we're moving towards. > > At it's simplest, for download on demand, we don't need to save the full set > of URLs, we only need to save the one most appropriate URL. This CL is > recording that information AND downloading the image, the latter part not > being what we want. But I was trying to make incremental changes to get > there, and the download-on-demand part depends on the URL info being in the > DB, so this came first. > > We don't want the FAVICON handler to record URLs, specifically because that > would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, eventually > manifest, etc). Neither do we want the TOUCH handler, because that excludes > the FAVICON. Instead we need a handler that considers all of the icon > candidates and selects the best one for a large icon. > > Saving the complete list of URLs and their sizes is still a viable approach. > It just means that the TODO I added is in the wrong place and that the URLs > need to be saved and the process aborted just before the list would have > been sorted and pruned. > > But, we would still need a way to keep those favicon records distinct from > those managed as regular FAVICONs and TOUCH*ICONS, or they end up getting > clobbered by the regular favicon download and mapping process. In short, > we'd still need something like LARGE_ICON ... or... we need a pretty large > overhaul of the history backend's handling of favicons ... or ... we need > some new place to store this info ... or ... there's something we've missed > (which is entirely possible)? > > > > On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: >> >> My understanding of what we agreed upon is download on demand. To do >> download on >> demand you need to know the set of urls images are at, and potentially >> download >> one of them. We currently aren't recording this information. That is, >> FaviconHandler selects the best image to use, downloads it, and saves the >> image >> for later use, dropping the list of potential urls. I don't think an >> additional >> FaviconHandler for LARGE is what we want. Rather we want one of the >> FaviconHandlers (FAVICON) to record all the possible urls. When the NTP >> (or >> whoever) wants a different icon it can get the set of possible urls, with >> already downloaded images and then make the decision as to whether it >> wants to >> download something knew. It may be possible to use FaviconHandler for >> this, but >> I suspect it would be easier not to. >> >> >> On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: >>> >>> Re: Download on demand. >> >> >>> This is a precursor CL to add large favicon handling to the existing >>> download >>> flow. Philippe is working on the CL to wire in download on demand once >>> the >> >> flow >>> >>> to capture the URLs of large icons is locked down (hopefully by this CL, >>> plus >>> one to only save the favicon record and skip the download). >> >> >>> The near-term goal is to have the user-visible icons-on-the-NTP >>> functionality >>> behind a flag so chrome leadership can live with the icon based NTP for >> >> awhile, >>> >>> so we can get consensus on whether this is the UI direction Chrome >>> desktop >>> should take. (Mobile already downloads large icons as you browse) >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >>> >>> File components/favicon/core/favicon_handler.cc (right): >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >>> >>> components/favicon/core/favicon_handler.cc:231: return >>> favicon_base::FAVICON | >>> favicon_base::TOUCH_ICON | >>> On 2015/05/06 19:50:42, sky wrote: >>> > Why do we need FAVICON here too? If you use FAVICON here won't that >>> > mean >>> you'll >>> > have two FaviconHandlers attempting to download FAVICON? >> >> >>> Yes and no. >> >> >>> Attempts to get the existing FAVICON and TOUCH handlers to do all the >>> work >> >> were >>> >>> unsuccessful (see https://codereview.chromium.org/1118073002/) because >>> the >>> handler seems to only capture a single representation of the icon. I.e., >>> if >> >> you >>> >>> tell the regular favicon handler to capture a large icon, it forgoes the >>> 16x16 >>> version which is needed for tab rendering. >> >> >>> If you have any suggestions on how to fix that, I'd be glad to hear them >>> and >> >> go >>> >>> back to that approach. >> >> >>> But, it's not quite as bad as it may seem... >> >> >>> The issue you've brought up occurs when the small and large icon are in >>> the >> >> same >>> >>> file (e.g., an ICO file that stores both the small and the large images >>> or a >>> single one-size fits all icon). In that case, the downside is that the >>> file >> >> will >>> >>> be downloaded twice (because the image fetcher bypasses the cache) but >>> the >>> handler will (potentially) extract different representations from it. >> >> >>> In the more general case, the small and large icon are in different >>> files, >>> possibly across different icon types. In this case, multiple downloads >>> and >>> handlers are exactly what is needed. >> >> >>> In both cases, you want the bitmaps and the mappings to be updated >> >> indepedently >>> >>> for the small and large icons, hence their treatment as different icon >>> types >> >> by >>> >>> the backend. >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >>> >>> components/favicon/core/favicon_handler.cc:570: // type, so we're not >> >> expecting >>> >>> a call to get a maximal size to downlaod. >>> On 2015/05/06 19:50:42, sky wrote: >>> > downlaod->download >> >> >>> Will do. >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >>> >>> File components/favicon_base/favicon_types.h (right): >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >>> >>> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >>> On 2015/05/06 19:50:42, sky wrote: >>> > Why do we need the new type? >> >> >>> The way the url-to-icon mappings are managed causes multiple >>> representations >> >> of >>> >>> the same IconType to step all over one another. >> >> >>> For example, if there two link-rel-icon entries in an html file one for >>> 16x16 >>> and one for 192x192, both get mapped to the FAVICON type in the DB. Say >>> the >>> 16x16 gets inserted first. When the 192x192 gets mapped, there is no good >>> way >> >> to >>> >>> determine whether or not it should replace the 16x16 or be added >>> alongside it. >>> They both claim to be the FAVICON for the page. >> >> >>> The current mapping update logic causes the 16x16 would be unmapped and >> >> deleted. >> >> >> >> https://codereview.chromium.org/1119163003/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That's fair enough, though I worry about YAGNI. I think that means we need to manage the icon mappings up front (on blink informing the icon driver of the set of icon urls) instead of in response to actually getting the bitmap data. So... On being notified of the set of icon URLs for a page URL, we need to save the new mapping, including size info. This allows fetch on demand. The favicon handlers can then trigger (or not) downloads of the resources their policies want them to download up front. On Thu, May 7, 2015 at 3:46 PM, Scott Violet <sky@chromium.org> wrote: > I favor recording all image urls so that features that want to > download (and potentially save) have the information available to them > without needing to change the core favicon code. For example, you want > LARGE to be 192, mobile wants a bigger large. What if desktop wants > the mobile one in some place too, does that mean we need LARGER? Ugh! > > I have no doubt this will necessitate changes, that's what happens > when new features change requirements. > > -Scott > > On Thu, May 7, 2015 at 11:37 AM, Roger McFarlane <rogerm@chromium.org> > wrote: > > > > Yes, download on demand is what we agreed to and what we're moving > towards. > > > > At it's simplest, for download on demand, we don't need to save the full > set > > of URLs, we only need to save the one most appropriate URL. This CL is > > recording that information AND downloading the image, the latter part not > > being what we want. But I was trying to make incremental changes to get > > there, and the download-on-demand part depends on the URL info being in > the > > DB, so this came first. > > > > We don't want the FAVICON handler to record URLs, specifically because > that > > would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, eventually > > manifest, etc). Neither do we want the TOUCH handler, because that > excludes > > the FAVICON. Instead we need a handler that considers all of the icon > > candidates and selects the best one for a large icon. > > > > Saving the complete list of URLs and their sizes is still a viable > approach. > > It just means that the TODO I added is in the wrong place and that the > URLs > > need to be saved and the process aborted just before the list would have > > been sorted and pruned. > > > > But, we would still need a way to keep those favicon records distinct > from > > those managed as regular FAVICONs and TOUCH*ICONS, or they end up getting > > clobbered by the regular favicon download and mapping process. In short, > > we'd still need something like LARGE_ICON ... or... we need a pretty > large > > overhaul of the history backend's handling of favicons ... or ... we need > > some new place to store this info ... or ... there's something we've > missed > > (which is entirely possible)? > > > > > > > > On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: > >> > >> My understanding of what we agreed upon is download on demand. To do > >> download on > >> demand you need to know the set of urls images are at, and potentially > >> download > >> one of them. We currently aren't recording this information. That is, > >> FaviconHandler selects the best image to use, downloads it, and saves > the > >> image > >> for later use, dropping the list of potential urls. I don't think an > >> additional > >> FaviconHandler for LARGE is what we want. Rather we want one of the > >> FaviconHandlers (FAVICON) to record all the possible urls. When the NTP > >> (or > >> whoever) wants a different icon it can get the set of possible urls, > with > >> already downloaded images and then make the decision as to whether it > >> wants to > >> download something knew. It may be possible to use FaviconHandler for > >> this, but > >> I suspect it would be easier not to. > >> > >> > >> On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: > >>> > >>> Re: Download on demand. > >> > >> > >>> This is a precursor CL to add large favicon handling to the existing > >>> download > >>> flow. Philippe is working on the CL to wire in download on demand once > >>> the > >> > >> flow > >>> > >>> to capture the URLs of large icons is locked down (hopefully by this > CL, > >>> plus > >>> one to only save the favicon record and skip the download). > >> > >> > >>> The near-term goal is to have the user-visible icons-on-the-NTP > >>> functionality > >>> behind a flag so chrome leadership can live with the icon based NTP for > >> > >> awhile, > >>> > >>> so we can get consensus on whether this is the UI direction Chrome > >>> desktop > >>> should take. (Mobile already downloads large icons as you browse) > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >>> > >>> File components/favicon/core/favicon_handler.cc (right): > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >>> > >>> components/favicon/core/favicon_handler.cc:231: return > >>> favicon_base::FAVICON | > >>> favicon_base::TOUCH_ICON | > >>> On 2015/05/06 19:50:42, sky wrote: > >>> > Why do we need FAVICON here too? If you use FAVICON here won't that > >>> > mean > >>> you'll > >>> > have two FaviconHandlers attempting to download FAVICON? > >> > >> > >>> Yes and no. > >> > >> > >>> Attempts to get the existing FAVICON and TOUCH handlers to do all the > >>> work > >> > >> were > >>> > >>> unsuccessful (see https://codereview.chromium.org/1118073002/) because > >>> the > >>> handler seems to only capture a single representation of the icon. > I.e., > >>> if > >> > >> you > >>> > >>> tell the regular favicon handler to capture a large icon, it forgoes > the > >>> 16x16 > >>> version which is needed for tab rendering. > >> > >> > >>> If you have any suggestions on how to fix that, I'd be glad to hear > them > >>> and > >> > >> go > >>> > >>> back to that approach. > >> > >> > >>> But, it's not quite as bad as it may seem... > >> > >> > >>> The issue you've brought up occurs when the small and large icon are in > >>> the > >> > >> same > >>> > >>> file (e.g., an ICO file that stores both the small and the large images > >>> or a > >>> single one-size fits all icon). In that case, the downside is that the > >>> file > >> > >> will > >>> > >>> be downloaded twice (because the image fetcher bypasses the cache) but > >>> the > >>> handler will (potentially) extract different representations from it. > >> > >> > >>> In the more general case, the small and large icon are in different > >>> files, > >>> possibly across different icon types. In this case, multiple downloads > >>> and > >>> handlers are exactly what is needed. > >> > >> > >>> In both cases, you want the bitmaps and the mappings to be updated > >> > >> indepedently > >>> > >>> for the small and large icons, hence their treatment as different icon > >>> types > >> > >> by > >>> > >>> the backend. > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >>> > >>> components/favicon/core/favicon_handler.cc:570: // type, so we're not > >> > >> expecting > >>> > >>> a call to get a maximal size to downlaod. > >>> On 2015/05/06 19:50:42, sky wrote: > >>> > downlaod->download > >> > >> > >>> Will do. > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >>> > >>> File components/favicon_base/favicon_types.h (right): > >> > >> > >> > >> > >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >>> > >>> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > >>> On 2015/05/06 19:50:42, sky wrote: > >>> > Why do we need the new type? > >> > >> > >>> The way the url-to-icon mappings are managed causes multiple > >>> representations > >> > >> of > >>> > >>> the same IconType to step all over one another. > >> > >> > >>> For example, if there two link-rel-icon entries in an html file one for > >>> 16x16 > >>> and one for 192x192, both get mapped to the FAVICON type in the DB. Say > >>> the > >>> 16x16 gets inserted first. When the 192x192 gets mapped, there is no > good > >>> way > >> > >> to > >>> > >>> determine whether or not it should replace the 16x16 or be added > >>> alongside it. > >>> They both claim to be the FAVICON for the page. > >> > >> > >>> The current mapping update logic causes the 16x16 would be unmapped and > >> > >> deleted. > >> > >> > >> > >> https://codereview.chromium.org/1119163003/ > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, May 7, 2015 at 1:58 PM, Roger McFarlane <rogerm@chromium.org> wrote: > That's fair enough, though I worry about YAGNI. > > I think that means we need to manage the icon mappings up front (on blink > informing the icon driver of the set of icon urls) instead of in response to > actually getting the bitmap data. > > So... > > On being notified of the set of icon URLs for a page URL, we need to save > the new mapping, including size info. This allows fetch on demand. The > favicon handlers can then trigger (or not) downloads of the resources their > policies want them to download up front. I agree. The only thing I'm not too happy with is how to deal with urls with no size information, or urls with the wrong size. -Scott > > On Thu, May 7, 2015 at 3:46 PM, Scott Violet <sky@chromium.org> wrote: >> >> I favor recording all image urls so that features that want to >> download (and potentially save) have the information available to them >> without needing to change the core favicon code. For example, you want >> LARGE to be 192, mobile wants a bigger large. What if desktop wants >> the mobile one in some place too, does that mean we need LARGER? Ugh! >> >> I have no doubt this will necessitate changes, that's what happens >> when new features change requirements. >> >> -Scott >> >> On Thu, May 7, 2015 at 11:37 AM, Roger McFarlane <rogerm@chromium.org> >> wrote: >> > >> > Yes, download on demand is what we agreed to and what we're moving >> > towards. >> > >> > At it's simplest, for download on demand, we don't need to save the full >> > set >> > of URLs, we only need to save the one most appropriate URL. This CL is >> > recording that information AND downloading the image, the latter part >> > not >> > being what we want. But I was trying to make incremental changes to get >> > there, and the download-on-demand part depends on the URL info being in >> > the >> > DB, so this came first. >> > >> > We don't want the FAVICON handler to record URLs, specifically because >> > that >> > would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, eventually >> > manifest, etc). Neither do we want the TOUCH handler, because that >> > excludes >> > the FAVICON. Instead we need a handler that considers all of the icon >> > candidates and selects the best one for a large icon. >> > >> > Saving the complete list of URLs and their sizes is still a viable >> > approach. >> > It just means that the TODO I added is in the wrong place and that the >> > URLs >> > need to be saved and the process aborted just before the list would have >> > been sorted and pruned. >> > >> > But, we would still need a way to keep those favicon records distinct >> > from >> > those managed as regular FAVICONs and TOUCH*ICONS, or they end up >> > getting >> > clobbered by the regular favicon download and mapping process. In short, >> > we'd still need something like LARGE_ICON ... or... we need a pretty >> > large >> > overhaul of the history backend's handling of favicons ... or ... we >> > need >> > some new place to store this info ... or ... there's something we've >> > missed >> > (which is entirely possible)? >> > >> > >> > >> > On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: >> >> >> >> My understanding of what we agreed upon is download on demand. To do >> >> download on >> >> demand you need to know the set of urls images are at, and potentially >> >> download >> >> one of them. We currently aren't recording this information. That is, >> >> FaviconHandler selects the best image to use, downloads it, and saves >> >> the >> >> image >> >> for later use, dropping the list of potential urls. I don't think an >> >> additional >> >> FaviconHandler for LARGE is what we want. Rather we want one of the >> >> FaviconHandlers (FAVICON) to record all the possible urls. When the NTP >> >> (or >> >> whoever) wants a different icon it can get the set of possible urls, >> >> with >> >> already downloaded images and then make the decision as to whether it >> >> wants to >> >> download something knew. It may be possible to use FaviconHandler for >> >> this, but >> >> I suspect it would be easier not to. >> >> >> >> >> >> On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: >> >>> >> >>> Re: Download on demand. >> >> >> >> >> >>> This is a precursor CL to add large favicon handling to the existing >> >>> download >> >>> flow. Philippe is working on the CL to wire in download on demand once >> >>> the >> >> >> >> flow >> >>> >> >>> to capture the URLs of large icons is locked down (hopefully by this >> >>> CL, >> >>> plus >> >>> one to only save the favicon record and skip the download). >> >> >> >> >> >>> The near-term goal is to have the user-visible icons-on-the-NTP >> >>> functionality >> >>> behind a flag so chrome leadership can live with the icon based NTP >> >>> for >> >> >> >> awhile, >> >>> >> >>> so we can get consensus on whether this is the UI direction Chrome >> >>> desktop >> >>> should take. (Mobile already downloads large icons as you browse) >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >>> >> >>> File components/favicon/core/favicon_handler.cc (right): >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >>> >> >>> components/favicon/core/favicon_handler.cc:231: return >> >>> favicon_base::FAVICON | >> >>> favicon_base::TOUCH_ICON | >> >>> On 2015/05/06 19:50:42, sky wrote: >> >>> > Why do we need FAVICON here too? If you use FAVICON here won't that >> >>> > mean >> >>> you'll >> >>> > have two FaviconHandlers attempting to download FAVICON? >> >> >> >> >> >>> Yes and no. >> >> >> >> >> >>> Attempts to get the existing FAVICON and TOUCH handlers to do all the >> >>> work >> >> >> >> were >> >>> >> >>> unsuccessful (see https://codereview.chromium.org/1118073002/) because >> >>> the >> >>> handler seems to only capture a single representation of the icon. >> >>> I.e., >> >>> if >> >> >> >> you >> >>> >> >>> tell the regular favicon handler to capture a large icon, it forgoes >> >>> the >> >>> 16x16 >> >>> version which is needed for tab rendering. >> >> >> >> >> >>> If you have any suggestions on how to fix that, I'd be glad to hear >> >>> them >> >>> and >> >> >> >> go >> >>> >> >>> back to that approach. >> >> >> >> >> >>> But, it's not quite as bad as it may seem... >> >> >> >> >> >>> The issue you've brought up occurs when the small and large icon are >> >>> in >> >>> the >> >> >> >> same >> >>> >> >>> file (e.g., an ICO file that stores both the small and the large >> >>> images >> >>> or a >> >>> single one-size fits all icon). In that case, the downside is that the >> >>> file >> >> >> >> will >> >>> >> >>> be downloaded twice (because the image fetcher bypasses the cache) but >> >>> the >> >>> handler will (potentially) extract different representations from it. >> >> >> >> >> >>> In the more general case, the small and large icon are in different >> >>> files, >> >>> possibly across different icon types. In this case, multiple downloads >> >>> and >> >>> handlers are exactly what is needed. >> >> >> >> >> >>> In both cases, you want the bitmaps and the mappings to be updated >> >> >> >> indepedently >> >>> >> >>> for the small and large icons, hence their treatment as different icon >> >>> types >> >> >> >> by >> >>> >> >>> the backend. >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >>> >> >>> components/favicon/core/favicon_handler.cc:570: // type, so we're not >> >> >> >> expecting >> >>> >> >>> a call to get a maximal size to downlaod. >> >>> On 2015/05/06 19:50:42, sky wrote: >> >>> > downlaod->download >> >> >> >> >> >>> Will do. >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >>> >> >>> File components/favicon_base/favicon_types.h (right): >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >>> >> >>> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >> >>> On 2015/05/06 19:50:42, sky wrote: >> >>> > Why do we need the new type? >> >> >> >> >> >>> The way the url-to-icon mappings are managed causes multiple >> >>> representations >> >> >> >> of >> >>> >> >>> the same IconType to step all over one another. >> >> >> >> >> >>> For example, if there two link-rel-icon entries in an html file one >> >>> for >> >>> 16x16 >> >>> and one for 192x192, both get mapped to the FAVICON type in the DB. >> >>> Say >> >>> the >> >>> 16x16 gets inserted first. When the 192x192 gets mapped, there is no >> >>> good >> >>> way >> >> >> >> to >> >>> >> >>> determine whether or not it should replace the 16x16 or be added >> >>> alongside it. >> >>> They both claim to be the FAVICON for the page. >> >> >> >> >> >>> The current mapping update logic causes the 16x16 would be unmapped >> >>> and >> >> >> >> deleted. >> >> >> >> >> >> >> >> https://codereview.chromium.org/1119163003/ >> > >> > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So, a concern we've got in building this feature out is that there isn't consensus that icons are the direction we want to go in on desktop. We're trying to land the ability to show icon based NTP behind a flag so chrome product leadership can live with it behind a flag to get a feel for it and for the type of icon coverage we can expect. There's still a distinct possibility that the ultimate decision will be to not go in this direction. For that reason, I'm hesitant to make significant changes to the way we're managing icons and the icon mappings at this point. It's a somewhat complex and fairly invasive change that, if we don't end up moving towards icons on desktop, doesn't bring value in and of itself (in the opinion of the myself and the team, since there are no other concrete plans to use these features other than the proposed NTP changes). On Thu, May 7, 2015 at 7:16 PM, Scott Violet <sky@chromium.org> wrote: > On Thu, May 7, 2015 at 1:58 PM, Roger McFarlane <rogerm@chromium.org> > wrote: > > That's fair enough, though I worry about YAGNI. > > > > I think that means we need to manage the icon mappings up front (on blink > > informing the icon driver of the set of icon urls) instead of in > response to > > actually getting the bitmap data. > > > > So... > > > > On being notified of the set of icon URLs for a page URL, we need to save > > the new mapping, including size info. This allows fetch on demand. The > > favicon handlers can then trigger (or not) downloads of the resources > their > > policies want them to download up front. > > I agree. The only thing I'm not too happy with is how to deal with > urls with no size information, or urls with the wrong size. > > -Scott > > > > > On Thu, May 7, 2015 at 3:46 PM, Scott Violet <sky@chromium.org> wrote: > >> > >> I favor recording all image urls so that features that want to > >> download (and potentially save) have the information available to them > >> without needing to change the core favicon code. For example, you want > >> LARGE to be 192, mobile wants a bigger large. What if desktop wants > >> the mobile one in some place too, does that mean we need LARGER? Ugh! > >> > >> I have no doubt this will necessitate changes, that's what happens > >> when new features change requirements. > >> > >> -Scott > >> > >> On Thu, May 7, 2015 at 11:37 AM, Roger McFarlane <rogerm@chromium.org> > >> wrote: > >> > > >> > Yes, download on demand is what we agreed to and what we're moving > >> > towards. > >> > > >> > At it's simplest, for download on demand, we don't need to save the > full > >> > set > >> > of URLs, we only need to save the one most appropriate URL. This CL is > >> > recording that information AND downloading the image, the latter part > >> > not > >> > being what we want. But I was trying to make incremental changes to > get > >> > there, and the download-on-demand part depends on the URL info being > in > >> > the > >> > DB, so this came first. > >> > > >> > We don't want the FAVICON handler to record URLs, specifically because > >> > that > >> > would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, > eventually > >> > manifest, etc). Neither do we want the TOUCH handler, because that > >> > excludes > >> > the FAVICON. Instead we need a handler that considers all of the icon > >> > candidates and selects the best one for a large icon. > >> > > >> > Saving the complete list of URLs and their sizes is still a viable > >> > approach. > >> > It just means that the TODO I added is in the wrong place and that the > >> > URLs > >> > need to be saved and the process aborted just before the list would > have > >> > been sorted and pruned. > >> > > >> > But, we would still need a way to keep those favicon records distinct > >> > from > >> > those managed as regular FAVICONs and TOUCH*ICONS, or they end up > >> > getting > >> > clobbered by the regular favicon download and mapping process. In > short, > >> > we'd still need something like LARGE_ICON ... or... we need a pretty > >> > large > >> > overhaul of the history backend's handling of favicons ... or ... we > >> > need > >> > some new place to store this info ... or ... there's something we've > >> > missed > >> > (which is entirely possible)? > >> > > >> > > >> > > >> > On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: > >> >> > >> >> My understanding of what we agreed upon is download on demand. To do > >> >> download on > >> >> demand you need to know the set of urls images are at, and > potentially > >> >> download > >> >> one of them. We currently aren't recording this information. That is, > >> >> FaviconHandler selects the best image to use, downloads it, and saves > >> >> the > >> >> image > >> >> for later use, dropping the list of potential urls. I don't think an > >> >> additional > >> >> FaviconHandler for LARGE is what we want. Rather we want one of the > >> >> FaviconHandlers (FAVICON) to record all the possible urls. When the > NTP > >> >> (or > >> >> whoever) wants a different icon it can get the set of possible urls, > >> >> with > >> >> already downloaded images and then make the decision as to whether it > >> >> wants to > >> >> download something knew. It may be possible to use FaviconHandler for > >> >> this, but > >> >> I suspect it would be easier not to. > >> >> > >> >> > >> >> On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: > >> >>> > >> >>> Re: Download on demand. > >> >> > >> >> > >> >>> This is a precursor CL to add large favicon handling to the existing > >> >>> download > >> >>> flow. Philippe is working on the CL to wire in download on demand > once > >> >>> the > >> >> > >> >> flow > >> >>> > >> >>> to capture the URLs of large icons is locked down (hopefully by this > >> >>> CL, > >> >>> plus > >> >>> one to only save the favicon record and skip the download). > >> >> > >> >> > >> >>> The near-term goal is to have the user-visible icons-on-the-NTP > >> >>> functionality > >> >>> behind a flag so chrome leadership can live with the icon based NTP > >> >>> for > >> >> > >> >> awhile, > >> >>> > >> >>> so we can get consensus on whether this is the UI direction Chrome > >> >>> desktop > >> >>> should take. (Mobile already downloads large icons as you browse) > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> >>> > >> >>> File components/favicon/core/favicon_handler.cc (right): > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> >>> > >> >>> components/favicon/core/favicon_handler.cc:231: return > >> >>> favicon_base::FAVICON | > >> >>> favicon_base::TOUCH_ICON | > >> >>> On 2015/05/06 19:50:42, sky wrote: > >> >>> > Why do we need FAVICON here too? If you use FAVICON here won't > that > >> >>> > mean > >> >>> you'll > >> >>> > have two FaviconHandlers attempting to download FAVICON? > >> >> > >> >> > >> >>> Yes and no. > >> >> > >> >> > >> >>> Attempts to get the existing FAVICON and TOUCH handlers to do all > the > >> >>> work > >> >> > >> >> were > >> >>> > >> >>> unsuccessful (see https://codereview.chromium.org/1118073002/) > because > >> >>> the > >> >>> handler seems to only capture a single representation of the icon. > >> >>> I.e., > >> >>> if > >> >> > >> >> you > >> >>> > >> >>> tell the regular favicon handler to capture a large icon, it forgoes > >> >>> the > >> >>> 16x16 > >> >>> version which is needed for tab rendering. > >> >> > >> >> > >> >>> If you have any suggestions on how to fix that, I'd be glad to hear > >> >>> them > >> >>> and > >> >> > >> >> go > >> >>> > >> >>> back to that approach. > >> >> > >> >> > >> >>> But, it's not quite as bad as it may seem... > >> >> > >> >> > >> >>> The issue you've brought up occurs when the small and large icon are > >> >>> in > >> >>> the > >> >> > >> >> same > >> >>> > >> >>> file (e.g., an ICO file that stores both the small and the large > >> >>> images > >> >>> or a > >> >>> single one-size fits all icon). In that case, the downside is that > the > >> >>> file > >> >> > >> >> will > >> >>> > >> >>> be downloaded twice (because the image fetcher bypasses the cache) > but > >> >>> the > >> >>> handler will (potentially) extract different representations from > it. > >> >> > >> >> > >> >>> In the more general case, the small and large icon are in different > >> >>> files, > >> >>> possibly across different icon types. In this case, multiple > downloads > >> >>> and > >> >>> handlers are exactly what is needed. > >> >> > >> >> > >> >>> In both cases, you want the bitmaps and the mappings to be updated > >> >> > >> >> indepedently > >> >>> > >> >>> for the small and large icons, hence their treatment as different > icon > >> >>> types > >> >> > >> >> by > >> >>> > >> >>> the backend. > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> >>> > >> >>> components/favicon/core/favicon_handler.cc:570: // type, so we're > not > >> >> > >> >> expecting > >> >>> > >> >>> a call to get a maximal size to downlaod. > >> >>> On 2015/05/06 19:50:42, sky wrote: > >> >>> > downlaod->download > >> >> > >> >> > >> >>> Will do. > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> >>> > >> >>> File components/favicon_base/favicon_types.h (right): > >> >> > >> >> > >> >> > >> >> > >> >> > >> >> > https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... > >> >>> > >> >>> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 > >> >>> On 2015/05/06 19:50:42, sky wrote: > >> >>> > Why do we need the new type? > >> >> > >> >> > >> >>> The way the url-to-icon mappings are managed causes multiple > >> >>> representations > >> >> > >> >> of > >> >>> > >> >>> the same IconType to step all over one another. > >> >> > >> >> > >> >>> For example, if there two link-rel-icon entries in an html file one > >> >>> for > >> >>> 16x16 > >> >>> and one for 192x192, both get mapped to the FAVICON type in the DB. > >> >>> Say > >> >>> the > >> >>> 16x16 gets inserted first. When the 192x192 gets mapped, there is no > >> >>> good > >> >>> way > >> >> > >> >> to > >> >>> > >> >>> determine whether or not it should replace the 16x16 or be added > >> >>> alongside it. > >> >>> They both claim to be the FAVICON for the page. > >> >> > >> >> > >> >>> The current mapping update logic causes the 16x16 would be unmapped > >> >>> and > >> >> > >> >> deleted. > >> >> > >> >> > >> >> > >> >> https://codereview.chromium.org/1119163003/ > >> > > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I prefer seeing things done the right way. If you don't, and you land this, you'll have little reason to clean things up later on. -Scott On Fri, May 8, 2015 at 12:17 PM, Roger McFarlane <rogerm@chromium.org> wrote: > So, a concern we've got in building this feature out is that there isn't > consensus that icons are the direction we want to go in on desktop. We're > trying to land the ability to show icon based NTP behind a flag so chrome > product leadership can live with it behind a flag to get a feel for it and > for the type of icon coverage we can expect. > > There's still a distinct possibility that the ultimate decision will be to > not go in this direction. > > For that reason, I'm hesitant to make significant changes to the way we're > managing icons and the icon mappings at this point. It's a somewhat complex > and fairly invasive change that, if we don't end up moving towards icons on > desktop, doesn't bring value in and of itself (in the opinion of the myself > and the team, since there are no other concrete plans to use these features > other than the proposed NTP changes). > > > On Thu, May 7, 2015 at 7:16 PM, Scott Violet <sky@chromium.org> wrote: >> >> On Thu, May 7, 2015 at 1:58 PM, Roger McFarlane <rogerm@chromium.org> >> wrote: >> > That's fair enough, though I worry about YAGNI. >> > >> > I think that means we need to manage the icon mappings up front (on >> > blink >> > informing the icon driver of the set of icon urls) instead of in >> > response to >> > actually getting the bitmap data. >> > >> > So... >> > >> > On being notified of the set of icon URLs for a page URL, we need to >> > save >> > the new mapping, including size info. This allows fetch on demand. The >> > favicon handlers can then trigger (or not) downloads of the resources >> > their >> > policies want them to download up front. >> >> I agree. The only thing I'm not too happy with is how to deal with >> urls with no size information, or urls with the wrong size. >> >> -Scott >> >> > >> > On Thu, May 7, 2015 at 3:46 PM, Scott Violet <sky@chromium.org> wrote: >> >> >> >> I favor recording all image urls so that features that want to >> >> download (and potentially save) have the information available to them >> >> without needing to change the core favicon code. For example, you want >> >> LARGE to be 192, mobile wants a bigger large. What if desktop wants >> >> the mobile one in some place too, does that mean we need LARGER? Ugh! >> >> >> >> I have no doubt this will necessitate changes, that's what happens >> >> when new features change requirements. >> >> >> >> -Scott >> >> >> >> On Thu, May 7, 2015 at 11:37 AM, Roger McFarlane <rogerm@chromium.org> >> >> wrote: >> >> > >> >> > Yes, download on demand is what we agreed to and what we're moving >> >> > towards. >> >> > >> >> > At it's simplest, for download on demand, we don't need to save the >> >> > full >> >> > set >> >> > of URLs, we only need to save the one most appropriate URL. This CL >> >> > is >> >> > recording that information AND downloading the image, the latter part >> >> > not >> >> > being what we want. But I was trying to make incremental changes to >> >> > get >> >> > there, and the download-on-demand part depends on the URL info being >> >> > in >> >> > the >> >> > DB, so this came first. >> >> > >> >> > We don't want the FAVICON handler to record URLs, specifically >> >> > because >> >> > that >> >> > would exclude the other icon types (TOUCH, TOUCH_PRECOMPOSED, >> >> > eventually >> >> > manifest, etc). Neither do we want the TOUCH handler, because that >> >> > excludes >> >> > the FAVICON. Instead we need a handler that considers all of the icon >> >> > candidates and selects the best one for a large icon. >> >> > >> >> > Saving the complete list of URLs and their sizes is still a viable >> >> > approach. >> >> > It just means that the TODO I added is in the wrong place and that >> >> > the >> >> > URLs >> >> > need to be saved and the process aborted just before the list would >> >> > have >> >> > been sorted and pruned. >> >> > >> >> > But, we would still need a way to keep those favicon records distinct >> >> > from >> >> > those managed as regular FAVICONs and TOUCH*ICONS, or they end up >> >> > getting >> >> > clobbered by the regular favicon download and mapping process. In >> >> > short, >> >> > we'd still need something like LARGE_ICON ... or... we need a pretty >> >> > large >> >> > overhaul of the history backend's handling of favicons ... or ... we >> >> > need >> >> > some new place to store this info ... or ... there's something we've >> >> > missed >> >> > (which is entirely possible)? >> >> > >> >> > >> >> > >> >> > On Thu, May 7, 2015 at 1:37 PM, <sky@chromium.org> wrote: >> >> >> >> >> >> My understanding of what we agreed upon is download on demand. To do >> >> >> download on >> >> >> demand you need to know the set of urls images are at, and >> >> >> potentially >> >> >> download >> >> >> one of them. We currently aren't recording this information. That >> >> >> is, >> >> >> FaviconHandler selects the best image to use, downloads it, and >> >> >> saves >> >> >> the >> >> >> image >> >> >> for later use, dropping the list of potential urls. I don't think an >> >> >> additional >> >> >> FaviconHandler for LARGE is what we want. Rather we want one of the >> >> >> FaviconHandlers (FAVICON) to record all the possible urls. When the >> >> >> NTP >> >> >> (or >> >> >> whoever) wants a different icon it can get the set of possible urls, >> >> >> with >> >> >> already downloaded images and then make the decision as to whether >> >> >> it >> >> >> wants to >> >> >> download something knew. It may be possible to use FaviconHandler >> >> >> for >> >> >> this, but >> >> >> I suspect it would be easier not to. >> >> >> >> >> >> >> >> >> On 2015/05/06 21:22:51, Roger McFarlane (Chromium) wrote: >> >> >>> >> >> >>> Re: Download on demand. >> >> >> >> >> >> >> >> >>> This is a precursor CL to add large favicon handling to the >> >> >>> existing >> >> >>> download >> >> >>> flow. Philippe is working on the CL to wire in download on demand >> >> >>> once >> >> >>> the >> >> >> >> >> >> flow >> >> >>> >> >> >>> to capture the URLs of large icons is locked down (hopefully by >> >> >>> this >> >> >>> CL, >> >> >>> plus >> >> >>> one to only save the favicon record and skip the download). >> >> >> >> >> >> >> >> >>> The near-term goal is to have the user-visible icons-on-the-NTP >> >> >>> functionality >> >> >>> behind a flag so chrome leadership can live with the icon based NTP >> >> >>> for >> >> >> >> >> >> awhile, >> >> >>> >> >> >>> so we can get consensus on whether this is the UI direction Chrome >> >> >>> desktop >> >> >>> should take. (Mobile already downloads large icons as you browse) >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >>> >> >> >>> File components/favicon/core/favicon_handler.cc (right): >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >>> >> >> >>> components/favicon/core/favicon_handler.cc:231: return >> >> >>> favicon_base::FAVICON | >> >> >>> favicon_base::TOUCH_ICON | >> >> >>> On 2015/05/06 19:50:42, sky wrote: >> >> >>> > Why do we need FAVICON here too? If you use FAVICON here won't >> >> >>> > that >> >> >>> > mean >> >> >>> you'll >> >> >>> > have two FaviconHandlers attempting to download FAVICON? >> >> >> >> >> >> >> >> >>> Yes and no. >> >> >> >> >> >> >> >> >>> Attempts to get the existing FAVICON and TOUCH handlers to do all >> >> >>> the >> >> >>> work >> >> >> >> >> >> were >> >> >>> >> >> >>> unsuccessful (see https://codereview.chromium.org/1118073002/) >> >> >>> because >> >> >>> the >> >> >>> handler seems to only capture a single representation of the icon. >> >> >>> I.e., >> >> >>> if >> >> >> >> >> >> you >> >> >>> >> >> >>> tell the regular favicon handler to capture a large icon, it >> >> >>> forgoes >> >> >>> the >> >> >>> 16x16 >> >> >>> version which is needed for tab rendering. >> >> >> >> >> >> >> >> >>> If you have any suggestions on how to fix that, I'd be glad to hear >> >> >>> them >> >> >>> and >> >> >> >> >> >> go >> >> >>> >> >> >>> back to that approach. >> >> >> >> >> >> >> >> >>> But, it's not quite as bad as it may seem... >> >> >> >> >> >> >> >> >>> The issue you've brought up occurs when the small and large icon >> >> >>> are >> >> >>> in >> >> >>> the >> >> >> >> >> >> same >> >> >>> >> >> >>> file (e.g., an ICO file that stores both the small and the large >> >> >>> images >> >> >>> or a >> >> >>> single one-size fits all icon). In that case, the downside is that >> >> >>> the >> >> >>> file >> >> >> >> >> >> will >> >> >>> >> >> >>> be downloaded twice (because the image fetcher bypasses the cache) >> >> >>> but >> >> >>> the >> >> >>> handler will (potentially) extract different representations from >> >> >>> it. >> >> >> >> >> >> >> >> >>> In the more general case, the small and large icon are in different >> >> >>> files, >> >> >>> possibly across different icon types. In this case, multiple >> >> >>> downloads >> >> >>> and >> >> >>> handlers are exactly what is needed. >> >> >> >> >> >> >> >> >>> In both cases, you want the bitmaps and the mappings to be updated >> >> >> >> >> >> indepedently >> >> >>> >> >> >>> for the small and large icons, hence their treatment as different >> >> >>> icon >> >> >>> types >> >> >> >> >> >> by >> >> >>> >> >> >>> the backend. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >>> >> >> >>> components/favicon/core/favicon_handler.cc:570: // type, so we're >> >> >>> not >> >> >> >> >> >> expecting >> >> >>> >> >> >>> a call to get a maximal size to downlaod. >> >> >>> On 2015/05/06 19:50:42, sky wrote: >> >> >>> > downlaod->download >> >> >> >> >> >> >> >> >>> Will do. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >>> >> >> >>> File components/favicon_base/favicon_types.h (right): >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://chromiumcodereview.appspot.com/1119163003/diff/120001/components/favi... >> >> >>> >> >> >>> components/favicon_base/favicon_types.h:44: LARGE_ICON = 1 << 3 >> >> >>> On 2015/05/06 19:50:42, sky wrote: >> >> >>> > Why do we need the new type? >> >> >> >> >> >> >> >> >>> The way the url-to-icon mappings are managed causes multiple >> >> >>> representations >> >> >> >> >> >> of >> >> >>> >> >> >>> the same IconType to step all over one another. >> >> >> >> >> >> >> >> >>> For example, if there two link-rel-icon entries in an html file one >> >> >>> for >> >> >>> 16x16 >> >> >>> and one for 192x192, both get mapped to the FAVICON type in the DB. >> >> >>> Say >> >> >>> the >> >> >>> 16x16 gets inserted first. When the 192x192 gets mapped, there is >> >> >>> no >> >> >>> good >> >> >>> way >> >> >> >> >> >> to >> >> >>> >> >> >>> determine whether or not it should replace the 16x16 or be added >> >> >>> alongside it. >> >> >>> They both claim to be the FAVICON for the page. >> >> >> >> >> >> >> >> >>> The current mapping update logic causes the 16x16 would be unmapped >> >> >>> and >> >> >> >> >> >> deleted. >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.chromium.org/1119163003/ >> >> > >> >> > >> > >> > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just a minor note from the involved PM. After weighing the advantages of having this code be in for easy feature evaluation against my slight distaste at the implementation method, on balance I'd still be happy if this could be approved. We want to submit this code for now so that we could at least potentially have a working demo of fetch-on-demand icons hidden behind a flag for Chrome UX and leads to try out. That way, when we revisit the idea of having icons on the desktop New Tab Page in the future, it won't require any engineering work to reevaluate the current state of icon coverage. If we can't land this, we'd probably have to get Roger to go over the code again, reestablish context with reviewers, go through another week or two of reviews, and then resubmit. I took a look at the CL itself: I agree that this isn't the cleanest solution, but I don't think the addition of a LARGE_ICON type fundamentally goes against the mental model that the clients of favicon_service and favicon_types would have. Instead of each type being really-close-to-the-metal base types, each type becomes just slightly more abstract. While this is marginally more complex internally, it really doesn't matter as much for external users of the service. Overall, I think this is still worth getting approved but I'll defer to you guys for the final decision.
I still stand by my comments. I prefer this done the way we want the code structured long term. -Scott On Mon, May 11, 2015 at 3:39 PM, <harryyu@chromium.org> wrote: > Just a minor note from the involved PM. After weighing the advantages of > having > this code be in for easy feature evaluation against my slight distaste at > the > implementation method, on balance I'd still be happy if this could be > approved. > > We want to submit this code for now so that we could at least potentially > have a > working demo of fetch-on-demand icons hidden behind a flag for Chrome UX and > leads to try out. That way, when we revisit the idea of having icons on the > desktop New Tab Page in the future, it won't require any engineering work to > reevaluate the current state of icon coverage. If we can't land this, we'd > probably have to get Roger to go over the code again, reestablish context > with > reviewers, go through another week or two of reviews, and then resubmit. > > I took a look at the CL itself: I agree that this isn't the cleanest > solution, > but I don't think the addition of a LARGE_ICON type fundamentally goes > against > the mental model that the clients of favicon_service and favicon_types would > have. Instead of each type being really-close-to-the-metal base types, each > type > becomes just slightly more abstract. While this is marginally more complex > internally, it really doesn't matter as much for external users of the > service. > > Overall, I think this is still worth getting approved but I'll defer to you > guys > for the final decision. > > https://codereview.chromium.org/1119163003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Roger, what's the status?
On 2015/05/21 20:28:17, pkotwicz wrote: > Roger, what's the status? This appears to be dead for now. The context was that we wanted to gather evidence to prove/disprove the suitability and coverage of an icon based UI for the desktop NTP (and by implication, I suppose, other desktop UI surfaces). We had a timebox in which to explore this, after which we were asked by product leadership to move on to more pressing priorities. We might revisit this in a couple of quarters. :( It doesn't make sense to re-engineer the favicon service with generic support for various icon sizes when there's otherwise no requirement for those capabilities.
Removing myself from reviewers to clean things up.
beaudoin@chromium.org changed reviewers: - beaudoin@chromium.org
pkotwicz@chromium.org changed reviewers: - pkotwicz@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
