CrxDownloader::Result is not used in production code when passed from UrlFetcherDownloader::OnURLFetchDownloadProgress |
||
Issue description
void UrlFetcherDownloader::OnURLFetchDownloadProgress(
const net::URLFetcher* source,
int64_t current,
int64_t total,
int64_t current_network_bytes) {
(..)
Result result;
result.downloaded_bytes = downloaded_bytes_;
result.total_bytes = total_bytes_;
OnDownloadProgress(result);
}
|result| keeps getting passed as paramater 'till it reaches either
Component::StateDownloadingDiff::DownloadProgress or
Component::StateDownloading::DownloadProgress.
It is not used from both these methods.
On the other hand, unit test code uses |result|.
Given that we are migrating this code from URLFetcher to
SimpleURLLoader, which do not have a "progress update" hook in place,
it makes sense to clean this up while/after the migration.
This bug tracks this.
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0807dd6facfb91816655d58e5c6f429fd9bb5590 commit 0807dd6facfb91816655d58e5c6f429fd9bb5590 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Fri Aug 10 14:28:47 2018 Migrate components/update_client/url_fetcher_downloader.cc URLFetcher will stop working with advent of Network Service, and SimpleURLLoader is the replacement API for most clients. This CL migrates UrlFetcherDownloader away from URLFetcher. Note that UrlFetcherDownloader makes use of URLFetcher's "progress update" hook, in order to track fetch progresses. It turns out the progress values were not being used in production at all: UrlFetcherDownloader::OnURLFetchDownloadProgress used to instantiate a CrxDownloader::Result object, and pass it all the way to Component::StateDownloadingDiff::DownloadComplete and Component::StateDownloading::DownloadComplete. There, |result| parameter was being ignored in both methods. Given SimpleURLLoader's lack of ability to hook to a progress update callback at the time of doing this migration, and that the progress values were not being used in production code anyways, this CL replaces URLFetcher's progress upload hook by SimpleURLLoader's "response started" hook, to indicate that the load has been triggered, and update Component's status accordingly. Hence, references to |CrxDownloader::Result::downloaded_bytes| and |CrxDownloader::Result::total_bytes| were removed from CrxDownloaderTest.* (components/update_client/crx_downloader_unittest.cc). On the other hand, references to CrxDownloader::DownloadMetrics::downloaded_bytes and |CrxDownloader::DownloadMetrics::total_bytes| remain unchanged (update_client::BuildDownloadCompleteEventElement uses them). Bug:844972, 871211 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I50410b46576263aec9a9a4b648a8a627f4832702 Reviewed-on: https://chromium-review.googlesource.com/1160725 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Joshua Pawlicki <waffles@chromium.org> Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Cr-Commit-Position: refs/heads/master@{#582150} [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/BUILD.gn [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/component.cc [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/crx_downloader.cc [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/crx_downloader.h [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/crx_downloader_unittest.cc [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/update_client_unittest.cc [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/url_fetcher_downloader.cc [modify] https://crrev.com/0807dd6facfb91816655d58e5c6f429fd9bb5590/components/update_client/url_fetcher_downloader.h
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31237fb5f3bba96c2e260464de91e2fb6d9f29ac commit 31237fb5f3bba96c2e260464de91e2fb6d9f29ac Author: Antonio Gomes <tonikitoo@igalia.com> Date: Mon Aug 27 19:11:03 2018 Clean up CrxDownloader::Result and CrxDownloader::ProgressCallback This is a follow up of CL https://crrev.com/c/1136518 where, CrDownloader/UrlFetcherDownloader migrated from URLFetcher to SimpleURLLoader. As part of the migration, it was verified that CrxDownloader::Result had struct members only used in unit tests: |downloaded_bytes| and |total_bytes|. Additionally, the CrxDownloader::ProgressCallback did not use its |result| parameter. This CL removes the struct members from CrxDownloader::Result as well as their use (mocked up anyways) in unittests, and removes the unused |result| parameter from CrxDownloader::ProgressCallback. BUG= 844972 , 871211 Change-Id: I5b36c63784ff2632f71c1e5a0cd44b6e0c7c7f37 Reviewed-on: https://chromium-review.googlesource.com/1190324 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: Joshua Pawlicki <waffles@chromium.org> Cr-Commit-Position: refs/heads/master@{#586361} [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/background_downloader_win.cc [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/component.cc [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/component.h [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/crx_downloader.cc [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/crx_downloader.h [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/crx_downloader_unittest.cc [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/update_client_unittest.cc [modify] https://crrev.com/31237fb5f3bba96c2e260464de91e2fb6d9f29ac/components/update_client/url_fetcher_downloader.cc
,
Aug 31
|
||
►
Sign in to add a comment |
||
Comment 1 by toniki...@chromium.org
, Aug 9