Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799273002:
Add support to process favicons from Web Manifests (Closed)
DescriptionAdd support to process favicons from Web Manifests
With two main goals:
- Treat Web Manifest icons similarly to icons inlined in the HTML head.
- Avoid downloading manifests repeatedly for the trivial cases.
We achieve the second by reusing the favicon cache itself, and using
the manifest URL as a key in the tables. This is a bit hacky, but
avoids adding considerable complexity to the History Backend and APIs.
The new logic is behind a feature toggle and is disabled by default.
BUG=690383
Review-Url: https://codereview.chromium.org/2799273002
Cr-Commit-Position: refs/heads/master@{#472786}
Committed: https://chromium.googlesource.com/chromium/src/+/f5a564cc28633d8f086f057c1cf7fd58222fb2d4
Patch Set 1 #
Total comments: 7
Patch Set 2 : Adopt icon URL. #
Total comments: 19
Patch Set 3 : Handle manifests without icons. #Patch Set 4 : Handle manifests without icons. #Patch Set 5 : Added comment. #
Total comments: 32
Patch Set 6 : Addressed comments, improved tests. #
Total comments: 8
Patch Set 7 : Fix Javascript changes in flight. #Patch Set 8 : Add hacks for demoing (DONOTSUBMIT) #
Total comments: 31
Patch Set 9 : Addressed comments. #Patch Set 10 : Rebased. #Patch Set 11 : Rebased. #Patch Set 12 : Avoid hacks for demo purposes. #Patch Set 13 : Addressed comments. #
Total comments: 7
Patch Set 14 : Reverted smart handling of HTTP status code. #Patch Set 15 : Reverted fieldtrial_testing_config.json #
Total comments: 50
Patch Set 16 : Addressed comments. #Patch Set 17 : Addressed comments (most notably, three bugs). #Patch Set 18 : Added missing include. #Patch Set 19 : Rebased. #
Total comments: 37
Patch Set 20 : Addressed comments. #
Total comments: 2
Patch Set 21 : Updated comment. #Patch Set 22 : Avoid unnecessary processing of icons. #
Total comments: 14
Patch Set 23 : Browsertest comments addressed. #Messages
Total messages: 74 (31 generated)
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
Peter, could you take a preliminary look? Please avoid the temptation of nits, it's WIP. Most notably, I'd like to: 1. Agree if all except the implementation details inside FaviconHandler look sane, hoping to bring clarity to https://bugs.chromium.org/p/chromium/issues/detail?id=707179 as in why the URL would be needed. 2. Raise awareness that this feature will likely add considerably complexity to FaviconHandler (lots of new codepaths). Or am I totally on the wrong track? 3. (Further up) Compare two variants of the implementation of FaviconHandler: use webmanifest as page URL (current) or icon URL (preferred solution in design doc).
I'll take a look at this tomorrow morning. Yes, I expect that adding support for Web Manifests will add significant complexity to FaviconHandler
Sorry about the delay in the code review https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:256: service_->SetFavicons(*manifest_url_, icon_url, icon_type, image); - We will need to add expiry logic specifically for manifest URLs - We avoid some of these problems if we use the "manifest URL" as the "icon URL" instead of the "page URL". For instance, we would not need to call SetFavicons() twice here. When we use the "manifest URL" as the "page URL" we have to call FaviconService::SetFavicons() twice. We need to store the "page URL" <-> "image data" association for the sake of the bookmark page and the history page. https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:365: current_candidate()->icon_url == notification_icon_url_ && This check will fail if the database icon URL is from the Web Manifest https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:404: valid_result->pixel_size); Why are you calling FaviconService::MergeFavicon() instead of FaviconService::SetFavicons() If we stored the "manifest URL" as the "icon URL" we would be able to use FaviconService::UpdateFaviconMappingsAndFetch() to establish the "page URL" <-> "manifest URL" mapping
PTAL. Still WIP, but now icon-URL was adopted. https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:256: service_->SetFavicons(*manifest_url_, icon_url, icon_type, image); On 2017/04/07 14:13:04, pkotwicz wrote: > - We will need to add expiry logic specifically for manifest URLs > > - We avoid some of these problems if we use the "manifest URL" as the "icon URL" > instead of the "page URL". For instance, we would not need to call SetFavicons() > twice here. When we use the "manifest URL" as the "page URL" we have to call > FaviconService::SetFavicons() twice. We need to store the "page URL" <-> "image > data" association for the sake of the bookmark page and the history page. The first argument above is convincing to go for icon URLs, and it'd be good to document that in the Design Doc. I'll change the implementation. https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:365: current_candidate()->icon_url == notification_icon_url_ && On 2017/04/07 14:13:04, pkotwicz wrote: > This check will fail if the database icon URL is from the Web Manifest Can you please elaborate this? I can see how this would be a problem if we used the manifest URL as icon URL, but this version actually doesn't. https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:404: valid_result->pixel_size); On 2017/04/07 14:13:04, pkotwicz wrote: > Why are you calling FaviconService::MergeFavicon() instead of > FaviconService::SetFavicons() > > If we stored the "manifest URL" as the "icon URL" we would be able to use > FaviconService::UpdateFaviconMappingsAndFetch() to establish the "page URL" <-> > "manifest URL" mapping I could explain why but this is a moot point anyway, unless we get back to using manifest URLs as page URLs.
https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/1/components/favicon/core/fav... components/favicon/core/favicon_handler.cc:365: current_candidate()->icon_url == notification_icon_url_ && Looks like I was wrong. I was thinking that this check would not work if OnGotInitialHistoryDataAndIconURLCandidates() is called: after OnUpdateCandidates() is called before manifest download completes However, I see that the candidates are only stored once OnGotFinalIconURLCandidates() is called https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:302: // manifest URL can be specified via Javascript? Yes, the manifest URL can be changed via JavaScript. The browser side code (e.g. InstallableManager) currently assumes that this never happens. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:314: if (redownload_icons_) { Do we need to wait till OnFaviconDataForInitialURLFromFaviconService() is called before proceeding? https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:352: manifest_download_request_.callback()); We need to take into account that the Web Manifest might not have any icons. For instance: https://www.washingtonpost.com/pb/resources/json/desktop-notifications-manife... https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); If the |icon_url| is from the Web Manifest, we have to use the manifest URL as the icon URL https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:517: favicon_bitmap_results))) { We should trigger this if() statement if the "database icon URL" is the Web Manifest URL
Thanks peter! PTAL. In this round, the most valuable feedback would be: 1. Caching of manifests without icons. 2. Caching of non-200 responses from manifests downloads (this is relevant to discuss about the required WebContents API). 3. Test coverage in general (not necessarily test implementation itself, since I suspect you'll prefer introducing a FakeManifestDownloader). https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:302: // manifest URL can be specified via Javascript? On 2017/04/11 03:47:07, pkotwicz wrote: > Yes, the manifest URL can be changed via JavaScript. The browser side code (e.g. > InstallableManager) currently assumes that this never happens. Ack, thanks. Added a new test. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:314: if (redownload_icons_) { On 2017/04/11 03:47:07, pkotwicz wrote: > Do we need to wait till OnFaviconDataForInitialURLFromFaviconService() is called > before proceeding? redownload_icons_ can only be true only after OnFaviconDataForInitialURLFromFaviconService(). Added a comment. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:352: manifest_download_request_.callback()); On 2017/04/11 03:47:07, pkotwicz wrote: > We need to take into account that the Web Manifest might not have any icons. > > For instance: > https://www.washingtonpost.com/pb/resources/json/desktop-notifications-manife... Done, implemented one proposal for this. Unless you have better ideas, I had to cache this in our in-memory 404 table for icons. I believe a cleaner solution would involve changes in the history backend. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); On 2017/04/11 03:47:07, pkotwicz wrote: > If the |icon_url| is from the Web Manifest, we have to use the manifest URL as > the icon URL This is implemented inside SetFavicon and NotifyFaviconUpdated. The second has two callers so I slightly preferred this alternative, but I'm fine either way (it's more important that tests cover all these cases, which I believe they do). https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:517: favicon_bitmap_results))) { On 2017/04/11 03:47:07, pkotwicz wrote: > We should trigger this if() statement if the "database icon URL" is the Web > Manifest URL This situation would cause current_candidate() == null in all cases, wouldn't it? It's also exercised by at least one test.
I'll take a look at your CL tomorrow morning. Sorry for the delay
Your CL looks mostly good :) Your test coverage looks ok. I have one suggestion for an additional test in line https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:314: if (redownload_icons_) { I'd recommend getting rid of this if() statement and always fetching data for the manifest URL. Otherwise there are too many cases to think about: OnFaviconDataForInitialURLFromFaviconService() OnUpdateCandidates() OnGotFinalIconURLCandidates() OnUpdateCandidates() OnFaviconDataForInitialURLFromFaviconService() OnGotFinalIconURLCandidates() OnUpdateCandidates() OnGotFinalIconURLCandidates() OnFaviconDataForInitialURLFromFaviconService() https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:352: manifest_download_request_.callback()); This is OK for now https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); I think that it is clearer if you pass the correct value to SetFavicon() and NotifyFaviconUpdated(). From looking at the code, it looks like you don't need to apply the transformation to any of the NotifyFaviconUpdated() calls. In the places that NotifyFaviconUpdated() is called, the "icon URL" should already be the "manifest URL" https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:300: Shouldn't the TOUCH_LARGEST FaviconHandler completely ignore the Web Manifest. It is weird that both the TOUCH_LARGEST and NON_TOUCH_LARGEST FaviconHandlers will download the Web Manifest https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:369: DCHECK(status_code == 200); Why the DCHECK() ? https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:370: OnGotFinalIconURLCandidates(candidates); You want to return here? https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Why is this if() checking the status code? https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:380: service_->UnableToDownloadFavicon(*manifest_url_); If we end up download the Web Manifest on each page load, we can always download the Web Manifest here. (Because it will likely already have been cached) I think that Dominick has landed an experiment which fetches the Web Manifest on page load: https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0.... It is possible that we don't need to cache Web Manifests the way we cache favicons. A default favicon name is assumed if the page does not specify one. So we are likely to get a 404 when we download a favicon. However, a default Web Manifest name is not assumed if the page does not specify one. I don't think that I am qualified to determine whether "We should download Web Manifests on page load". The Android "Emerging Markets" team can likely answer this question. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:545: favicon_bitmap_results))) { I think that this should be changed to: if (has_valid_result) { if (!current_candidate() || DoUrlsAndIconsMatch(current_candidate()->icon_url, current_candidate()->icon_type, favicon_bitmap_results) || DoUrlsAndIconsMatch(manifest_url_, favicon_base::FAVICON, favicon_bitmap_results)) { } } https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1049: CreateRawBitmapResult(kIconURL16x16)); This test looks like it is left over from when the "manifest URL" was used as the "page URL" https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1067: TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistoryIfCandidatesFaster) { This test looks like it is left over from when the "manifest URL" was used as the "page URL" https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1238: // TODO(mastiz) / DONOTSUBMIT: Add missing tests. Can you please add a test for when: - There is a Web Manifest - The Web Manifest does not contain any icon URLs (It is not a 404) - The page has an icon URL provided via a <link rel="icon"> tag - The database does not know about the page URL - The database knows about the icon URL
PTAL, low prio. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:314: if (redownload_icons_) { On 2017/04/12 22:10:06, pkotwicz wrote: > I'd recommend getting rid of this if() statement and always fetching data for > the manifest URL. > > Otherwise there are too many cases to think about: > > OnFaviconDataForInitialURLFromFaviconService() > OnUpdateCandidates() > OnGotFinalIconURLCandidates() > > OnUpdateCandidates() > OnFaviconDataForInitialURLFromFaviconService() > OnGotFinalIconURLCandidates() > > OnUpdateCandidates() > OnGotFinalIconURLCandidates() > OnFaviconDataForInitialURLFromFaviconService() Done. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); On 2017/04/12 22:10:06, pkotwicz wrote: > I think that it is clearer if you pass the correct value to SetFavicon() and > NotifyFaviconUpdated(). > > From looking at the code, it looks like you don't need to apply the > transformation to any of the NotifyFaviconUpdated() calls. In the places that > NotifyFaviconUpdated() is called, the "icon URL" should already be the "manifest > URL" Done. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:300: On 2017/04/12 22:10:06, pkotwicz wrote: > Shouldn't the TOUCH_LARGEST FaviconHandler completely ignore the Web Manifest. > > It is weird that both the TOUCH_LARGEST and NON_TOUCH_LARGEST FaviconHandlers > will download the Web Manifest This happens in upper layers: the manifest URL is provided only to one of the two handlers. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:369: DCHECK(status_code == 200); On 2017/04/12 22:10:06, pkotwicz wrote: > Why the DCHECK() ? To add clarity, but I realize you're not a big fan of CHECKs so removed. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:370: OnGotFinalIconURLCandidates(candidates); On 2017/04/12 22:10:07, pkotwicz wrote: > You want to return here? Correct, done (btw tests were failing due to this). https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/04/12 22:10:07, pkotwicz wrote: > Why is this if() checking the status code? We shouldn't call UnableToDownloadFavicon() for all HTTP response codes like 503. Similar logic exists for images, in OnDidDownloadFavicon. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:380: service_->UnableToDownloadFavicon(*manifest_url_); On 2017/04/12 22:10:07, pkotwicz wrote: > If we end up download the Web Manifest on each page load, we can always download > the Web Manifest here. (Because it will likely already have been cached) > > I think that Dominick has landed an experiment which fetches the Web Manifest on > page load: > https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0.... > > It is possible that we don't need to cache Web Manifests the way we cache > favicons. A default favicon name is assumed if the page does not specify one. So > we are likely to get a 404 when we download a favicon. However, a default Web > Manifest name is not assumed if the page does not specify one. > > I don't think that I am qualified to determine whether "We should download Web > Manifests on page load". The Android "Emerging Markets" team can likely answer > this question. If we were to always load and parse manifests, this whole patch might need to be reverted. Upper layers could simply feed in a merged list of favicons. Meanwhile, I think it's responsible to implement the caching presented here. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:545: favicon_bitmap_results))) { On 2017/04/12 22:10:06, pkotwicz wrote: > I think that this should be changed to: > > if (has_valid_result) { > if (!current_candidate() || > DoUrlsAndIconsMatch(current_candidate()->icon_url, > current_candidate()->icon_type, > favicon_bitmap_results) || > DoUrlsAndIconsMatch(manifest_url_, > favicon_base::FAVICON, > favicon_bitmap_results)) { > } > } Done. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1049: CreateRawBitmapResult(kIconURL16x16)); On 2017/04/12 22:10:07, pkotwicz wrote: > This test looks like it is left over from when the "manifest URL" was used as > the "page URL" Done. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1067: TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistoryIfCandidatesFaster) { On 2017/04/12 22:10:07, pkotwicz wrote: > This test looks like it is left over from when the "manifest URL" was used as > the "page URL" Done. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler_unittest.cc:1238: // TODO(mastiz) / DONOTSUBMIT: Add missing tests. On 2017/04/12 22:10:07, pkotwicz wrote: > Can you please add a test for when: > - There is a Web Manifest > - The Web Manifest does not contain any icon URLs (It is not a 404) > - The page has an icon URL provided via a <link rel="icon"> tag > - The database does not know about the page URL > - The database knows about the icon URL Done, UnknownManifestWithoutIconsAndKnownRegularIcons.
On 2017/04/20 18:06:33, mastiz wrote: > PTAL, low prio. > > https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... > File components/favicon/core/favicon_handler.cc (right): > > https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... > components/favicon/core/favicon_handler.cc:314: if (redownload_icons_) { > On 2017/04/12 22:10:06, pkotwicz wrote: > > I'd recommend getting rid of this if() statement and always fetching data for > > the manifest URL. > > > > Otherwise there are too many cases to think about: > > > > OnFaviconDataForInitialURLFromFaviconService() > > OnUpdateCandidates() > > OnGotFinalIconURLCandidates() > > > > OnUpdateCandidates() > > OnFaviconDataForInitialURLFromFaviconService() > > OnGotFinalIconURLCandidates() > > > > OnUpdateCandidates() > > OnGotFinalIconURLCandidates() > > OnFaviconDataForInitialURLFromFaviconService() > > Done. > > https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... > components/favicon/core/favicon_handler.cc:473: > best_favicon_.candidate.icon_type); > On 2017/04/12 22:10:06, pkotwicz wrote: > > I think that it is clearer if you pass the correct value to SetFavicon() and > > NotifyFaviconUpdated(). > > > > From looking at the code, it looks like you don't need to apply the > > transformation to any of the NotifyFaviconUpdated() calls. In the places that > > NotifyFaviconUpdated() is called, the "icon URL" should already be the > "manifest > > URL" > > Done. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > File components/favicon/core/favicon_handler.cc (right): > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:300: > On 2017/04/12 22:10:06, pkotwicz wrote: > > Shouldn't the TOUCH_LARGEST FaviconHandler completely ignore the Web Manifest. > > > > It is weird that both the TOUCH_LARGEST and NON_TOUCH_LARGEST FaviconHandlers > > will download the Web Manifest > > This happens in upper layers: the manifest URL is provided only to one of the > two handlers. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:369: DCHECK(status_code == 200); > On 2017/04/12 22:10:06, pkotwicz wrote: > > Why the DCHECK() ? > > To add clarity, but I realize you're not a big fan of CHECKs so removed. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:370: > OnGotFinalIconURLCandidates(candidates); > On 2017/04/12 22:10:07, pkotwicz wrote: > > You want to return here? > > Correct, done (btw tests were failing due to this). > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == > 404 || status_code == 200)) > On 2017/04/12 22:10:07, pkotwicz wrote: > > Why is this if() checking the status code? > > We shouldn't call UnableToDownloadFavicon() for all HTTP response codes like > 503. Similar logic exists for images, in OnDidDownloadFavicon. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:380: > service_->UnableToDownloadFavicon(*manifest_url_); > On 2017/04/12 22:10:07, pkotwicz wrote: > > If we end up download the Web Manifest on each page load, we can always > download > > the Web Manifest here. (Because it will likely already have been cached) > > > > I think that Dominick has landed an experiment which fetches the Web Manifest > on > > page load: > > > https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0.... > > > > It is possible that we don't need to cache Web Manifests the way we cache > > favicons. A default favicon name is assumed if the page does not specify one. > So > > we are likely to get a 404 when we download a favicon. However, a default Web > > Manifest name is not assumed if the page does not specify one. > > > > I don't think that I am qualified to determine whether "We should download Web > > Manifests on page load". The Android "Emerging Markets" team can likely answer > > this question. > > If we were to always load and parse manifests, this whole patch might need to be > reverted. Upper layers could simply feed in a merged list of favicons. > Meanwhile, I think it's responsible to implement the caching presented here. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler.cc:545: favicon_bitmap_results))) { > On 2017/04/12 22:10:06, pkotwicz wrote: > > I think that this should be changed to: > > > > if (has_valid_result) { > > if (!current_candidate() || > > DoUrlsAndIconsMatch(current_candidate()->icon_url, > > current_candidate()->icon_type, > > favicon_bitmap_results) || > > DoUrlsAndIconsMatch(manifest_url_, > > favicon_base::FAVICON, > > favicon_bitmap_results)) { > > } > > } > > Done. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > File components/favicon/core/favicon_handler_unittest.cc (right): > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler_unittest.cc:1049: > CreateRawBitmapResult(kIconURL16x16)); > On 2017/04/12 22:10:07, pkotwicz wrote: > > This test looks like it is left over from when the "manifest URL" was used as > > the "page URL" > > Done. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler_unittest.cc:1067: > TEST_F(FaviconHandlerTest, GetFaviconFromManifestInHistoryIfCandidatesFaster) { > On 2017/04/12 22:10:07, pkotwicz wrote: > > This test looks like it is left over from when the "manifest URL" was used as > > the "page URL" > > Done. > > https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... > components/favicon/core/favicon_handler_unittest.cc:1238: // TODO(mastiz) / > DONOTSUBMIT: Add missing tests. > On 2017/04/12 22:10:07, pkotwicz wrote: > > Can you please add a test for when: > > - There is a Web Manifest > > - The Web Manifest does not contain any icon URLs (It is not a 404) > > - The page has an icon URL provided via a <link rel="icon"> tag > > - The database does not know about the page URL > > - The database knows about the icon URL > > Done, UnknownManifestWithoutIconsAndKnownRegularIcons.
I'll take a look tomorrow
Sorry for the delay. I will look tomorrow
Sorry again for the delay https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); It looks like there is still the manifest_url_.value_or(icon_url) statement in NotifyFaviconUpdated() https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:300: I see. This is done in FaviconDriverImpl https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Perhaps FaviconHandler::OnDidDownloadManifest() should take a manifest as an argument and you can check !manifest->IsEmpty() && manifest->icons.empty() instead of checking for |status_code| == 200 It is super weird that you are trying to infer whether the Web Manifest download succeeded based on the status code https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:380: service_->UnableToDownloadFavicon(*manifest_url_); It might not be that nice because: - Web Manifest downloads take non zero time. - We want to avoid a flash of the default icon - I am unsure whether downloading the Web Manifest triggers the spinner on Desktop I think that we will likely still need to vend the icon URLs and the manifest URL to FaviconHandler prior to download the Web Manifest (even if we always downloading the Web Manifest) https://codereview.chromium.org/2799273002/diff/100001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:178: favicon_urls_ = candidates; I don't understand how this logic works. Both the favicon URLs and the Web Manifest URL can be update via JavaScript https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:326: DCHECK_EQ(page_url, url_); Can you please double check that the DCHECK_EQ() is valid for iOS? https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:334: manifest_url_ = manifest_url; Would this code work correctly if the manifest URL is dynamically changed via JavaScript In particular: - There used to be a Web Manifest URL - Prior to FaviconHandler::OnFaviconDataForManifestFromFaviconService() returning, JavaScript changes the Web Manifest URL to empty There is probably a similarly interesting scenario if JavaScript switches from a non cached Web Manifest URL to a cached Web Manifest URL https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.h:332: base::Optional<GURL> manifest_url_; Can you use an empty GURL() instead of base::Optional::hash_value() ?
Thanks! PTAL, again low-prio. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); On 2017/04/24 14:42:14, pkotwicz wrote: > It looks like there is still the manifest_url_.value_or(icon_url) statement in > NotifyFaviconUpdated() There's at least one codepath (via OnFaviconDataForManifestFromFaviconService) where this is needed. What would you suggest otherwise? We can avoid this overload of NotifyFaviconUpdated() but that'd lead to some duplicated code (call to SelectFaviconFramesFromPNGs()). https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/04/24 14:42:15, pkotwicz wrote: > Perhaps FaviconHandler::OnDidDownloadManifest() should take a manifest as an > argument and you can check > !manifest->IsEmpty() && manifest->icons.empty() > instead of checking for |status_code| == 200 How would that distinguish a 404 from 503? > > It is super weird that you are trying to infer whether the Web Manifest download > succeeded based on the status code I don't find this weird at all. Can you elaborate why you find this weird? It's analogous to image download, and I think we have legit reasons to be interested in HTTP status code (please refer to earlier question). https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:380: service_->UnableToDownloadFavicon(*manifest_url_); On 2017/04/24 14:42:15, pkotwicz wrote: > It might not be that nice because: > - Web Manifest downloads take non zero time. > - We want to avoid a flash of the default icon > - I am unsure whether downloading the Web Manifest triggers the spinner on > Desktop > > I think that we will likely still need to vend the icon URLs and the manifest > URL to FaviconHandler prior to download the Web Manifest (even if we always > downloading the Web Manifest) Acknowledged, makes sense. We'd need to see how that compares to icon download itself, and if it's negligible, a simpler solution would be desirable. https://codereview.chromium.org/2799273002/diff/100001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:178: favicon_urls_ = candidates; On 2017/04/24 14:42:15, pkotwicz wrote: > I don't understand how this logic works. Both the favicon URLs and the Web > Manifest URL can be update via JavaScript The assumption here is that DidUpdateManifestURL() is guaranteed to be called after OnFaviconUpdated(). You're right that this implementation didn't handle well Javascript updates of favicons: I improved this now (but details might change depending on the API proposed by dominickn@). https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:334: manifest_url_ = manifest_url; On 2017/04/24 14:42:15, pkotwicz wrote: > Would this code work correctly if the manifest URL is dynamically changed via > JavaScript > > In particular: > - There used to be a Web Manifest URL > - Prior to FaviconHandler::OnFaviconDataForManifestFromFaviconService() > returning, JavaScript changes the Web Manifest URL to empty > > There is probably a similarly interesting scenario if JavaScript switches from a > non cached Web Manifest URL to a cached Web Manifest URL Fixed this by splitting into two CancelableTaskTracker instances, and added the two tests you mention. https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.h:332: base::Optional<GURL> manifest_url_; On 2017/04/24 14:42:15, pkotwicz wrote: > Can you use an empty GURL() instead of base::Optional::hash_value() ? I've no objection to that, but let's defer this discussion to dominick's upcoming patch to extend WebContents API.
I'll take a look at your CL tomorrow :)
Your CL looks pretty good now :) (Including the tests) https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); When NotifyFaviconUpdated() is called as a result of OnFaviconDataForManifestFromFaviconService() |icon_url| should be equal to |manifest_url| https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) I suggest doing this: if (status_code == 404 || (!manifest->IsEmpty() && manifest->icons.empty()) { } If the HTTP status code is 503, content::Manifest::IsEmpty() should return true I think that checking for status code 200 weird. It is weird because you are checking for status code == 200 as a round about way for: - the Web Manifest was fetched correctly - the Web Manifest has no icon URLs https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2799273002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.h:332: base::Optional<GURL> manifest_url_; OK https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); - Can this be moved outside of the if() statement. That way you don't need to call it in OnGotFinalIconURLCandidates() - Shouldn't you also cancel |manifest_download_request_| here? - I am tempted to recommend creating an inner subclass which deals with fetching manifest data from the database and downloading the manifest. This would be in nice in the sense that the code here could call: manifest_fetcher_.reset() https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:652: NOTREACHED(); Can you move the removal of this if() statement and the introduction of the second CancelableTaskTracker into a separate CL (which will likely land before this CL)? https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1290: Each test should be preceeded with a comment which documents what the test is testing https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1357: SetFavicons(kPageURL, kManifestURL, FAVICON, _)); kManifestURL is the only parameter we care about https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1358: EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _)); We don't care about the page URL https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1380: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kManifestURL, _, _)); We don't care about the page URL for either OnFaviconUpdated() or SetFavicons() https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1429: // database, simply because it's not associated to the manifest's URL. This comment is confusing. Maybe remove the "simply because ..." part https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1462: EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0); Isn't this covered by the expectation on line 1469? https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1545: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _)); This is redundant with line 1551? https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1555: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL2, _)); Isn't this redundant with line 1563?
PTAL! There's a few open questions left, otherwise the review seems quite mature. As you suggest, and probably after one more iteration, I'll split some changes to dedicated CLs, e.g: - Trivial renamings. - Changes in regular tests, including manual triggering database responses. - Cancellation of ongoing candidate processing tasks. https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/20001/components/favicon/core... components/favicon/core/favicon_handler.cc:473: best_favicon_.candidate.icon_type); On 2017/05/01 04:56:53, pkotwicz wrote: > When NotifyFaviconUpdated() is called as a result of > OnFaviconDataForManifestFromFaviconService() |icon_url| should be equal to > |manifest_url| You're right, thanks. Done. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/05/01 04:56:53, pkotwicz wrote: > I suggest doing this: > > if (status_code == 404 || (!manifest->IsEmpty() && manifest->icons.empty()) { > } > > If the HTTP status code is 503, content::Manifest::IsEmpty() should return true > > I think that checking for status code 200 weird. It is weird because you are > checking for status code == 200 as a round about way for: > - the Web Manifest was fetched correctly > - the Web Manifest has no icon URLs Would that work for a manifest that returns a 200 but is empty or contains invalid JSON? We wouldn't blacklist it, then. I think verifying non-emptiness is as weird as the 200 comparison, for the purpose of inferring whether a manifest was fetched correctly. Perhaps we should blacklist anything other than 5xx? The only reason why we want this is because we assume some errors are temporary. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); On 2017/05/01 04:56:53, pkotwicz wrote: > - Can this be moved outside of the if() statement. That way you don't need to > call it in OnGotFinalIconURLCandidates() > > - Shouldn't you also cancel |manifest_download_request_| here? I don't think this is desirable because OnGotFinalIconURLCandidates() will ignore the call if the candidates didn't change. In that case, there's no need to start from scratch. In fact, cancelling here alone would abort the ongoing processing and not start a new one, which is a bug. UpdateSameIconURLsWhileProcessingShouldBeNoop() doesn't catch it, so I introduced a new dedicated test. > > - I am tempted to recommend creating an inner subclass which deals with fetching > manifest data from the database and downloading the manifest. This would be in > nice in the sense that the code here could call: > manifest_fetcher_.reset() Let's defer this discussion to when the rest of the comments are addressed. More broadly, I would love we could refactor the code to a throw-away class that does the actual fetching of manifest and also icons, but I'm not 100% convinced the result would be much simpler. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:652: NOTREACHED(); On 2017/05/01 04:56:53, pkotwicz wrote: > Can you move the removal of this if() statement and the introduction of the > second CancelableTaskTracker into a separate CL (which will likely land before > this CL)? Yes, will do, together with a few changes in tests. Let's settle on the few open high-level points and I'll start splitting the changes. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1290: On 2017/05/01 04:56:53, pkotwicz wrote: > Each test should be preceeded with a comment which documents what the test is > testing Done. No intent for sarcasm here but, based on my earlier experience, you seem quite opinionated about comment phrasing so it might be best that you propose you version of these summaries, thanks! https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1357: SetFavicons(kPageURL, kManifestURL, FAVICON, _)); On 2017/05/01 04:56:53, pkotwicz wrote: > kManifestURL is the only parameter we care about Removed kPageURL. I think FAVICON is worth verifying, since it's a new codepath. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1358: EXPECT_CALL(delegate_, OnFaviconUpdated(kPageURL, _, kManifestURL, _, _)); On 2017/05/01 04:56:53, pkotwicz wrote: > We don't care about the page URL Done. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1380: EXPECT_CALL(favicon_service_, SetFavicons(kPageURL, kManifestURL, _, _)); On 2017/05/01 04:56:53, pkotwicz wrote: > We don't care about the page URL for either OnFaviconUpdated() or SetFavicons() Done. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1429: // database, simply because it's not associated to the manifest's URL. On 2017/05/01 04:56:53, pkotwicz wrote: > This comment is confusing. > > Maybe remove the "simply because ..." part Rephrased, since I believe the "because" part is precisely the interesting part (the rest is redundant with the code). https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1462: EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0); On 2017/05/01 04:56:53, pkotwicz wrote: > Isn't this covered by the expectation on line 1469? Unless DownloadManifest were used with kIconURL12x12. I can drop it if you prefer. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1545: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _)); On 2017/05/01 04:56:53, pkotwicz wrote: > This is redundant with line 1551? Ditto. I can either ignore the problem and drop this line, leave it as is, or split image downloads from manifest downloads. WDYT? https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1555: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL2, _)); On 2017/05/01 04:56:53, pkotwicz wrote: > Isn't this redundant with line 1563? Ditto.
I agree that this CL is pretty mature https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Black listing anything other than 5xx sounds reasonable https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); > In fact, cancelling here alone would abort the ongoing processing and > not start > a new one, which is a bug. > UpdateSameIconURLsWhileProcessingShouldBeNoop() > doesn't catch it, so I introduced a new dedicated test. Isn't this because |cancelable_task_tracker_for_candidates_| is used for both: - getting the icon for the Web Manifest URL from history - getting the icon for a candidate URL from history I think that these two tasks should use different CancellableTaskTrackers to make things simpler OnUpdateCandidates() should cancel any in progress tasks to compute the candidates passed to OnGotFinalIconURLCandidates() https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1357: SetFavicons(kPageURL, kManifestURL, FAVICON, _)); Fair enough https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1462: EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0); I would rather that you drop it https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1545: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _)); I would vote for dropping the line and ignoring the problem.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: Add support to process favicons from Web Manifests With two main goals: - Treat Web Manifest icons similarly to icons inlined in the HTML head. - Avoid downloading manifests repeatedly for the trivial cases. We achieve the second by reusing the favicon cache itself, and using the manifest URL as a key in the tables. This is a bit hacky, but avoids adding considerable complexity to the History Backend and APIs. BUG=690383 ========== to ========== Add support to process favicons from Web Manifests With two main goals: - Treat Web Manifest icons similarly to icons inlined in the HTML head. - Avoid downloading manifests repeatedly for the trivial cases. We achieve the second by reusing the favicon cache itself, and using the manifest URL as a key in the tables. This is a bit hacky, but avoids adding considerable complexity to the History Backend and APIs. The new logic is behind a feature toggle and is disabled by default. BUG=690383 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
PTAL! I believe this is the actual first version that could potentially land, except for the HTTP status codes not being propagated by WebContents (marked with DONOTSUBMIT). I've put the logic behind a feature flag. I've rebased on top of the patch that extends WebContents API (currently in CQ). https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/05/04 17:28:24, pkotwicz wrote: > Black listing anything other than 5xx sounds reasonable Done. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); On 2017/05/04 17:28:25, pkotwicz wrote: > > In fact, cancelling here alone would abort the ongoing processing and > > not start > > a new one, which is a bug. > > UpdateSameIconURLsWhileProcessingShouldBeNoop() > > doesn't catch it, so I introduced a new dedicated test. > > Isn't this because |cancelable_task_tracker_for_candidates_| is used for both: > - getting the icon for the Web Manifest URL from history > - getting the icon for a candidate URL from history > I think that these two tasks should use different CancellableTaskTrackers to > make things simpler > > OnUpdateCandidates() should cancel any in progress tasks to compute the > candidates passed to OnGotFinalIconURLCandidates() I'm not really sure why splitting would be any simpler. Due to the code in GetFaviconAndUpdateMappingsUnlessIncognito() requiring the tracker, what you propose will lead to duplicate code (for incognito-handling, which is the kind of subtle code you'd rather not duplicate) or code complexity (inject tracker as parameter). Higher-level, IIUC, you're proposing that we don't cancel image lookups while we trigger the manifest's lookup. This adds yet another possible state in our state machine to think about: should the lookup completion trigger NotifyFaviconUpdated()? Should it process further candidates? I think it's simpler to make explicit that up to one DB lookup is ongoing at all times for candidate icons and manifest. This seems more natural with a single tracker. WDYT? https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:652: NOTREACHED(); On 2017/05/04 10:57:53, mastiz wrote: > On 2017/05/01 04:56:53, pkotwicz wrote: > > Can you move the removal of this if() statement and the introduction of the > > second CancelableTaskTracker into a separate CL (which will likely land before > > this CL)? > > Yes, will do, together with a few changes in tests. Let's settle on the few open > high-level points and I'll start splitting the changes. Done. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1462: EXPECT_CALL(delegate_, DownloadManifest(_, _)).Times(0); On 2017/05/04 17:28:25, pkotwicz wrote: > I would rather that you drop it Done. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1545: EXPECT_CALL(delegate_, DownloadManifest(kManifestURL1, _)); On 2017/05/04 17:28:25, pkotwicz wrote: > I would vote for dropping the line and ignoring the problem. Done.
I'll look at this CL some more tomorrow morning https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:87: base::Optional<std::vector<content::FaviconURL>> favicon_urls_; Can this be a normal std::vector? I think that we treat an empty vector just as if ContentFaviconDriver::DidUpdateFaviconURL() has not been called https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:92: base::Optional<GURL> manifest_url_; Can this just be a GURL? I think that we treat an empty |manifest_url_| just as if DidUpdateWebManifestURL() has not been called
Peter: it'd be great to target this BP (tomorrow, Friday) for this feature, now that the CL with the WebContent's API extensions has landed. Thanks! https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:87: base::Optional<std::vector<content::FaviconURL>> favicon_urls_; On 2017/05/11 06:12:57, pkotwicz wrote: > Can this be a normal std::vector? I think that we treat an empty vector just as > if ContentFaviconDriver::DidUpdateFaviconURL() has not been called The condition in content_favicon_driver.cc:199 needs to distinguish the two cases. When a web manifest URL is received, you need to know whether favicons (including an empty list) have already been received (e.g. manifest URL updated via Javascript for a page that doesn't contain linked favicons in the HTML). https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:92: base::Optional<GURL> manifest_url_; On 2017/05/11 06:12:57, pkotwicz wrote: > Can this just be a GURL? I think that we treat an empty |manifest_url_| just as > if DidUpdateWebManifestURL() has not been called This is feasible, but note that WebContents' API exposes an optional (I've pointed out your preference during code review but the suggestion was not followed). So the question is: do you prefer consistency with the WebContents API or not? I'm fine either way.
https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Did you forget to upload a patch set. There does not seem to be any 5xx handling in this CL anymore. (And the UnknownManifestWithoutIconsAndKnownRegularIcons test is failing now) https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); I think that splitting the trackers would fix "Bug #2" that I have outlined in my other comment. Passing a tracker to GetFaviconAndUpdateMappingsUnlessIncognito() is reasonable. The way I think about things is that state is reset in FaviconHandler::OnGotFinalIconURLCandidates() and the processing between FaviconHandler::OnUpdateCandidates() and FaviconHandler::OnGotFinalIconURLCandidates() is self contained (hence my earlier proposal for moving this processing to a dedicated class) I am not suggesting that you move the processing to a dedicated class in this CL I am fine if you find a way of addressing the bug while cancelling both the "manifest URL" and the "icon URL" database lookups in OnUpdateCandidates() https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:87: base::Optional<std::vector<content::FaviconURL>> favicon_urls_; I see. Thank you for pointing this out https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:92: base::Optional<GURL> manifest_url_; In my opinion being consistent with the WebContents API is not very important. Passing a raw GURL makes things simpler I think https://codereview.chromium.org/2799273002/diff/280001/chrome/browser/favicon... File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right): https://codereview.chromium.org/2799273002/diff/280001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:245: // those icons to be used.. Nit: Just one period https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:39: } // namespace Nit: "namespace" -> "anonymous namespace" https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:212: manifest_url_.reset(); Can this code be in DidFinishNavigation() instead? I am wondering whether we can move all of the code from DidStartNavigation() to DidFinishNavigation() (in a separate CL). The reason for the split is that originally only ContentFaviconDriver::DidStartNavigation() had access to the reload type. See ContentFaviconDriver::DidStartNavigationToPendingEntry() in https://chromium.googlesource.com/chromium/src/+blame/0df1d3a01bed974dfa51c33... https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:42: } Can this function return a std::vector<content::FaviconURL>() ? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:88: // Whether DidUpdateManifestURL() was called at least once after I am probably missing something. Does DidUpdateWebManifestURL() get called if the page has no Web Manifest? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:91: bool received_manifest_url_; |received_manifest_url_| seems unused? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_driver_impl.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_driver_impl.cc:106: handler->icon_types() & favicon_base::FAVICON We should document why the manifest URL is passed to the favicon_base::FAVICON FaviconHandler in a comment https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:327: const base::Optional<GURL>& manifest_url) { I found two bugs :( Scenario for Bug #1 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the database does not know about any of the favicon URLs 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL. The database has bitmap data for the manifest URL. The second OnUpdateCandidates() call is called while a favicon download is in progress The second call to OnUpdateCandidates() does not cancel the download because OnGotFinalIconURLCandidates() is not called from OnFaviconDataForManifestFromFaviconService(). I think that it would be clearer if OnGotFinalIconURLCandidates() was always called. Expected: FaviconService::SetFavicons() is not called. Actual: FaviconService::SetFavicons() is called. Scenario for Bug #2 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the database does not know about any of the favicon URLs 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL. The Web Manifest contains no icons. The second OnUpdateCandidates() call is called while a "favicon URL database request is in progress" Expected: NotifyFaviconUpdated() is called Actual: NotifyFaviconUpdated() is not called https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:333: // |candidates| or |manifest_url| could have been modified via Javascript. Nit: You can move the if() statement on line 334 outside of the containing if() statement https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:350: cancelable_task_tracker_for_candidates_.TryCancelAll(); For the sake of sanity, should we cancel |manifest_download_request_| here? It does not make a practical difference but it spares me from considering the case of an in progress Web Manifest download. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:527: // manifest URLs override icon URLs if available. Nit: "Note that manifest URLs override icon URLs if available." -> "The Manifest URL, if available, is used instead of the icon URL." There is only one "manifest URL" https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:572: if (has_valid_result) { Optional: In a separate CL you can remove the if() statement on line 573 and always call NotifyFaviconUpdated() if |has_valid_result| == true The rationale is that in the common case where current_candidate() == null we call NotifyFaviconUpdated() (despite it being possible that the favicon URLs once we get them won't match what is in the database). It should be ok to call NotifyFaviconUpdated() if: - we get the favicon URLs prior to the database request completing - the data stored in the database does not match the favicon URLs from the renderer https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:597: // and this lookup has happened earlier. How about: "since the manifest's URL is using for caching anyway," -> "since the manifest's URL is used for caching, not the icon URL," I think that my version is clearer https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:219: // Implementation of FaviconHalder::Delegate's DownloadManifest(). If a given Nit: FaviconHalder -> FaviconHandler
I think I found another bug :( (Sorry for not spotting it in a previous code review round) https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, The following scenario is buggy: 1) User is using non incognito browser 2) User visits a page - with a Web Manifest which is specified but does not contain any icon data - with a single favicon URL specified 3) The database has a mapping between the page URL to the favicon URL 4) OnFaviconDataForInitialURLFromFaviconService() runs before OnUpdateCandidates() - The GetFaviconAndUpdateMappingsUnlessIncognito() database request on line 352 will delete the "correct page URL -> favicon URL mapping" and replace it with a mapping from "page URL -> manifest URL" (because of UpdateFaviconMappingsAndFetch()) - OnGotInitialHistoryDataAndIconURLCandidates() won't do any database lookups or downloads because OnFaviconDataForInitialURLFromFaviconService() would have done the only call to NotifyFaviconUpdated() (because the Web Manifest is empty) I suggest changing FaviconHandler::OnUpdateCandidates() to FaviconHandler::OnUpdateCandidates(const GURL& page_url, const std::vector<FaviconURL>& candidates, const GURL& manifest_url) { DCHECK_EQ(page_url, url_); if (page_url != url_) return; non_manifest_original_candidates_ = candidates; original_manifest_ = manifest; if (got_favicon_from_history_) OnGotInitialHistoryDataAndIconURLCandidates(); } OnGotInitialHistoryDataAndIconURLCandidates() would do most of the processing currently in FaviconHandler::OnUpdateCandidates() https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1480: base::test::ScopedFeatureList override_features_; Nit: DISALLOW_COPY_AND_ASSIGN(FaviconHandlerManifestsEnabledTest) https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1487: // FaviconHandler::OnUpdateCandidates() is called. You mean FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() not FaviconService::OnFaviconDataForManifestFromFaviconService() ? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1515: // FaviconService::OnFaviconDataForManifestFromFaviconService() runs. You mean FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() not FaviconService::OnFaviconDataForManifestFromFaviconService() ? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1632: EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, FAVICON, _)); We don't care about the icon type here https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1640: // URL instead of the manifest's URL. How about: "This is because FaviconHandler only checks whether there is an icon cached with the manifest URL as the "icon URL" when a page has a non empty Web Manifest." https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1651: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL12x12, FAVICON, _)); We don't care about the icon type here either
Thanks, PTAL! Unfortunately a power outage slowed me down and didn't have the chance to work on the most important part which are the bugs you bring up. I might have more time later today but it'll be a stretch. Without wanting to sound pushy, the high level decision is whether the feature (the CL), considering the severity (IMO low) of the known bugs, is ready to landed prior to FF. https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/05/12 06:13:28, pkotwicz wrote: > Did you forget to upload a patch set. There does not seem to be any 5xx handling > in this CL anymore. (And the UnknownManifestWithoutIconsAndKnownRegularIcons > test is failing now) This was done previously but I've reverted it (in fact removed all HTTP status code handling) for a later patch, dependent on crbug.com/720289. Meanwhile, the idea is to treat 5xx just like 404s. https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/240001/components/favicon/con... components/favicon/content/content_favicon_driver.h:92: base::Optional<GURL> manifest_url_; On 2017/05/12 06:13:28, pkotwicz wrote: > In my opinion being consistent with the WebContents API is not very important. > Passing a raw GURL makes things simpler I think Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:39: } // namespace On 2017/05/12 06:13:28, pkotwicz wrote: > Nit: "namespace" -> "anonymous namespace" Code search (//\ namespace$) says otherwise (21K vs 1K), and besides the term anonymous is not accurate, should be unnamed. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:212: manifest_url_.reset(); On 2017/05/12 06:13:28, pkotwicz wrote: > Can this code be in DidFinishNavigation() instead? > > I am wondering whether we can move all of the code from DidStartNavigation() to > DidFinishNavigation() (in a separate CL). The reason for the split is that > originally only ContentFaviconDriver::DidStartNavigation() had access to the > reload type. See ContentFaviconDriver::DidStartNavigationToPendingEntry() in > https://chromium.googlesource.com/chromium/src/+blame/0df1d3a01bed974dfa51c33... I assume a navigation can be interrupted and hence no DidFinishNavigation called in that case. Hence, we need to reset this in DidStartNavigation. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:42: } On 2017/05/12 06:13:29, pkotwicz wrote: > Can this function return a std::vector<content::FaviconURL>() ? By reintroducing the ambiguity between not having received favicons and empty favicon list? You'll see that the comment above is wrong and some code (extensions) in fact seem to rely on that fact. I prefer an API without ambiguities, but if you're feeling strongly, I'll revert. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:88: // Whether DidUpdateManifestURL() was called at least once after On 2017/05/12 06:13:28, pkotwicz wrote: > I am probably missing something. Does DidUpdateWebManifestURL() get called if > the page has no Web Manifest? I removed the comment. Wrt to your question, in general it won't, but it *could* (via javascript). Hence, I think the comment was accurate (wrt to no guarantees), but I can see how it is misleading so removed. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:91: bool received_manifest_url_; On 2017/05/12 06:13:29, pkotwicz wrote: > |received_manifest_url_| seems unused? Removed, thanks. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_driver_impl.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_driver_impl.cc:106: handler->icon_types() & favicon_base::FAVICON On 2017/05/12 06:13:29, pkotwicz wrote: > We should document why the manifest URL is passed to the favicon_base::FAVICON > FaviconHandler in a comment Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:327: const base::Optional<GURL>& manifest_url) { On 2017/05/12 06:13:29, pkotwicz wrote: > I found two bugs :( > > Scenario for Bug #1 > 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the > database does not know about any of the favicon URLs > 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL. > The database has bitmap data for the manifest URL. The second > OnUpdateCandidates() call is called while a favicon download is in progress > > The second call to OnUpdateCandidates() does not cancel the download because > OnGotFinalIconURLCandidates() is not called from > OnFaviconDataForManifestFromFaviconService(). > > I think that it would be clearer if OnGotFinalIconURLCandidates() was always > called. > > Expected: > FaviconService::SetFavicons() is not called. > Actual: > FaviconService::SetFavicons() is called. > > Scenario for Bug #2 > 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the > database does not know about any of the favicon URLs > 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL. > The Web Manifest contains no icons. The second OnUpdateCandidates() call is > called while a "favicon URL database request is in progress" > > Expected: > NotifyFaviconUpdated() is called > Actual: > NotifyFaviconUpdated() is not called I will work on this, I think you're right bugs exist. It also seems to me that those are corner cases in reality, so what would you think of getting the feature in and polishing them in the next two weeks? https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:333: // |candidates| or |manifest_url| could have been modified via Javascript. On 2017/05/12 06:13:29, pkotwicz wrote: > Nit: You can move the if() statement on line 334 outside of the containing if() > statement Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:350: cancelable_task_tracker_for_candidates_.TryCancelAll(); On 2017/05/12 06:13:29, pkotwicz wrote: > For the sake of sanity, should we cancel |manifest_download_request_| here? It > does not make a practical difference but it spares me from considering the case > of an in progress Web Manifest download. Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:527: // manifest URLs override icon URLs if available. On 2017/05/12 06:13:29, pkotwicz wrote: > Nit: "Note that manifest URLs override icon URLs if available." -> "The Manifest > URL, if available, is used instead of the icon URL." > > There is only one "manifest URL" Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:597: // and this lookup has happened earlier. On 2017/05/12 06:13:29, pkotwicz wrote: > How about: "since the manifest's URL is using for caching anyway," -> "since the > manifest's URL is used for caching, not the icon URL," > > I think that my version is clearer Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1480: base::test::ScopedFeatureList override_features_; On 2017/05/12 07:20:26, pkotwicz wrote: > Nit: DISALLOW_COPY_AND_ASSIGN(FaviconHandlerManifestsEnabledTest) Done, although it's unnecessary because the base class is non-copyable and non-assignable. Besides, doing this for fixture classes is pointless IMO, I'd be interested in knowing more about the rationale. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1487: // FaviconHandler::OnUpdateCandidates() is called. On 2017/05/12 07:20:26, pkotwicz wrote: > You mean FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() not > FaviconService::OnFaviconDataForManifestFromFaviconService() ? Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1515: // FaviconService::OnFaviconDataForManifestFromFaviconService() runs. On 2017/05/12 07:20:26, pkotwicz wrote: > You mean FaviconHandler::OnFaviconDataForInitialURLFromFaviconService() not > FaviconService::OnFaviconDataForManifestFromFaviconService() ? Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1632: EXPECT_CALL(favicon_service_, SetFavicons(_, kManifestURL, FAVICON, _)); On 2017/05/12 07:20:26, pkotwicz wrote: > We don't care about the icon type here Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1640: // URL instead of the manifest's URL. On 2017/05/12 07:20:26, pkotwicz wrote: > How about: "This is because FaviconHandler only checks whether there is an icon > cached with the manifest URL as the "icon URL" when a page has a non empty Web > Manifest." Done. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1651: EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL12x12, FAVICON, _)); On 2017/05/12 07:20:26, pkotwicz wrote: > We don't care about the icon type here either Done.
I think that the third bug that I found affects https://www.twitter.com/manifest.json hence I am against landing the code as is https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Fair enough. You will need to delete the tests which are testing for this behavior. For instance: FaviconHandlerManifestsEnabledTest.IgnoreManifestWithPrior404 https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:39: } // namespace Fair enough https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:212: manifest_url_.reset(); My understanding is that DidFinishNavigation() should preceed calls to DidUpdateFaviconURL() and DidUpdateWebManifestURL() DidFinishNavigation() is called really early in the navigation. I think that it is called when the renderer gets the initial data for the page (prior to the web page having finished parsing and prior to window.onload() being called) https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:42: } The comment is correct for http page URLs because a default URL is passed if the page has no <link rel="icon"> tags. I see that the comment is incorrect for non http page URLs Making this function return a base::Optional is useless given that the base::Optional is stripped in FaviconDownloader::GetFaviconURLsFromWebContents() https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:88: // Whether DidUpdateManifestURL() was called at least once after Thanks https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, I think that this bug affects https://www.twitter.com/manifest.json
PTAL, thanks! https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) On 2017/05/12 15:37:41, pkotwicz wrote: > Fair enough. You will need to delete the tests which are testing for this > behavior. For instance: > FaviconHandlerManifestsEnabledTest.IgnoreManifestWithPrior404 404 behavior hasn't changed, only 5xx. That is, any failed manifest download will be blacklisted. https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); On 2017/05/12 06:13:28, pkotwicz wrote: > I think that splitting the trackers would fix "Bug #2" that I have outlined in > my other comment. > > Passing a tracker to GetFaviconAndUpdateMappingsUnlessIncognito() is reasonable. > > The way I think about things is that state is reset in > FaviconHandler::OnGotFinalIconURLCandidates() and the processing between > FaviconHandler::OnUpdateCandidates() and > FaviconHandler::OnGotFinalIconURLCandidates() is self contained (hence my > earlier proposal for moving this processing to a dedicated class) I am not > suggesting that you move the processing to a dedicated class in this CL > > I am fine if you find a way of addressing the bug while cancelling both the > "manifest URL" and the "icon URL" database lookups in OnUpdateCandidates() I might have achieved this, PTAL. I've traded simplicity for a small behavioral change (the only change not guarded by the feature flag): the no-op handling for candidate equality is now done based on the original candidate favicons (non-filtered, non-sorted). Let me know if this is something you wouldn't like to change. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:212: manifest_url_.reset(); On 2017/05/12 15:37:41, pkotwicz wrote: > My understanding is that DidFinishNavigation() should preceed calls to > DidUpdateFaviconURL() and DidUpdateWebManifestURL() > > DidFinishNavigation() is called really early in the navigation. I think that it > is called when the renderer gets the initial data for the page (prior to the web > page having finished parsing and prior to window.onload() being called) The question is whether it's guaranteed to be called. I think the code is more robust by handling the reset during start, and I don't see an immediate upside. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... File components/favicon/content/content_favicon_driver.h (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/con... components/favicon/content/content_favicon_driver.h:42: } On 2017/05/12 15:37:41, pkotwicz wrote: > The comment is correct for http page URLs because a default URL is passed if the > page has no <link rel="icon"> tags. I see that the comment is incorrect for non > http page URLs > > Making this function return a base::Optional is useless given that the > base::Optional is stripped in FaviconDownloader::GetFaviconURLsFromWebContents() Reverted. It however returns by value, since returning a reference requires some boilerplate (e.g. const member field with empty vector) due to base::optional. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:327: const base::Optional<GURL>& manifest_url) { On 2017/05/12 13:31:33, mastiz wrote: > On 2017/05/12 06:13:29, pkotwicz wrote: > > I found two bugs :( > > > > Scenario for Bug #1 > > 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the > > database does not know about any of the favicon URLs > > 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest > URL. > > The database has bitmap data for the manifest URL. The second > > OnUpdateCandidates() call is called while a favicon download is in progress > > > > The second call to OnUpdateCandidates() does not cancel the download because > > OnGotFinalIconURLCandidates() is not called from > > OnFaviconDataForManifestFromFaviconService(). > > > > I think that it would be clearer if OnGotFinalIconURLCandidates() was always > > called. > > > > Expected: > > FaviconService::SetFavicons() is not called. > > Actual: > > FaviconService::SetFavicons() is called. > > > > Scenario for Bug #2 > > 1) OnUpdateCandidates() is initially called with no Web Manifest URL and the > > database does not know about any of the favicon URLs > > 2) OnUpdateCandidates() is called with the same favicon URLs and a manifest > URL. > > The Web Manifest contains no icons. The second OnUpdateCandidates() call is > > called while a "favicon URL database request is in progress" > > > > Expected: > > NotifyFaviconUpdated() is called > > Actual: > > NotifyFaviconUpdated() is not called > > I will work on this, I think you're right bugs exist. It also seems to me that > those are corner cases in reality, so what would you think of getting the > feature in and polishing them in the next two weeks? I added the two corresponding tests which now pass, please review carefully: #1: AddKnownManifestViaJavascriptWhileImageDownload. #2: AddManifestWithoutIconsViaJavascriptWhileDatabaseLookup. https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, On 2017/05/12 15:37:42, pkotwicz wrote: > I think that this bug affects https://www.twitter.com/manifest.json Added UnknownManifestWithoutIconsAndRegularIconInHistory which now passes. In our offline discussion, you did mention something which I haven't addressed: "I would rather delay the database fetch for the "manifest URL" till the database fetch for the "page URL" has completed". I'm not even sure I understood the proposal. Possibly related, I added a TODO in the test reflecting a possible issue with this approach. The only clean alternative I see here is to avoid UpdateFaviconMappingsAndFetch() until we know a manifest contains icons and use GetFavicon() instead. Would you prefer that?
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I will look at your CL tonight
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:379: if (service_ && (status_code == 404 || status_code == 200)) Ok, I see now https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/140001/components/favicon/cor... components/favicon/core/favicon_handler.cc:345: cancelable_task_tracker_for_candidates_.TryCancelAll(); This change is reasonable. Its a neat solution to the bug https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, Yes I would prefer that. That works well with my plan with my plan of adding FaviconService::UpdateFaviconMappings() I had something like this in mind: FaviconHandler::OnUpdateCandidates() { if (manifest_url_ == manifest_url && std::equal(candidates.begin(), candidates.end(), non_manifest_original_candidates_.begin(), &FaviconURLEquals)) { return; } non_manifest_original_candidates_ = candidates; ... best_favicon_ = DownloadedFavicon(); if (base::FeatureList::IsEnabled(kFaviconsFromWebManifest)) manifest_url_ = manifest_url; if (got_favicon_from_history_) { DoNextStepOfProcessing(); } } void DoNextStepOfProcessing() { if (manifest_url_.is_empty()) { OnGotFinalIconURLCandidates(non_manifest_original_candidates_); return; } // See if there is a cached favicon for the manifest. This will update the DB // mappings which is an optimistic approach, since the download of the // manifest might fail or it might list no icons (although // WasUnableToDownloadFavicon() above attempts to capture those cases). GetFaviconAndUpdateMappingsUnlessIncognito( /*icon_url=*/manifest_url_, favicon_base::FAVICON, base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, base::Unretained(this))); } https://codereview.chromium.org/2799273002/diff/360001/components/favicon/con... File components/favicon/content/content_favicon_driver.cc (right): https://codereview.chromium.org/2799273002/diff/360001/components/favicon/con... components/favicon/content/content_favicon_driver.cc:29: ContentFaviconDriver::ManifestDownloadCallback callback, Nit: const ref https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler.cc:319: if (manifest_url_ == manifest_url && If the kFaviconsFromWebManifest is not enabled, won't this if() statement always return false? https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1504: .Times(2); In a follow up CL, it would be nice to decrease this to '1' https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1531: kManifestURL, _, _)); This should be Times(2) with https://codereview.chromium.org/2876263003/ applied https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1533: RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL); You need to store the returned FaviconHandler. Otherwise the manual callback is cancelled. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1538: EXPECT_THAT(delegate_.downloads(), IsEmpty()); Nit: It is probably worth moving this two expectations after line 1542 https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1551: Nit: Get rid of the new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1571: Nit: Nuke the new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1596: Nit: nuke the new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1621: Nit: Nuke the new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1634: // when a page has a non empty Web Manifest. Nit: "non empty" -> "non-empty" https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1638: // Test that attempting to download a manifest that returns a 404 gets Nit: "attempting to download a manifest that returns a 404" -> "a manifest URL that returns a 404" In my version it is clearer what gets blacklisted. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1697: Nit: Nuke the new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1765: For the sake of consistency with the assertions on line 1774 - 1781 you should probably check that OnFaviconUdpated() is called in this block too https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1792: Nit: nuke new line https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1801: // Database lookup for |kManifestURL1| is ongoing. Nit: kManifestURL1 -> kManifestURL https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1882: // Finalizes download, which should be thrown away as the manifest URLs was Nit: URLs -> URL https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1911: EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); This test seems wrong. FaviconHandler should download |kIconURL16x16| and call SetFavicons() with |kIconURL16x16| https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1924: // - The page link to one Web Manifest, which contains one icon. Nit: link -> links
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks! https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, On 2017/05/16 06:36:35, pkotwicz wrote: > Yes I would prefer that. That works well with my plan with my plan of adding > FaviconService::UpdateFaviconMappings() > > I had something like this in mind: > > FaviconHandler::OnUpdateCandidates() { > if (manifest_url_ == manifest_url && > std::equal(candidates.begin(), candidates.end(), > non_manifest_original_candidates_.begin(), > &FaviconURLEquals)) { > return; > } > > non_manifest_original_candidates_ = candidates; > ... > best_favicon_ = DownloadedFavicon(); > > if (base::FeatureList::IsEnabled(kFaviconsFromWebManifest)) > manifest_url_ = manifest_url; > > if (got_favicon_from_history_) { > DoNextStepOfProcessing(); > } > } > > void DoNextStepOfProcessing() { > if (manifest_url_.is_empty()) { > OnGotFinalIconURLCandidates(non_manifest_original_candidates_); > return; > } > > // See if there is a cached favicon for the manifest. This will update the DB > // mappings which is an optimistic approach, since the download of the > // manifest might fail or it might list no icons (although > // WasUnableToDownloadFavicon() above attempts to capture those cases). > GetFaviconAndUpdateMappingsUnlessIncognito( > /*icon_url=*/manifest_url_, favicon_base::FAVICON, > base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, > base::Unretained(this))); > } I have some trouble following this discussion, might be worth a VC (luckily, we have the weekly sync today). Looks like got_favicon_from_history_ is the key behind the idea of "I would rather delay the database fetch for the "manifest URL". However, remember that the History task runner is a sequenced task runner and requests cannot overtake each other, so OnFaviconDataForManifestFromFaviconService() is guaranteed to be called after OnFaviconDataForInitialURLFromFaviconService(). I added a DCHECK and a comment to reflect this. Besides, I start to doubt the original description of the bug is legit. Won't UpdateFaviconMappingsAndFetch(kManifestURL) be ignored because kManifestURL is not known? Anyway, this now has a browser test too: LoadRegularIconDespiteWebManifestWithoutIcons https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler.cc:319: if (manifest_url_ == manifest_url && On 2017/05/16 06:36:35, pkotwicz wrote: > If the kFaviconsFromWebManifest is not enabled, won't this if() statement always > return false? Not always but you're right, it deserves something better. Changed. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1504: .Times(2); On 2017/05/16 06:36:37, pkotwicz wrote: > In a follow up CL, it would be nice to decrease this to '1' Done, in this CL. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1531: kManifestURL, _, _)); On 2017/05/16 06:36:36, pkotwicz wrote: > This should be Times(2) with https://codereview.chromium.org/2876263003/ applied I had already rebased on top of that CL. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1533: RunHandlerWithSimpleFaviconCandidates({kIconURL12x12}, kManifestURL); On 2017/05/16 06:36:36, pkotwicz wrote: > You need to store the returned FaviconHandler. Otherwise the manual callback is > cancelled. Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1538: EXPECT_THAT(delegate_.downloads(), IsEmpty()); On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: It is probably worth moving this two expectations after line 1542 Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1551: On 2017/05/16 06:36:37, pkotwicz wrote: > Nit: Get rid of the new line All (or most) tests prior to this CL use a new line to separate local constants from the rest. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1571: On 2017/05/16 06:36:37, pkotwicz wrote: > Nit: Nuke the new line Ditto. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1596: On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: nuke the new line Ditto. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1621: On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: Nuke the new line Ditto. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1634: // when a page has a non empty Web Manifest. On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: "non empty" -> "non-empty" Done and change other preceding occurrences too. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1638: // Test that attempting to download a manifest that returns a 404 gets On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: "attempting to download a manifest that returns a 404" -> "a manifest URL > that returns a 404" > > In my version it is clearer what gets blacklisted. Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1697: On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: Nuke the new line Same as earlier. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1765: On 2017/05/16 06:36:36, pkotwicz wrote: > For the sake of consistency with the assertions on line 1774 - 1781 you should > probably check that OnFaviconUdpated() is called in this block too Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1792: On 2017/05/16 06:36:37, pkotwicz wrote: > Nit: nuke new line Same as earlier. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1801: // Database lookup for |kManifestURL1| is ongoing. On 2017/05/16 06:36:35, pkotwicz wrote: > Nit: kManifestURL1 -> kManifestURL Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1882: // Finalizes download, which should be thrown away as the manifest URLs was On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: URLs -> URL Done. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1911: EXPECT_CALL(delegate_, OnFaviconUpdated(_, _, _, _, _)).Times(0); On 2017/05/16 06:36:36, pkotwicz wrote: > This test seems wrong. FaviconHandler should download |kIconURL16x16| and call > SetFavicons() with |kIconURL16x16| Fixed, PTAL. https://codereview.chromium.org/2799273002/diff/360001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1924: // - The page link to one Web Manifest, which contains one icon. On 2017/05/16 06:36:36, pkotwicz wrote: > Nit: link -> links Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2799273002/diff/380001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/380001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1887: // Test that SetFavicons() is not called when: Nit: You need to update the test comment
https://codereview.chromium.org/2799273002/diff/380001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/380001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1887: // Test that SetFavicons() is not called when: On 2017/05/16 15:17:15, pkotwicz wrote: > Nit: You need to update the test comment Done.
https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/280001/components/favicon/cor... components/favicon/core/favicon_handler.cc:354: base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, On 2017/05/16 11:05:22, mastiz wrote: > On 2017/05/16 06:36:35, pkotwicz wrote: > > Yes I would prefer that. That works well with my plan with my plan of adding > > FaviconService::UpdateFaviconMappings() > > > > I had something like this in mind: > > > > FaviconHandler::OnUpdateCandidates() { > > if (manifest_url_ == manifest_url && > > std::equal(candidates.begin(), candidates.end(), > > non_manifest_original_candidates_.begin(), > > &FaviconURLEquals)) { > > return; > > } > > > > non_manifest_original_candidates_ = candidates; > > ... > > best_favicon_ = DownloadedFavicon(); > > > > if (base::FeatureList::IsEnabled(kFaviconsFromWebManifest)) > > manifest_url_ = manifest_url; > > > > if (got_favicon_from_history_) { > > DoNextStepOfProcessing(); > > } > > } > > > > void DoNextStepOfProcessing() { > > if (manifest_url_.is_empty()) { > > OnGotFinalIconURLCandidates(non_manifest_original_candidates_); > > return; > > } > > > > // See if there is a cached favicon for the manifest. This will update the > DB > > // mappings which is an optimistic approach, since the download of the > > // manifest might fail or it might list no icons (although > > // WasUnableToDownloadFavicon() above attempts to capture those cases). > > GetFaviconAndUpdateMappingsUnlessIncognito( > > /*icon_url=*/manifest_url_, favicon_base::FAVICON, > > base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService, > > base::Unretained(this))); > > } > > I have some trouble following this discussion, might be worth a VC (luckily, we > have the weekly sync today). > > Looks like got_favicon_from_history_ is the key behind the idea of "I would > rather delay the database fetch for the "manifest URL". However, remember that > the History task runner is a sequenced task runner and requests cannot overtake > each other, so OnFaviconDataForManifestFromFaviconService() is guaranteed to be > called after OnFaviconDataForInitialURLFromFaviconService(). I added a DCHECK > and a comment to reflect this. > > Besides, I start to doubt the original description of the bug is legit. Won't > UpdateFaviconMappingsAndFetch(kManifestURL) be ignored because kManifestURL is > not known? > > Anyway, this now has a browser test too: > LoadRegularIconDespiteWebManifestWithoutIcons As discussed offline: the original bug is not an actual issue so I reverted some precautions, which are reflected in the unit test (UnknownManifestWithoutIconsAndRegularIconInHistory). There was some red-herring due to the browser test failing but that was an issue with the browser test which I now fixed via a periodic check according to our offline discussion.
Just comments for the browser test https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right): https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:138: // FaviconHandler. I find this comment confusing How about: "We need to poll because Delegate::OnFaviconUpdated() is not guaranteed to be called upon completion of the last database request / download. In particular, OnFaviconUpdated() might not be called if a database request confirms the data sent in the previous OnFaviconUpdated() call." https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:139: CallOnNotificationPeriodically(); Maybe rename this function CheckStopWaitingPeriodically() https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:147: const gfx::Image& image) override { Should we just delete this function now that we poll? https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:163: OnNotification(); Can we call EndLoopIfCanStopWaiting() directly? https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:320: // Initial visit with the feature still disabled. This comment does not really match up https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:334: delegate->Reset(); Do you need |delegate| at all for this test? https://codereview.chromium.org/2799273002/diff/420001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2799273002/diff/420001/components/favicon/cor... components/favicon/core/favicon_handler.cc:347: manifest_url_ = GURL(); For a follow up CL. Setting |manifest_url_| here affects the if() statement on lines 323 - 329 https://codereview.chromium.org/2799273002/diff/420001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2799273002/diff/420001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:1637: // Test a manifest that returns a 404 gets blacklisted via Nit: "Test" -> "Test that"
Thanks, PTAL. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right): https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:138: // FaviconHandler. On 2017/05/17 05:20:34, pkotwicz wrote: > I find this comment confusing > > How about: "We need to poll because Delegate::OnFaviconUpdated() is not > guaranteed to be called upon completion of the last database request / download. > In particular, OnFaviconUpdated() might not be called if a database request > confirms the data sent in the previous OnFaviconUpdated() call." Done. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:139: CallOnNotificationPeriodically(); On 2017/05/17 05:20:34, pkotwicz wrote: > Maybe rename this function CheckStopWaitingPeriodically() Done. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:147: const gfx::Image& image) override { On 2017/05/17 05:20:34, pkotwicz wrote: > Should we just delete this function now that we poll? Done. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:163: OnNotification(); On 2017/05/17 05:20:34, pkotwicz wrote: > Can we call EndLoopIfCanStopWaiting() directly? Done. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:320: // Initial visit with the feature still disabled. On 2017/05/17 05:20:34, pkotwicz wrote: > This comment does not really match up Done. https://codereview.chromium.org/2799273002/diff/420001/chrome/browser/favicon... chrome/browser/favicon/content_favicon_driver_browsertest.cc:334: delegate->Reset(); On 2017/05/17 05:20:34, pkotwicz wrote: > Do you need |delegate| at all for this test? Done.
LGTM! Thank you for bearing with me!
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1495109699228310,
"parent_rev": "8bf05bb205b2693eae180c0b69c7aa5385a3592f", "commit_rev":
"f5a564cc28633d8f086f057c1cf7fd58222fb2d4"}
Message was sent while issue was closed.
Description was changed from ========== Add support to process favicons from Web Manifests With two main goals: - Treat Web Manifest icons similarly to icons inlined in the HTML head. - Avoid downloading manifests repeatedly for the trivial cases. We achieve the second by reusing the favicon cache itself, and using the manifest URL as a key in the tables. This is a bit hacky, but avoids adding considerable complexity to the History Backend and APIs. The new logic is behind a feature toggle and is disabled by default. BUG=690383 ========== to ========== Add support to process favicons from Web Manifests With two main goals: - Treat Web Manifest icons similarly to icons inlined in the HTML head. - Avoid downloading manifests repeatedly for the trivial cases. We achieve the second by reusing the favicon cache itself, and using the manifest URL as a key in the tables. This is a bit hacky, but avoids adding considerable complexity to the History Backend and APIs. The new logic is behind a feature toggle and is disabled by default. BUG=690383 Review-Url: https://codereview.chromium.org/2799273002 Cr-Commit-Position: refs/heads/master@{#472786} Committed: https://chromium.googlesource.com/chromium/src/+/f5a564cc28633d8f086f057c1cf7... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001) as https://chromium.googlesource.com/chromium/src/+/f5a564cc28633d8f086f057c1cf7... |
