New issue
Advanced search Search tips

Issue 844972 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 10
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: ----

Blocking:
issue 871211
issue 871542



Sign in to add a comment

Migrate components/update_client/url_fetcher_downloader.cc

Project Member Reported by dxie@google.com, May 20 2018

Issue description


 

Comment 1 by dxie@google.com, May 20 2018

Labels: Proj-Servicification-Canary Proj-Servicification OS-Windows OS-Linux OS-Mac OS-Chrome Proj-Servicification-network-url OS-Android
Status: Available (was: Untriaged)
Owner: toniki...@chromium.org
Status: Assigned (was: Available)
Related to  issue 844973  (which is WIP).
Issue 862988 has been merged into this issue.
Labels: Pri-1
Antonio: can we prioritize this as a bunch of duplicate bugs have been filed on it? Thanks
Status: Started (was: Assigned)
on it, now, jam.
 Issue 853810  has been merged into this issue.
Blocking: 871542
FTR, this is CL https://crrev.com/c/1160725.
Blocking: 871211
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d36e8296a5882ac323aa2e4ffb3704d438b3838

commit 8d36e8296a5882ac323aa2e4ffb3704d438b3838
Author: Antonio Gomes <tonikitoo@igalia.com>
Date: Thu Aug 09 23:14:59 2018

s13n: Expose an API to get the total of decompressed bytes of a response body

While migrating some classes from URLFetcher to SimpleURLLoader,
it is useful to have the size of the resulting decompressed body.

In case of DownloadToString* calls, this is straightfoward:
the length to the string is the size.
However, in the case of DownloadTo{Temp}File calls, where only the
resulting file path is passed to the completion callback, having
an API to get the decompressed body size is useful. It also avoids
sync calls to access the file data.

This CL adds a SimpleURLLoader::GetContentSize API that provides such
value. When used with SimpleURLLloader::SetAllowPartialResults(true),
the API returns the total decompressed body size even in the case of
errors.

BUG=773295, 844972 

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ia8cdb3326b5d5c74276e384f966a7e98f61a8c46
Reviewed-on: https://chromium-review.googlesource.com/1167683
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581952}
[modify] https://crrev.com/8d36e8296a5882ac323aa2e4ffb3704d438b3838/services/network/public/cpp/simple_url_loader.cc
[modify] https://crrev.com/8d36e8296a5882ac323aa2e4ffb3704d438b3838/services/network/public/cpp/simple_url_loader.h
[modify] https://crrev.com/8d36e8296a5882ac323aa2e4ffb3704d438b3838/services/network/public/cpp/simple_url_loader_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Sign in to add a comment