Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1081003002:
Simplify FaviconHandler::OnFaviconData() (Closed)
DescriptionSimplify FaviconHandler::OnFaviconData()
BUG=None
TEST=None
Committed: https://crrev.com/a998690349cae04f1ffd2143e5ef20eafeb0d790
Cr-Commit-Position: refs/heads/master@{#327284}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Messages
Total messages: 31 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
pkotwicz@chromium.org changed reviewers: + rogerm@chromium.org
rogerm@ can you please take a look? This CL depends on https://codereview.chromium.org/1084473002/ https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:664: // one got from page. Request the current one. The results should be empty if the type is different than the one that we got from the page. In particular, we pass in just one type to UpdateFaviconMappingsAndFetch() and GetFaviconFromFaviconService() https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:644: preferred_icon_size(), favicon_bitmap_results); Yes, storing this in |favicon_expired_or_incomplete_| would be more correct, but that's for another time
https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:646: history_results_ = favicon_bitmap_results; Do you need to save the results if they're not valid? How independent are these booleans?
rogerm@ can you please take another look? https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:646: history_results_ = favicon_bitmap_results; It is unclear what the correct behavior is. - The only user of |history_results_| is ProcessCurrentUrl(). Thus |history_results_| and |favicon_expired_or_incomplete_| should be updated at the same time. This matters when OnUpdateFaviconURL() gets called after OnFaviconData() has been called for a particular page URL - It would be nice if FaviconHandler (ProcessCurrentUrl() in particular()) depended less on state from FaviconDriver. To that end it would probably be nice to update |history_results_| only when HasValidResult() returns true.
https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:646: history_results_ = favicon_bitmap_results; The reason that it might be nice to only update |history_results_| when HasValidResult() returns true is that this is "kind of" when NotifyFaviconAvailable() is called. Figuring out why the logic for when NotifyFaviconAvailable() is called in OnFaviconDataForInitialURLFromFaviconService() and OnFaviconData() is different would be useful
LGTM. I too am trying to grok the diff between OnFaviconData and OnFaviconDataForInitialURLFromFaviconService. It looks like the latter is called with the result of trying to find the favicon in the cache, and triggers the download if nothing was in the cache, or if the cached entry appears to have expired. https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:643: bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult( has_results && HasExpired...? See ONFaviconDataForInitialURFromFaviconService()
Aside: It should be possible to make OnFaviconDataForInitialURLFromFaviconService() call ProcessCurrentUrl() if there is a current candidate.
https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/1081003002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:643: bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult( I have changed HasExpiredOrIncompleteResult() to return true if the passed in FaviconRawBitmapResult vector is empty
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org Link to the patchset: https://codereview.chromium.org/1081003002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081003002/140001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
Scott, can you please take a look?
LGTM
The CQ bit was checked by pkotwicz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081003002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Patchset #4 (id:160001) has been deleted
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1081003002/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081003002/180001
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a998690349cae04f1ffd2143e5ef20eafeb0d790 Cr-Commit-Position: refs/heads/master@{#327284} |
