Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1084473002:
Simplify FaviconHandler (Closed)
DescriptionSimplify FaviconHandler
This CL:
- Removes the |is_active_favicon| parameter from NotifyFaviconAvailable() since
its value can be computed from member variables
- Refactors FaviconHandler::OnDidDownloadFavicon() to exit early if the URL for
which the favicon was downloaded is unexpected
- Changes FaviconHandler::FetchFavicon() to reset as much state as possible.
- Changes the signature of FaviconHandler::ScheduleDownload() to void because
its return value is never used
- Removes unused member variable
FaviconHandler::waiting_for_favicon_service_data_
BUG=None
TEST=None
Committed: https://crrev.com/e002b6edc1407681c20a84f5b7cf3b249791a0bc
Cr-Commit-Position: refs/heads/master@{#326042}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : #
Messages
Total messages: 28 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:30003) has been deleted
pkotwicz@chromium.org changed reviewers: + rogerm@chromium.org
rogerm@ PTAL This CL makes things slightly better There are several other things which can be improved / refactored. I am going to resist the temptation for now. I would rather get the bugs with FaviconHandler fixed this milestone than getting the refactors done this milestone https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) I removed the UrlMatches() check because we clear |download_requests_| whenver |url_| changes. https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:427: if (!bitmaps.empty()) { I removed this check because it is redundant https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:396: if (PageChangedSinceFaviconWasRequested() || I think that the original code has the PageChangedSinceFaviconWasRequested() check where it does because OnDidDownloadFavicon() is supposed to work even if download_request.url is for an old page (Stuff gets saved to the database even if a user browses through sites really fast) However, the original code does not do this correctly, because it resets |image_urls_| even if download_request.url does not match (And having the check here makes things simpler) https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:428: if (!image_skia.isNull()) { I still need this check because |image_skia| might be NULL if one of |bitmaps| is empty.
LGTM https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:326: bool is_active_favicon = How would this interact with download requests initiating from a different page (like the NTP, for a pointed example)? For example, the LargeIconSource would be referenced from the NTP to trigger on-demand downloads of large icon bitmaps. Would this run afoul of the handler trying to infer if the icon was for the current page?
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
sky@ for OWNERS https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:326: bool is_active_favicon = Downloads initiated from a different part of Chrome should have no impact. MetroPinTabHelper::TogglePinnedToStartScreen() already does this for example. Care will be needed when LargeIconSource writes data to the Favicons database so that it does not clobber data inserted by FaviconHandler.
https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) On 2015/04/13 02:55:30, pkotwicz wrote: > I removed the UrlMatches() check because we clear |download_requests_| whenver > |url_| changes. It's not clear to me this is right. From the comment in PageChangedSinceFaviconWasRequested: "If the URL has changed out from under us (as will happen with redirects)..." If a redirect doesn't change url_ then we need to continue calling PageChangedSinceFaviconWasRequested.
https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) I am still checking PageChangedSinceFaviconWasRequested(). I have just moved the check to the caller: OnDidDownloadFavicon()
https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) On 2015/04/13 20:06:46, pkotwicz wrote: > I am still checking PageChangedSinceFaviconWasRequested(). I have just moved the > check to the caller: OnDidDownloadFavicon() Is there a particular reason to push the check like you did? I prefer it here with the early out, but maybe I'm not understanding why you want to call through to the driver.
https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) I made this change because I had a hard time understanding why PageChangedSinceFaviconWasRequested() was being called where it was (e.g why is PageChangedSinceFaviconWasRequested() not called at the beginning of OnDidDownloadFavicon()?). Calling PageChangedSinceFaviconWasRequested() at the start of OnDidDownloadFavicon() is easier to understand and is equivalent in terms of behavior. I don't mind adding a PageChangedSinceFaviconWasRequested() check at the beginning of SetFavicon() if it feels safer to have it there too
https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) On 2015/04/13 21:19:18, pkotwicz wrote: > I made this change because I had a hard time understanding why > PageChangedSinceFaviconWasRequested() was being called where it was (e.g why is > PageChangedSinceFaviconWasRequested() not called at the beginning of > OnDidDownloadFavicon()?). > > Calling PageChangedSinceFaviconWasRequested() at the start of > OnDidDownloadFavicon() is easier to understand and is equivalent in terms of > behavior. > > I don't mind adding a PageChangedSinceFaviconWasRequested() check at the > beginning of SetFavicon() if it feels safer to have it there too The idea is that we still persist a favicon we downloaded, even if it isn't for the current page. By that I mean if we start a download for url1, the user navigates to url2, then we get the icon for url1 we still persist url1's icon if necessary. I suspect this got broke a long time ago...
sky@ can you please take another look? Sorry for taking so long to get back to you https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested()) Yes, this got broken in 2012 (https://chromiumcodereview.appspot.com/9810023 in particular) I think that the feature: "Persisting the favicon which was downloaded even if it was not for the current page" is as old as Chrome itself. (It is already present in https://codereview.chromium.org/6672065/) However: - The feature made more sense back when we did not consider pages with multiple icons - I am unsure whether the value of having this feature was ever assessed. Currently: - We incorrectly clear image_urls_ on line 446 in the old code even if PageChangedSinceFaviconWasRequested() is false. - We might end up saving data for a suboptimal icon URL if the page has several icon URLs. We interpret the page URL changing as "save the best one we got so far". This will produce interesting results if we are replacing an expired favicon. In terms of resolutions: - I am ok with fixing the feature if it was intended. - I am ok with fixing the feature and adding UMA stats to identify how often this feature is used - I am of course ok with removing the feature because it helps reduce code complexity
Given that this feature has been broken for a while. Lets nuke it. On Sun, Apr 19, 2015 at 4:59 PM, <pkotwicz@chromium.org> wrote: > sky@ can you please take another look? Sorry for taking so long to get back > to > you > > > https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... > File components/favicon/core/favicon_handler.cc (left): > > https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, > url_) || PageChangedSinceFaviconWasRequested()) > Yes, this got broken in 2012 > (https://chromiumcodereview.appspot.com/9810023 in particular) > I think that the feature: "Persisting the favicon which was downloaded > even if it was not for the current page" is as old as Chrome itself. (It > is already present in https://codereview.chromium.org/6672065/) > However: > - The feature made more sense back when we did not consider pages with > multiple icons > - I am unsure whether the value of having this feature was ever > assessed. > > Currently: > - We incorrectly clear image_urls_ on line 446 in the old code even if > PageChangedSinceFaviconWasRequested() is false. > - We might end up saving data for a suboptimal icon URL if the page has > several icon URLs. We interpret the page URL changing as "save the best > one we got so far". This will produce interesting results if we are > replacing an expired favicon. > > In terms of resolutions: > - I am ok with fixing the feature if it was intended. > - I am ok with fixing the feature and adding UMA stats to identify how > often this feature is used > - I am of course ok with removing the feature because it helps reduce > code complexity > > https://codereview.chromium.org/1084473002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Scott, which changes do you want me to make to this CL?
I'm concerned about how this feature (or lack thereof, if we nuke it) will interact with download on demand triggered by the NTP. In this context, the download will be kicked of in the context of the NTP's WebContents. I would expect the framework to interpret that as "I started downloading an icon for foo.com, I've finished, but now I'm on the NTP, or even some other web-domain". We still want to keep those icons. On Mon, Apr 20, 2015 at 11:25 AM, Scott Violet <sky@chromium.org> wrote: > Given that this feature has been broken for a while. Lets nuke it. > > On Sun, Apr 19, 2015 at 4:59 PM, <pkotwicz@chromium.org> wrote: > > sky@ can you please take another look? Sorry for taking so long to get > back > > to > > you > > > > > > > https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... > > File components/favicon/core/favicon_handler.cc (left): > > > > > https://codereview.chromium.org/1084473002/diff/80001/components/favicon/core... > > components/favicon/core/favicon_handler.cc:301: if (!UrlMatches(url, > > url_) || PageChangedSinceFaviconWasRequested()) > > Yes, this got broken in 2012 > > (https://chromiumcodereview.appspot.com/9810023 in particular) > > I think that the feature: "Persisting the favicon which was downloaded > > even if it was not for the current page" is as old as Chrome itself. (It > > is already present in https://codereview.chromium.org/6672065/) > > However: > > - The feature made more sense back when we did not consider pages with > > multiple icons > > - I am unsure whether the value of having this feature was ever > > assessed. > > > > Currently: > > - We incorrectly clear image_urls_ on line 446 in the old code even if > > PageChangedSinceFaviconWasRequested() is false. > > - We might end up saving data for a suboptimal icon URL if the page has > > several icon URLs. We interpret the page URL changing as "save the best > > one we got so far". This will produce interesting results if we are > > replacing an expired favicon. > > > > In terms of resolutions: > > - I am ok with fixing the feature if it was intended. > > - I am ok with fixing the feature and adding UMA stats to identify how > > often this feature is used > > - I am of course ok with removing the feature because it helps reduce > > code complexity > > > > https://codereview.chromium.org/1084473002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
rogerm@, we don't know yet how download on demand will work. We are creating LargeIconService for the purpose of handling "download on demand". I think there is a 99% chance of us using a different code path for "download on demand". If we end up needing it, I do not mind doing the work to put this feature back in but I am 99% sure that we will not need it (and if we do we can reimplement it in a better and less confusing way)
LGTM
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084473002/80001
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1084473002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1084473002/100001
Message was sent while issue was closed.
Committed patchset #2 (id:100001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e002b6edc1407681c20a84f5b7cf3b249791a0bc Cr-Commit-Position: refs/heads/master@{#326042} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
