Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880933002:
Download driver for components/download. (Closed)
|
Created:
3 years, 7 months ago by xingliu Modified:
3 years, 7 months ago Reviewers:
asanka, David Trainor- moved to gerrit, jochen (gone - plz use gerrit), Peter Beverloo, ncarter (slow), dcheng, qinmin CC:
blundell+watchlist_chromium.org, chromium-reviews, darin-cc_chromium.org, droger+watchlist_chromium.org, jam, sdefresne+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDownload driver for components/download.
This CL introduces the glue layer between //components/download and
//content. The DownloadDriver interface will be the only place to access
content functionalities from download service.
Also provides a test support class for //components/download code. The
test support version of DownloadManager is not done in this CL.
The response/request headers pumping is not done in this CL, which
requires tweak on download network code.
BUG=717180
Review-Url: https://codereview.chromium.org/2880933002
Cr-Commit-Position: refs/heads/master@{#474191}
Committed: https://chromium.googlesource.com/chromium/src/+/d64fde65aa583828811a43bddb897726590a403a
Patch Set 1 #
Total comments: 20
Patch Set 2 : Work on feedbacks. #
Total comments: 22
Patch Set 3 : Work on feedback. #
Total comments: 4
Patch Set 4 : Work on feedback. #Patch Set 5 : Fix windows compiling. #
Total comments: 2
Patch Set 6 : DEPS polish. #
Total comments: 2
Patch Set 7 : Polish manager logic. #
Total comments: 7
Patch Set 8 : use MakeRefCounted. #Patch Set 9 : Add a TODO for guid collision. #Patch Set 10 : Polish comment. #Messages
Total messages: 68 (30 generated)
The CQ bit was checked by xingliu@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:36: // TODO(xingliu): Handles the request headers from |params|, needs to tweak Handles -> Handle, needs -> need Passes -> Pass https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:37: // download network stack. Passes the file path and file name. We should make the file name the guid or something. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:71: observer_ = observer; Do we need to notify the observer that we're initialized here if the DownloadManager is already initialized? I guess we can check IsReady as well, but it might just add more complexity to the caller. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:85: item->GetReceivedBytes()); Remove observer on the item here and/or when it fails? https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:87: observer_->OnDownloadUpdated(item->GetGuid(), item->GetReceivedBytes()); We'll need estimated progress as well. Would it just make sense to have a DriverEntryState struct that has things like: guid state (not started, progress, paused, complete, cancelled, failed, etc.) percent completed bytes received request headers request response code? etc? https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:89: content::DownloadInterruptReason::DOWNLOAD_INTERRUPT_REASON_NONE) { We probably need all of the paused/resumed information as well so we can track other in-progress downloads. See state suggestion above. https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl.h (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.h:32: void SetObserver(DownloadDriver::Observer* observer) override; Should we just have an initialize method that takes a Client? https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.h:49: DownloadDriver::Observer* observer_; Can this be called Client or something non-observer? Observer just seems like a 1:many concept. https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl_unittest.cc (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl_unittest.cc:56: TEST_F(DownloadDriverImplTest, DownloadItemUpdateEvents) { Can we test things like setup flow as well? https://codereview.chromium.org/2880933002/diff/1/components/download/interna... File components/download/internal/download_driver.h (right): https://codereview.chromium.org/2880933002/diff/1/components/download/interna... components/download/internal/download_driver.h:30: virtual void OnDriverReady() = 0; We will need a list of (at least) all in progress downloads here. Should we just pass that through on the ready call?
dtrainor@chromium.org changed reviewers: + peter@chromium.org
Thanks for the feedback, PTAL. https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:36: // TODO(xingliu): Handles the request headers from |params|, needs to tweak On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > Handles -> Handle, needs -> need Passes -> Pass Done. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:37: // download network stack. Passes the file path and file name. On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > We should make the file name the guid or something. Done. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:71: observer_ = observer; On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > Do we need to notify the observer that we're initialized here if the > DownloadManager is already initialized? I guess we can check IsReady as well, > but it might just add more complexity to the caller. Done, change to pass a Client* through Initialize call, removed IsReady. Client will be notified in the Initialize call if DownloadManager is already initialized. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:85: item->GetReceivedBytes()); On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > Remove observer on the item here and/or when it fails? Done, good catch. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:87: observer_->OnDownloadUpdated(item->GetGuid(), item->GetReceivedBytes()); On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > We'll need estimated progress as well. Would it just make sense to have a > DriverEntryState struct that has things like: > > guid > state (not started, progress, paused, complete, cancelled, failed, etc.) > percent completed > bytes received > request headers > request response code? > etc? Done. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.cc:89: content::DownloadInterruptReason::DOWNLOAD_INTERRUPT_REASON_NONE) { On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > We probably need all of the paused/resumed information as well so we can track > other in-progress downloads. See state suggestion above. Done. added the is_paused, if we need more thing we can add later. https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl.h (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.h:32: void SetObserver(DownloadDriver::Observer* observer) override; On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > Should we just have an initialize method that takes a Client? Done. https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl.h:49: DownloadDriver::Observer* observer_; On 2017/05/15 20:08:14, David Trainor-ping if over 24h wrote: > Can this be called Client or something non-observer? Observer just seems like a > 1:many concept. Done. https://codereview.chromium.org/2880933002/diff/1/components/download/content... File components/download/content/download_driver_impl_unittest.cc (right): https://codereview.chromium.org/2880933002/diff/1/components/download/content... components/download/content/download_driver_impl_unittest.cc:56: TEST_F(DownloadDriverImplTest, DownloadItemUpdateEvents) { On 2017/05/15 20:08:15, David Trainor-ping if over 24h wrote: > Can we test things like setup flow as well? Done. https://codereview.chromium.org/2880933002/diff/1/components/download/interna... File components/download/internal/download_driver.h (right): https://codereview.chromium.org/2880933002/diff/1/components/download/interna... components/download/internal/download_driver.h:30: virtual void OnDriverReady() = 0; On 2017/05/15 20:08:15, David Trainor-ping if over 24h wrote: > We will need a list of (at least) all in progress downloads here. Should we > just pass that through on the ready call? I think get the vector of all downloads here is kind of expensive. Added a find call that returns a DriverEntry. So the service internal code can just query each entry based on database records. We can add it if we really need it.
https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/BUILD.gn (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/BUILD.gn:14: "driver_entry.cc", This needs to be in internal. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/DEPS (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/DEPS:1: include_rules = [ We should make the internal deps -components/download/content. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:17: download_manager_->AddObserver(this); Maybe do this ininitialize? That way we don't have to worry about getting callbacks without a client? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:23: DCHECK(!client_ && download_manager_); Don't need the download_manager_ dcheck here right? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:78: return item ? DriverEntry::Create(item) : entry; Your conversion function might have to live somewhere outside of DriverEntry. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:83: if (!client_) We should probably still clean up our observer ties? Maybe just DCHECK client? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/download_driver_impl.h (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.h:25: const base::FilePath& dir); Elaborate on dir in a comment and/or rename? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/driver_entry.cc (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.cc:27: return DriverEntry::State::UNKNOWN; move the bottom two lines to default? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.cc:36: DriverEntry DriverEntry::Create(const content::DownloadItem* item) { Move this to somewhere in content/. You'll have to move DriverEntry to internal/. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/driver_entry.h (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.h:42: bool empty() const; valid? https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.h:60: // The response headers for the download. "...for the most recent download request?"
The CQ bit was checked by xingliu@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi, thanks for the review, PTAL, fixed the dependency issue in driver_entry.h https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/BUILD.gn (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/BUILD.gn:14: "driver_entry.cc", On 2017/05/18 18:29:43, David Trainor-ping if over 24h wrote: > This needs to be in internal. Done, good catch. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/DEPS (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/DEPS:1: include_rules = [ On 2017/05/18 18:29:43, David Trainor-ping if over 24h wrote: > We should make the internal deps -components/download/content. Done. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:17: download_manager_->AddObserver(this); On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > Maybe do this ininitialize? That way we don't have to worry about getting > callbacks without a client? Done. Makes sense. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:23: DCHECK(!client_ && download_manager_); On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > Don't need the download_manager_ dcheck here right? Done, forgot we have a DCHECK in the ctor. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:78: return item ? DriverEntry::Create(item) : entry; On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > Your conversion function might have to live somewhere outside of DriverEntry. Done. Move the conversion function to this file as helper function. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.cc:83: if (!client_) On 2017/05/18 18:29:43, David Trainor-ping if over 24h wrote: > We should probably still clean up our observer ties? Maybe just DCHECK client? Done. Changed all if check to DCHECK(client_);. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/download_driver_impl.h (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/download_driver_impl.h:25: const base::FilePath& dir); On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > Elaborate on dir in a comment and/or rename? Done. Comment added here. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/driver_entry.cc (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.cc:27: return DriverEntry::State::UNKNOWN; On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > move the bottom two lines to default? Done. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.cc:36: DriverEntry DriverEntry::Create(const content::DownloadItem* item) { On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > Move this to somewhere in content/. You'll have to move DriverEntry to > internal/. Done. This is now a static function in driver impl. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... File components/download/content/driver_entry.h (right): https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.h:42: bool empty() const; On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > valid? Done. https://codereview.chromium.org/2880933002/diff/20001/components/download/con... components/download/content/driver_entry.h:60: // The response headers for the download. On 2017/05/18 18:29:44, David Trainor-ping if over 24h wrote: > "...for the most recent download request?" Done.
lgtm % optional check and valid() removal. https://codereview.chromium.org/2880933002/diff/40001/components/download/con... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/40001/components/download/con... components/download/content/download_driver_impl.cc:43: if (!item) DCHECK item instead? https://codereview.chromium.org/2880933002/diff/40001/components/download/con... components/download/content/download_driver_impl.cc:116: DriverEntry DownloadDriverImpl::Find(const std::string& guid) { base::Optional<DriverEntry>
The CQ bit was checked by xingliu@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 xingliu@chromium.org
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2880933002/#ps60001 (title: "Work on feedback.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by xingliu@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...
xingliu@chromium.org changed reviewers: + jochen@chromium.org, nick@chromium.org
Hi, PTAL, thanks. +jochen@ for DEPS file review: components/download/content/DEPS review. +nick@ for content/public review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2880933002/diff/80001/components/download/con... File components/download/content/DEPS (right): https://codereview.chromium.org/2880933002/diff/80001/components/download/con... components/download/content/DEPS:2: "+content", should be content/public/browser
https://codereview.chromium.org/2880933002/diff/40001/components/download/con... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/40001/components/download/con... components/download/content/download_driver_impl.cc:43: if (!item) On 2017/05/18 23:51:19, David Trainor-ping if over 24h wrote: > DCHECK item instead? Done. https://codereview.chromium.org/2880933002/diff/40001/components/download/con... components/download/content/download_driver_impl.cc:116: DriverEntry DownloadDriverImpl::Find(const std::string& guid) { On 2017/05/18 23:51:19, David Trainor-ping if over 24h wrote: > base::Optional<DriverEntry> Done. https://codereview.chromium.org/2880933002/diff/80001/components/download/con... File components/download/content/DEPS (right): https://codereview.chromium.org/2880933002/diff/80001/components/download/con... components/download/content/DEPS:2: "+content", On 2017/05/22 11:58:05, jochen wrote: > should be content/public/browser Done. Thanks.
content/public lgtm
https://codereview.chromium.org/2880933002/diff/100001/components/download/co... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/100001/components/download/co... components/download/content/download_driver_impl.cc:157: manager->RemoveObserver(this); is this needed?
Thanks for the reviews. https://codereview.chromium.org/2880933002/diff/100001/components/download/co... File components/download/content/download_driver_impl.cc (right): https://codereview.chromium.org/2880933002/diff/100001/components/download/co... components/download/content/download_driver_impl.cc:157: manager->RemoveObserver(this); On 2017/05/22 17:16:41, qinmin wrote: > is this needed? Done, move this call to destructor. This is a good point, also polished the logic related to |manager_| life cycle, I think |manager_| can be shut down at any point by content embedder.
Ping~.
xingliu@chromium.org changed reviewers: + asanka@chromium.org, dcheng@chromium.org
+dcheng@, PTAL at change in components/download/content/DEPS. for +base deps. +asanka@, PTAL at change in components/download/content/DEPS. for +net deps. Thanks.
https://codereview.chromium.org/2880933002/diff/120001/components/download/in... File components/download/internal/driver_entry.h (right): https://codereview.chromium.org/2880933002/diff/120001/components/download/in... components/download/internal/driver_entry.h:24: struct DriverEntry { download_row has a lot of information about a download, can we reuse that instead of creating a new class https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... content/browser/download/download_item_impl.cc:206: : guid_(info.guid.empty() ? base::ToUpperASCII(base::GenerateGUID()) will this download item be written to the history db? if so, why the GUID generation logic is here instead of in the history db?
base deps lgtm https://codereview.chromium.org/2880933002/diff/120001/components/download/in... File components/download/internal/test/test_download_driver.cc (right): https://codereview.chromium.org/2880933002/diff/120001/components/download/in... components/download/internal/test/test_download_driver.cc:64: make_scoped_refptr(new net::HttpResponseHeaders("HTTP/1.1 200")); Nit: consider using base::MakeRefCounted<net::HttpResponseHeaders>(...)
deps lgtm
https://codereview.chromium.org/2880933002/diff/120001/components/download/in... File components/download/internal/driver_entry.h (right): https://codereview.chromium.org/2880933002/diff/120001/components/download/in... components/download/internal/driver_entry.h:24: struct DriverEntry { On 2017/05/23 04:21:24, qinmin wrote: > download_row has a lot of information about a download, can we reuse that > instead of creating a new class It doesn't have response_header, also I think it will good if components/download does not depend on components/history. wdyt? https://codereview.chromium.org/2880933002/diff/120001/components/download/in... File components/download/internal/test/test_download_driver.cc (right): https://codereview.chromium.org/2880933002/diff/120001/components/download/in... components/download/internal/test/test_download_driver.cc:64: make_scoped_refptr(new net::HttpResponseHeaders("HTTP/1.1 200")); On 2017/05/23 05:35:08, dcheng wrote: > Nit: consider using base::MakeRefCounted<net::HttpResponseHeaders>(...) Done. https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... content/browser/download/download_item_impl.cc:206: : guid_(info.guid.empty() ? base::ToUpperASCII(base::GenerateGUID()) On 2017/05/23 04:21:24, qinmin wrote: > will this download item be written to the history db? if so, why the GUID > generation logic is here instead of in the history db? I think this logic happens before writing to history db. Also previously we generated the uuid here. I guess the reason to generate uuid here is, the in-memory object still needs a uuid before db persists.
https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... content/browser/download/download_item_impl.cc:206: : guid_(info.guid.empty() ? base::ToUpperASCII(base::GenerateGUID()) On 2017/05/23 16:56:13, xingliu wrote: > On 2017/05/23 04:21:24, qinmin wrote: > > will this download item be written to the history db? if so, why the GUID > > generation logic is here instead of in the history db? > > I think this logic happens before writing to history db. Also previously we > generated the uuid here. > > I guess the reason to generate uuid here is, the in-memory object still needs a > uuid before db persists. > Sorry, what I am saying is that why not let DownloadItemImpl create the GUID, and pass it back to the caller of DownloadDriver in onDownloadStarted()?
On 2017/05/23 18:12:17, qinmin wrote: > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > File content/browser/download/download_item_impl.cc (right): > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > content/browser/download/download_item_impl.cc:206: : guid_(info.guid.empty() ? > base::ToUpperASCII(base::GenerateGUID()) > On 2017/05/23 16:56:13, xingliu wrote: > > On 2017/05/23 04:21:24, qinmin wrote: > > > will this download item be written to the history db? if so, why the GUID > > > generation logic is here instead of in the history db? > > > > I think this logic happens before writing to history db. Also previously we > > generated the uuid here. > > > > I guess the reason to generate uuid here is, the in-memory object still needs > a > > uuid before db persists. > > > > Sorry, what I am saying is that why not let DownloadItemImpl create the GUID, > and pass it back to the caller of DownloadDriver in onDownloadStarted()? Oh, the download service provides the API to let the consumers to specify a guid, in DownloadParams::guid, which can be passed to DownloadService::StartDownload. if they don't specify guid, we'll generate it for them.
On 2017/05/23 18:28:33, xingliu wrote: > On 2017/05/23 18:12:17, qinmin wrote: > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > File content/browser/download/download_item_impl.cc (right): > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > content/browser/download/download_item_impl.cc:206: : guid_(info.guid.empty() > ? > > base::ToUpperASCII(base::GenerateGUID()) > > On 2017/05/23 16:56:13, xingliu wrote: > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > will this download item be written to the history db? if so, why the GUID > > > > generation logic is here instead of in the history db? > > > > > > I think this logic happens before writing to history db. Also previously we > > > generated the uuid here. > > > > > > I guess the reason to generate uuid here is, the in-memory object still > needs > > a > > > uuid before db persists. > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create the GUID, > > and pass it back to the caller of DownloadDriver in onDownloadStarted()? > Oh, the download service provides the API to let the consumers to specify a > guid, > in DownloadParams::guid, which can be passed to DownloadService::StartDownload. > if they don't specify guid, we'll generate it for them. that is very strange, why let users provide a GUID before download even start. IDs are not unique, it could collide with internal ones. So logically it should be the download system to generate the GUID (Although it could still collide with existing ones), rather than the external API.
On 2017/05/23 18:34:53, qinmin wrote: > On 2017/05/23 18:28:33, xingliu wrote: > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > content/browser/download/download_item_impl.cc:206: : > guid_(info.guid.empty() > > ? > > > base::ToUpperASCII(base::GenerateGUID()) > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > will this download item be written to the history db? if so, why the > GUID > > > > > generation logic is here instead of in the history db? > > > > > > > > I think this logic happens before writing to history db. Also previously > we > > > > generated the uuid here. > > > > > > > > I guess the reason to generate uuid here is, the in-memory object still > > needs > > > a > > > > uuid before db persists. > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create the > GUID, > > > and pass it back to the caller of DownloadDriver in onDownloadStarted()? > > Oh, the download service provides the API to let the consumers to specify a > > guid, > > in DownloadParams::guid, which can be passed to > DownloadService::StartDownload. > > if they don't specify guid, we'll generate it for them. > > that is very strange, why let users provide a GUID before download even start. > IDs are not unique, it could collide with internal ones. > So logically it should be the download system to generate the GUID (Although it > could still collide with existing ones), rather than the external API. Download service can queue up downloads in memory and persists in level db before reaching content/download, so it needs an identifier. Added a TODO in DownloadService::Start, we can return an error code in the download driver layer when detecting guid collision. wdyt? The chance of collision of uuid should be extremely small.
On 2017/05/23 20:33:29, xingliu wrote: > On 2017/05/23 18:34:53, qinmin wrote: > > On 2017/05/23 18:28:33, xingliu wrote: > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > content/browser/download/download_item_impl.cc:206: : > > guid_(info.guid.empty() > > > ? > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > will this download item be written to the history db? if so, why the > > GUID > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > I think this logic happens before writing to history db. Also previously > > we > > > > > generated the uuid here. > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object still > > > needs > > > > a > > > > > uuid before db persists. > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create the > > GUID, > > > > and pass it back to the caller of DownloadDriver in onDownloadStarted()? > > > Oh, the download service provides the API to let the consumers to specify a > > > guid, > > > in DownloadParams::guid, which can be passed to > > DownloadService::StartDownload. > > > if they don't specify guid, we'll generate it for them. > > > > that is very strange, why let users provide a GUID before download even start. > > IDs are not unique, it could collide with internal ones. > > So logically it should be the download system to generate the GUID (Although > it > > could still collide with existing ones), rather than the external API. > > Download service can queue up downloads in memory and persists in level db > before reaching content/download, so it needs an identifier. > > Added a TODO in DownloadService::Start, we can return an error code in the > download driver layer when detecting guid collision. wdyt? > > > The chance of collision of uuid should be extremely small. I think it makes more sense for level db to provide a auto-increasing key. And when submitting a download to DownloadDriver, a GUID will be generated by download core. Level db can check this GUID to see if a download is successfully submitted to the download service. If not, it can reschedule the download
On 2017/05/23 20:42:18, qinmin wrote: > On 2017/05/23 20:33:29, xingliu wrote: > > On 2017/05/23 18:34:53, qinmin wrote: > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > content/browser/download/download_item_impl.cc:206: : > > > guid_(info.guid.empty() > > > > ? > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > will this download item be written to the history db? if so, why the > > > GUID > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > I think this logic happens before writing to history db. Also > previously > > > we > > > > > > generated the uuid here. > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object > still > > > > needs > > > > > a > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create the > > > GUID, > > > > > and pass it back to the caller of DownloadDriver in onDownloadStarted()? > > > > Oh, the download service provides the API to let the consumers to specify > a > > > > guid, > > > > in DownloadParams::guid, which can be passed to > > > DownloadService::StartDownload. > > > > if they don't specify guid, we'll generate it for them. > > > > > > that is very strange, why let users provide a GUID before download even > start. > > > IDs are not unique, it could collide with internal ones. > > > So logically it should be the download system to generate the GUID (Although > > it > > > could still collide with existing ones), rather than the external API. > > > > Download service can queue up downloads in memory and persists in level db > > before reaching content/download, so it needs an identifier. > > > > Added a TODO in DownloadService::Start, we can return an error code in the > > download driver layer when detecting guid collision. wdyt? > > > > > > The chance of collision of uuid should be extremely small. > > I think it makes more sense for level db to provide a auto-increasing key. > And when submitting a download to DownloadDriver, a GUID will be generated by > download core. > Level db can check this GUID to see if a download is successfully submitted to > the download service. If not, it can reschedule the download Level db can probably have guid as key for records, I'm fine with creating a mapping between guid of level db record and content/download guid. But is it a little bit cumbersome to maintain this kind of mapping?
On 2017/05/23 20:51:02, xingliu wrote: > On 2017/05/23 20:42:18, qinmin wrote: > > On 2017/05/23 20:33:29, xingliu wrote: > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > guid_(info.guid.empty() > > > > > ? > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > will this download item be written to the history db? if so, why > the > > > > GUID > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > previously > > > > we > > > > > > > generated the uuid here. > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object > > still > > > > > needs > > > > > > a > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create > the > > > > GUID, > > > > > > and pass it back to the caller of DownloadDriver in > onDownloadStarted()? > > > > > Oh, the download service provides the API to let the consumers to > specify > > a > > > > > guid, > > > > > in DownloadParams::guid, which can be passed to > > > > DownloadService::StartDownload. > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > that is very strange, why let users provide a GUID before download even > > start. > > > > IDs are not unique, it could collide with internal ones. > > > > So logically it should be the download system to generate the GUID > (Although > > > it > > > > could still collide with existing ones), rather than the external API. > > > > > > Download service can queue up downloads in memory and persists in level db > > > before reaching content/download, so it needs an identifier. > > > > > > Added a TODO in DownloadService::Start, we can return an error code in the > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > I think it makes more sense for level db to provide a auto-increasing key. > > And when submitting a download to DownloadDriver, a GUID will be generated by > > download core. > > Level db can check this GUID to see if a download is successfully submitted to > > the download service. If not, it can reschedule the download > > Level db can probably have guid as key for records, I'm fine with creating a > mapping between guid of level db record and content/download guid. > But is it a little bit cumbersome to maintain this kind of mapping? why a GUID key? only an integer is fine, right? And I don't understand why you need the mapping. When a download is queued up in level db, we just pick all those ones that has no GUID to submit to the download service. The integer key is just for uniquely identifying the records (sorting), but doesn't necessarily need to be used.
On 2017/05/23 20:51:02, xingliu wrote: > On 2017/05/23 20:42:18, qinmin wrote: > > On 2017/05/23 20:33:29, xingliu wrote: > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > guid_(info.guid.empty() > > > > > ? > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > will this download item be written to the history db? if so, why > the > > > > GUID > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > previously > > > > we > > > > > > > generated the uuid here. > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object > > still > > > > > needs > > > > > > a > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create > the > > > > GUID, > > > > > > and pass it back to the caller of DownloadDriver in > onDownloadStarted()? > > > > > Oh, the download service provides the API to let the consumers to > specify > > a > > > > > guid, > > > > > in DownloadParams::guid, which can be passed to > > > > DownloadService::StartDownload. > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > that is very strange, why let users provide a GUID before download even > > start. > > > > IDs are not unique, it could collide with internal ones. > > > > So logically it should be the download system to generate the GUID > (Although > > > it > > > > could still collide with existing ones), rather than the external API. > > > > > > Download service can queue up downloads in memory and persists in level db > > > before reaching content/download, so it needs an identifier. > > > > > > Added a TODO in DownloadService::Start, we can return an error code in the > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > I think it makes more sense for level db to provide a auto-increasing key. > > And when submitting a download to DownloadDriver, a GUID will be generated by > > download core. > > Level db can check this GUID to see if a download is successfully submitted to > > the download service. If not, it can reschedule the download > > Level db can probably have guid as key for records, I'm fine with creating a > mapping between guid of level db record and content/download guid. > But is it a little bit cumbersome to maintain this kind of mapping? why a GUID key? only an integer is fine, right? And I don't understand why you need the mapping. When a download is queued up in level db, we just pick all those ones that has no GUID to submit to the download service. The integer key is just for uniquely identifying the records (sorting), but doesn't necessarily need to be used.
On 2017/05/23 21:01:18, qinmin wrote: > On 2017/05/23 20:51:02, xingliu wrote: > > On 2017/05/23 20:42:18, qinmin wrote: > > > On 2017/05/23 20:33:29, xingliu wrote: > > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > > guid_(info.guid.empty() > > > > > > ? > > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > > will this download item be written to the history db? if so, why > > the > > > > > GUID > > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > > previously > > > > > we > > > > > > > > generated the uuid here. > > > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object > > > still > > > > > > needs > > > > > > > a > > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create > > the > > > > > GUID, > > > > > > > and pass it back to the caller of DownloadDriver in > > onDownloadStarted()? > > > > > > Oh, the download service provides the API to let the consumers to > > specify > > > a > > > > > > guid, > > > > > > in DownloadParams::guid, which can be passed to > > > > > DownloadService::StartDownload. > > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > > > that is very strange, why let users provide a GUID before download even > > > start. > > > > > IDs are not unique, it could collide with internal ones. > > > > > So logically it should be the download system to generate the GUID > > (Although > > > > it > > > > > could still collide with existing ones), rather than the external API. > > > > > > > > Download service can queue up downloads in memory and persists in level db > > > > before reaching content/download, so it needs an identifier. > > > > > > > > Added a TODO in DownloadService::Start, we can return an error code in the > > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > > > I think it makes more sense for level db to provide a auto-increasing key. > > > And when submitting a download to DownloadDriver, a GUID will be generated > by > > > download core. > > > Level db can check this GUID to see if a download is successfully submitted > to > > > the download service. If not, it can reschedule the download > > > > Level db can probably have guid as key for records, I'm fine with creating a > > mapping between guid of level db record and content/download guid. > > But is it a little bit cumbersome to maintain this kind of mapping? > > why a GUID key? only an integer is fine, right? And I don't understand why you > need the mapping. When a download is queued up in level db, we just pick all > those ones that has no GUID to submit to the download service. The integer key > is just for uniquely identifying the records (sorting), but doesn't necessarily > need to be used. We need the caller to specify the id for recovery reasons. Otherwise we could be in a situation where we start a download but don't have any knowledge about it. This can happen anywhere in the chain like Client -> Service -> Manager. Looking at the chain from the Service to the Manager, this is a possible failure scenario that would result in a dangling download entry: 1. Service calls Manager::StartDownload. 2. Manager saves the download to the DB and calls back to the Service with the guid. 3. Process crashes before Service can persist the guid. Now when we restart we have an entry that the Service doesn't know about and isn't aware started.
On 2017/05/23 21:01:18, qinmin wrote: > On 2017/05/23 20:51:02, xingliu wrote: > > On 2017/05/23 20:42:18, qinmin wrote: > > > On 2017/05/23 20:33:29, xingliu wrote: > > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > > guid_(info.guid.empty() > > > > > > ? > > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > > will this download item be written to the history db? if so, why > > the > > > > > GUID > > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > > previously > > > > > we > > > > > > > > generated the uuid here. > > > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory object > > > still > > > > > > needs > > > > > > > a > > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl create > > the > > > > > GUID, > > > > > > > and pass it back to the caller of DownloadDriver in > > onDownloadStarted()? > > > > > > Oh, the download service provides the API to let the consumers to > > specify > > > a > > > > > > guid, > > > > > > in DownloadParams::guid, which can be passed to > > > > > DownloadService::StartDownload. > > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > > > that is very strange, why let users provide a GUID before download even > > > start. > > > > > IDs are not unique, it could collide with internal ones. > > > > > So logically it should be the download system to generate the GUID > > (Although > > > > it > > > > > could still collide with existing ones), rather than the external API. > > > > > > > > Download service can queue up downloads in memory and persists in level db > > > > before reaching content/download, so it needs an identifier. > > > > > > > > Added a TODO in DownloadService::Start, we can return an error code in the > > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > > > I think it makes more sense for level db to provide a auto-increasing key. > > > And when submitting a download to DownloadDriver, a GUID will be generated > by > > > download core. > > > Level db can check this GUID to see if a download is successfully submitted > to > > > the download service. If not, it can reschedule the download > > > > Level db can probably have guid as key for records, I'm fine with creating a > > mapping between guid of level db record and content/download guid. > > But is it a little bit cumbersome to maintain this kind of mapping? > > why a GUID key? only an integer is fine, right? And I don't understand why you > need the mapping. When a download is queued up in level db, we just pick all > those ones that has no GUID to submit to the download service. The integer key > is just for uniquely identifying the records (sorting), but doesn't necessarily > need to be used. If we use incremental integer as key in level db, then we probably need to build index or in-memory map on guid to accelerate lookup. If every download have a unique guid, then why not we use this as the key in a key-value store.
On 2017/05/23 21:14:45, David Trainor-ping if over 24h wrote: > On 2017/05/23 21:01:18, qinmin wrote: > > On 2017/05/23 20:51:02, xingliu wrote: > > > On 2017/05/23 20:42:18, qinmin wrote: > > > > On 2017/05/23 20:33:29, xingliu wrote: > > > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > > > guid_(info.guid.empty() > > > > > > > ? > > > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > > > will this download item be written to the history db? if so, > why > > > the > > > > > > GUID > > > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > > > previously > > > > > > we > > > > > > > > > generated the uuid here. > > > > > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory > object > > > > still > > > > > > > needs > > > > > > > > a > > > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl > create > > > the > > > > > > GUID, > > > > > > > > and pass it back to the caller of DownloadDriver in > > > onDownloadStarted()? > > > > > > > Oh, the download service provides the API to let the consumers to > > > specify > > > > a > > > > > > > guid, > > > > > > > in DownloadParams::guid, which can be passed to > > > > > > DownloadService::StartDownload. > > > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > > > > > that is very strange, why let users provide a GUID before download > even > > > > start. > > > > > > IDs are not unique, it could collide with internal ones. > > > > > > So logically it should be the download system to generate the GUID > > > (Although > > > > > it > > > > > > could still collide with existing ones), rather than the external API. > > > > > > > > > > Download service can queue up downloads in memory and persists in level > db > > > > > before reaching content/download, so it needs an identifier. > > > > > > > > > > Added a TODO in DownloadService::Start, we can return an error code in > the > > > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > > > > > I think it makes more sense for level db to provide a auto-increasing key. > > > > > And when submitting a download to DownloadDriver, a GUID will be generated > > by > > > > download core. > > > > Level db can check this GUID to see if a download is successfully > submitted > > to > > > > the download service. If not, it can reschedule the download > > > > > > Level db can probably have guid as key for records, I'm fine with creating a > > > mapping between guid of level db record and content/download guid. > > > But is it a little bit cumbersome to maintain this kind of mapping? > > > > why a GUID key? only an integer is fine, right? And I don't understand why you > > need the mapping. When a download is queued up in level db, we just pick all > > those ones that has no GUID to submit to the download service. The integer key > > is just for uniquely identifying the records (sorting), but doesn't > necessarily > > need to be used. > > We need the caller to specify the id for recovery reasons. Otherwise we could > be in a situation where we start a download but don't have any knowledge about > it. This can happen anywhere in the chain like Client -> Service -> Manager. > Looking at the chain from the Service to the Manager, this is a possible failure > scenario that would result in a dangling download entry: > > 1. Service calls Manager::StartDownload. > 2. Manager saves the download to the DB and calls back to the Service with the > guid. > 3. Process crashes before Service can persist the guid. > > Now when we restart we have an entry that the Service doesn't know about and > isn't aware started. So if this is an issue, then the current CL will cause another issue: 1. Service calls Manager::StartDownload by giving it a GUID 2. Manager saves the download to the DB and calls back to the service. 3. Process crash. Service is restarted and resend the same GUID to the Manager again. Which causes a duplicate key issue. I don't see that download_driver_impl.cc will check whether the GUID already exists in download database.
On 2017/05/23 21:23:31, qinmin wrote: > On 2017/05/23 21:14:45, David Trainor-ping if over 24h wrote: > > On 2017/05/23 21:01:18, qinmin wrote: > > > On 2017/05/23 20:51:02, xingliu wrote: > > > > On 2017/05/23 20:42:18, qinmin wrote: > > > > > On 2017/05/23 20:33:29, xingliu wrote: > > > > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > > > > guid_(info.guid.empty() > > > > > > > > ? > > > > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > > > > will this download item be written to the history db? if so, > > why > > > > the > > > > > > > GUID > > > > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > > > > > > > I think this logic happens before writing to history db. Also > > > > > previously > > > > > > > we > > > > > > > > > > generated the uuid here. > > > > > > > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory > > object > > > > > still > > > > > > > > needs > > > > > > > > > a > > > > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl > > create > > > > the > > > > > > > GUID, > > > > > > > > > and pass it back to the caller of DownloadDriver in > > > > onDownloadStarted()? > > > > > > > > Oh, the download service provides the API to let the consumers to > > > > specify > > > > > a > > > > > > > > guid, > > > > > > > > in DownloadParams::guid, which can be passed to > > > > > > > DownloadService::StartDownload. > > > > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > > > > > > > that is very strange, why let users provide a GUID before download > > even > > > > > start. > > > > > > > IDs are not unique, it could collide with internal ones. > > > > > > > So logically it should be the download system to generate the GUID > > > > (Although > > > > > > it > > > > > > > could still collide with existing ones), rather than the external > API. > > > > > > > > > > > > Download service can queue up downloads in memory and persists in > level > > db > > > > > > before reaching content/download, so it needs an identifier. > > > > > > > > > > > > Added a TODO in DownloadService::Start, we can return an error code in > > the > > > > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > > > > > > > I think it makes more sense for level db to provide a auto-increasing > key. > > > > > > > And when submitting a download to DownloadDriver, a GUID will be > generated > > > by > > > > > download core. > > > > > Level db can check this GUID to see if a download is successfully > > submitted > > > to > > > > > the download service. If not, it can reschedule the download > > > > > > > > Level db can probably have guid as key for records, I'm fine with creating > a > > > > mapping between guid of level db record and content/download guid. > > > > But is it a little bit cumbersome to maintain this kind of mapping? > > > > > > why a GUID key? only an integer is fine, right? And I don't understand why > you > > > need the mapping. When a download is queued up in level db, we just pick all > > > those ones that has no GUID to submit to the download service. The integer > key > > > is just for uniquely identifying the records (sorting), but doesn't > > necessarily > > > need to be used. > > > > We need the caller to specify the id for recovery reasons. Otherwise we could > > be in a situation where we start a download but don't have any knowledge about > > it. This can happen anywhere in the chain like Client -> Service -> Manager. > > Looking at the chain from the Service to the Manager, this is a possible > failure > > scenario that would result in a dangling download entry: > > > > 1. Service calls Manager::StartDownload. > > 2. Manager saves the download to the DB and calls back to the Service with the > > guid. > > 3. Process crashes before Service can persist the guid. > > > > Now when we restart we have an entry that the Service doesn't know about and > > isn't aware started. > > So if this is an issue, then the current CL will cause another issue: > 1. Service calls Manager::StartDownload by giving it a GUID > 2. Manager saves the download to the DB and calls back to the service. > 3. Process crash. Service is restarted and resend the same GUID to the Manager > again. Which causes a duplicate key issue. That's not true. We can query which downloads started and correlate the GUIDs. We can't in the alternate approach... > > I don't see that download_driver_impl.cc will check whether the GUID already > exists in download database. Actually we should make this change to download_manager. Even if base::GenerateGUID() *somehow* returns a duplicate, we should just fail the download cleanly IMO :).
On 2017/05/23 21:26:40, David Trainor-ping if over 24h wrote: > On 2017/05/23 21:23:31, qinmin wrote: > > On 2017/05/23 21:14:45, David Trainor-ping if over 24h wrote: > > > On 2017/05/23 21:01:18, qinmin wrote: > > > > On 2017/05/23 20:51:02, xingliu wrote: > > > > > On 2017/05/23 20:42:18, qinmin wrote: > > > > > > On 2017/05/23 20:33:29, xingliu wrote: > > > > > > > On 2017/05/23 18:34:53, qinmin wrote: > > > > > > > > On 2017/05/23 18:28:33, xingliu wrote: > > > > > > > > > On 2017/05/23 18:12:17, qinmin wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2880933002/diff/120001/content/browser/downlo... > > > > > > > > > > content/browser/download/download_item_impl.cc:206: : > > > > > > > > guid_(info.guid.empty() > > > > > > > > > ? > > > > > > > > > > base::ToUpperASCII(base::GenerateGUID()) > > > > > > > > > > On 2017/05/23 16:56:13, xingliu wrote: > > > > > > > > > > > On 2017/05/23 04:21:24, qinmin wrote: > > > > > > > > > > > > will this download item be written to the history db? if > so, > > > why > > > > > the > > > > > > > > GUID > > > > > > > > > > > > generation logic is here instead of in the history db? > > > > > > > > > > > > > > > > > > > > > > I think this logic happens before writing to history db. > Also > > > > > > previously > > > > > > > > we > > > > > > > > > > > generated the uuid here. > > > > > > > > > > > > > > > > > > > > > > I guess the reason to generate uuid here is, the in-memory > > > object > > > > > > still > > > > > > > > > needs > > > > > > > > > > a > > > > > > > > > > > uuid before db persists. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry, what I am saying is that why not let DownloadItemImpl > > > create > > > > > the > > > > > > > > GUID, > > > > > > > > > > and pass it back to the caller of DownloadDriver in > > > > > onDownloadStarted()? > > > > > > > > > Oh, the download service provides the API to let the consumers > to > > > > > specify > > > > > > a > > > > > > > > > guid, > > > > > > > > > in DownloadParams::guid, which can be passed to > > > > > > > > DownloadService::StartDownload. > > > > > > > > > if they don't specify guid, we'll generate it for them. > > > > > > > > > > > > > > > > that is very strange, why let users provide a GUID before download > > > even > > > > > > start. > > > > > > > > IDs are not unique, it could collide with internal ones. > > > > > > > > So logically it should be the download system to generate the GUID > > > > > (Although > > > > > > > it > > > > > > > > could still collide with existing ones), rather than the external > > API. > > > > > > > > > > > > > > Download service can queue up downloads in memory and persists in > > level > > > db > > > > > > > before reaching content/download, so it needs an identifier. > > > > > > > > > > > > > > Added a TODO in DownloadService::Start, we can return an error code > in > > > the > > > > > > > download driver layer when detecting guid collision. wdyt? > > > > > > > > > > > > > > > > > > > > > The chance of collision of uuid should be extremely small. > > > > > > > > > > > > I think it makes more sense for level db to provide a auto-increasing > > key. > > > > > > > > > And when submitting a download to DownloadDriver, a GUID will be > > generated > > > > by > > > > > > download core. > > > > > > Level db can check this GUID to see if a download is successfully > > > submitted > > > > to > > > > > > the download service. If not, it can reschedule the download > > > > > > > > > > Level db can probably have guid as key for records, I'm fine with > creating > > a > > > > > mapping between guid of level db record and content/download guid. > > > > > But is it a little bit cumbersome to maintain this kind of mapping? > > > > > > > > why a GUID key? only an integer is fine, right? And I don't understand why > > you > > > > need the mapping. When a download is queued up in level db, we just pick > all > > > > those ones that has no GUID to submit to the download service. The integer > > key > > > > is just for uniquely identifying the records (sorting), but doesn't > > > necessarily > > > > need to be used. > > > > > > We need the caller to specify the id for recovery reasons. Otherwise we > could > > > be in a situation where we start a download but don't have any knowledge > about > > > it. This can happen anywhere in the chain like Client -> Service -> > Manager. > > > Looking at the chain from the Service to the Manager, this is a possible > > failure > > > scenario that would result in a dangling download entry: > > > > > > 1. Service calls Manager::StartDownload. > > > 2. Manager saves the download to the DB and calls back to the Service with > the > > > guid. > > > 3. Process crashes before Service can persist the guid. > > > > > > Now when we restart we have an entry that the Service doesn't know about and > > > isn't aware started. > > > > So if this is an issue, then the current CL will cause another issue: > > 1. Service calls Manager::StartDownload by giving it a GUID > > 2. Manager saves the download to the DB and calls back to the service. > > 3. Process crash. Service is restarted and resend the same GUID to the Manager > > again. Which causes a duplicate key issue. > > That's not true. We can query which downloads started and correlate the GUIDs. > We can't in the alternate approach... > > > > > I don't see that download_driver_impl.cc will check whether the GUID already > > exists in download database. > > Actually we should make this change to download_manager. Even if > base::GenerateGUID() *somehow* returns a duplicate, we should just fail the > download cleanly IMO :). Ok, this sounds good to me. Please add a TODO to mention that before calling start(), Chrome will check for the same GUID and resume the download if the download already exists.
Thanks for the review, added a TODO for guid collision.
On 2017/05/23 21:52:12, xingliu wrote: > Thanks for the review, added a TODO for guid collision. lgtm
The CQ bit was checked by xingliu@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, nick@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2880933002/#ps180001 (title: "Polish comment.")
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": 180001, "attempt_start_ts": 1495594477680710,
"parent_rev": "d2c9e5e3d8e2d42c074ba3e6aea676321c6895ca", "commit_rev":
"d64fde65aa583828811a43bddb897726590a403a"}
Message was sent while issue was closed.
Description was changed from ========== Download driver for components/download. This CL introduces the glue layer between //components/download and //content. The DownloadDriver interface will be the only place to access content functionalities from download service. Also provides a test support class for //components/download code. The test support version of DownloadManager is not done in this CL. The response/request headers pumping is not done in this CL, which requires tweak on download network code. BUG=717180 ========== to ========== Download driver for components/download. This CL introduces the glue layer between //components/download and //content. The DownloadDriver interface will be the only place to access content functionalities from download service. Also provides a test support class for //components/download code. The test support version of DownloadManager is not done in this CL. The response/request headers pumping is not done in this CL, which requires tweak on download network code. BUG=717180 Review-Url: https://codereview.chromium.org/2880933002 Cr-Commit-Position: refs/heads/master@{#474191} Committed: https://chromium.googlesource.com/chromium/src/+/d64fde65aa583828811a43bddb89... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/d64fde65aa583828811a43bddb89... |
