New issue
Advanced search Search tips

Issue 734818 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug


Participants' hotlists:
Downloads-Framework-Service


Sign in to add a comment

[Download Service] guid compatibility with content download

Project Member Reported by xingliu@chromium.org, Jun 19 2017

Issue description

Currently content download code only supports upper case guid, where we do DCHECK in download manager.

This creates an issue that we need to convert upper/lower case when transferring guid between content and download service.

So either we enforce the client to pass in upper case guid, or we converts everything to the same case in download service.
 
After some discussion, a better solution would be decoupling guid from content layer. Use two guid, and build the mapping between them in model layer.

Status: Available (was: Untriaged)
In the very short term, we plan to limit all guid in download service to use upper case ascii chars.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 20 2017

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

commit 95d8be7f74506f6517885ab3a4571edfcecbc764
Author: Xing Liu <xingliu@chromium.org>
Date: Tue Jun 20 23:09:09 2017

Fixed the upper guid issue in driver.

Currently content download only supports upper case guid. This CL
limits guid in download service to be upper case ascii chars.

In the future, we should consider the two guid approach to decouple
from content layer guid details.



Bug:  734818 
Change-Id: Id03a234c0edb14c9ee7a66d5bb486cb1503e1840
Reviewed-on: https://chromium-review.googlesource.com/540679
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481004}
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/internal/controller_impl.cc
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/internal/download_service_impl.cc
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/internal/download_service_impl_unittest.cc
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/internal/scheduler/scheduler_impl.cc
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/public/download_params.h
[modify] https://crrev.com/95d8be7f74506f6517885ab3a4571edfcecbc764/components/download/public/download_service.h

Summary: [Download Service] guid compatibility with content download (was: download service guid compatibility with content download)
Labels: M-62
Status: Started (was: Available)
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 15 2017

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

commit 02d83f92c6a49683998ba011880fe8601d67e6df
Author: Xing Liu <xingliu@chromium.org>
Date: Tue Aug 15 21:38:02 2017

Remove the limitation of upper case guid in download.

Chrome's guid used to be in upper case, and in content/download, we
also limit all guid to be upper case. However base::GenerateGUID is
changed to generate lower case string at some point.

The guid(uuid) standard is here, download guid uses version 4.
https://tools.ietf.org/html/rfc4122
Most of uuid generators do lower case.

This CL removes the limitation, but didn't put new limitation on
download guid, since we need to consider existing upper case guids in
users' database.

Bug:  734818 
Change-Id: I618865203b6a77e8814836324fba00d1fea46937
Reviewed-on: https://chromium-review.googlesource.com/606673
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Michael Nordman <michaeln@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494557}
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/chrome/browser/browsing_data/downloads_counter_browsertest.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/download/internal/download_service_impl.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/download/internal/download_service_impl_unittest.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/download/public/download_params.h
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/download/public/download_service.h
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/history/core/browser/history_backend_db_unittest.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/components/offline_pages/core/prefetch/prefetch_downloader_impl.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/content/browser/download/download_item_impl.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/content/browser/download/download_item_impl.h
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/content/browser/download/download_manager_impl.cc
[modify] https://crrev.com/02d83f92c6a49683998ba011880fe8601d67e6df/content/browser/download/download_manager_impl_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment