Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662443005:
Convert FaviconDownloader to use the new navigation callbacks. (Closed)
DescriptionConvert FaviconDownloader to use the new navigation callbacks.
BUG=682002
Review-Url: https://codereview.chromium.org/2662443005
Cr-Commit-Position: refs/heads/master@{#446907}
Committed: https://chromium.googlesource.com/chromium/src/+/7e6919eb66f99e4ebf388cbfe6d92a4c837d7e2b
Patch Set 1 #
Total comments: 4
Messages
Total messages: 15 (8 generated)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
jam@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... chrome/browser/extensions/favicon_downloader.cc:118: if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) I realize that this is the conversion that would leave the same behavior, but I wonder if it's the most correct. It seems like what we're trying to do here is purge pending requests when the web contents navigates, because we know that we don't need to finish them anymore. I wonder if it would make sense for this to be DidStartNavigation() instead of waiting for the navigation to finish? WDYT?
https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... chrome/browser/extensions/favicon_downloader.cc:118: if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) On 2017/01/27 21:51:51, Devlin (catching up) wrote: > I realize that this is the conversion that would leave the same behavior, but I > wonder if it's the most correct. It seems like what we're trying to do here is > purge pending requests when the web contents navigates, because we know that we > don't need to finish them anymore. I wonder if it would make sense for this to > be DidStartNavigation() instead of waiting for the navigation to finish? WDYT? DidStartNavigation might not commit though, i.e. because the user starts navigating to a url that turns out to be a download.
lgtm regardless, since this is similar behavior to what we had before, but curious for my own edification. :) https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... chrome/browser/extensions/favicon_downloader.cc:118: if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) On 2017/01/27 23:04:25, jam wrote: > On 2017/01/27 21:51:51, Devlin (catching up) wrote: > > I realize that this is the conversion that would leave the same behavior, but > I > > wonder if it's the most correct. It seems like what we're trying to do here > is > > purge pending requests when the web contents navigates, because we know that > we > > don't need to finish them anymore. I wonder if it would make sense for this > to > > be DidStartNavigation() instead of waiting for the navigation to finish? > WDYT? > > DidStartNavigation might not commit though, i.e. because the user starts > navigating to a url that turns out to be a download. Interesting, okay. And there's no way to know that during DidStartNavigation?
https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... File chrome/browser/extensions/favicon_downloader.cc (right): https://codereview.chromium.org/2662443005/diff/1/chrome/browser/extensions/f... chrome/browser/extensions/favicon_downloader.cc:118: if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) On 2017/01/28 02:24:23, Devlin (catching up) wrote: > On 2017/01/27 23:04:25, jam wrote: > > On 2017/01/27 21:51:51, Devlin (catching up) wrote: > > > I realize that this is the conversion that would leave the same behavior, > but > > I > > > wonder if it's the most correct. It seems like what we're trying to do here > > is > > > purge pending requests when the web contents navigates, because we know that > > we > > > don't need to finish them anymore. I wonder if it would make sense for this > > to > > > be DidStartNavigation() instead of waiting for the navigation to finish? > > WDYT? > > > > DidStartNavigation might not commit though, i.e. because the user starts > > navigating to a url that turns out to be a download. > > Interesting, okay. And there's no way to know that during DidStartNavigation? Right, at the start is before the network request is made, so we don't know the response data.
The CQ bit was checked by jam@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": 1, "attempt_start_ts": 1485578105950940, "parent_rev":
"54db2003b44f32d6618e70682b84fb8d13fd4e15", "commit_rev":
"7e6919eb66f99e4ebf388cbfe6d92a4c837d7e2b"}
Message was sent while issue was closed.
Description was changed from ========== Convert FaviconDownloader to use the new navigation callbacks. BUG=682002 ========== to ========== Convert FaviconDownloader to use the new navigation callbacks. BUG=682002 Review-Url: https://codereview.chromium.org/2662443005 Cr-Commit-Position: refs/heads/master@{#446907} Committed: https://chromium.googlesource.com/chromium/src/+/7e6919eb66f99e4ebf388cbfe6d9... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/7e6919eb66f99e4ebf388cbfe6d9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
