Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9585009:
Add URLFetcher::SaveResponseToFileAtPath (Closed)
|
Created:
8 years, 9 months ago by hashimoto Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org, zel, kinaba Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd URLFetcher::SaveResponseToFileAtPath
BUG=chromium-os:26971
TEST=content_unittests --gtest_filter="URLFetcher*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125937
Patch Set 1 : _ #Patch Set 2 : _ #
Total comments: 15
Patch Set 3 : Rename SaveResponseToFile to SaveResponseToFileAtPath #
Total comments: 4
Patch Set 4 : Fixed nits #
Total comments: 8
Patch Set 5 : Fixed nits #
Messages
Total messages: 27 (0 generated)
willchan: Could you review this CL as the primary reviewer? skerner: Could you review this CL as the original author of SaveResponseToTemporaryFile?
Should this be using DownloadManager::DownloadURL (or ultimately ResourceDispatcherHost::BeginDownload) instead?
Not the URLFetcher itself - but the ultimate users of SaveResponseToFile. On Mon, Mar 5, 2012 at 9:59 AM, <cbentzel@chromium.org> wrote: > Should this be using DownloadManager::DownloadURL (or ultimately > ResourceDispatcherHost::**BeginDownload) instead? > > > http://codereview.chromium.**org/9585009/<http://codereview.chromium.org/9585... >
+kuan
Chris, the "customer" for this change is our gdata library layer. The approach that you proposed is not going to work well since the context for content download operation is very different here (part of File API implementation for remote file systems). Also, the calls made through this layer use OAuth2-based authentications schema that's scoped for this web service - token driven, no cookie jar, has build in token refresh logic... On Mon, Mar 5, 2012 at 7:01 AM, Chris Bentzel <cbentzel@chromium.org> wrote: > Not the URLFetcher itself - but the ultimate users of SaveResponseToFile. > > > On Mon, Mar 5, 2012 at 9:59 AM, <cbentzel@chromium.org> wrote: > >> Should this be using DownloadManager::DownloadURL (or ultimately >> ResourceDispatcherHost::**BeginDownload) instead? >> >> >> http://codereview.chromium.**org/9585009/<http://codereview.chromium.org/9585... >> > >
Great - thanks for the response. On Mon, Mar 5, 2012 at 12:44 PM, Zelidrag Hornung <zelidrag@chromium.org>wrote: > Chris, the "customer" for this change is our gdata library layer. The > approach that you proposed is not going to work well since the context > for content download operation is very different here (part of File API > implementation for remote file systems). Also, the calls made through this > layer use OAuth2-based authentications schema that's scoped for this web > service - token driven, no cookie jar, has build in token refresh logic... > > > > On Mon, Mar 5, 2012 at 7:01 AM, Chris Bentzel <cbentzel@chromium.org>wrote: > >> Not the URLFetcher itself - but the ultimate users of SaveResponseToFile. >> >> >> On Mon, Mar 5, 2012 at 9:59 AM, <cbentzel@chromium.org> wrote: >> >>> Should this be using DownloadManager::DownloadURL (or ultimately >>> ResourceDispatcherHost::**BeginDownload) instead? >>> >>> >>> http://codereview.chromium.**org/9585009/<http://codereview.chromium.org/9585... >>> >> >> >
cbentzel and willchan are more qualified than I am to decide if this should be part of URLFetcher. I assumed that it belongs here in my review, but I want to hear from them about the right place for this functionality. http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:625: if (!response_destination_file_path_.empty()) Looks like the signal that you want a specific file (and not a temp file) is the emptiness of response_destination_file_path_ . This if block is clear today, but I worry that as we make the code more complex, this will no longer be easy to understand. Could you make response_destination_'s type one of { STRING, TEMP_FILE, PERMANENT_FILE}? This would make the differences between temp and permanent paths easy to see. http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:1068: void URLFetcherImpl::SaveResponseToFile( The name of this method should distinguish it from saving to a temp path. How about SaveResponseToFileAtPath(const FilePath& ...) ? http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:1072: core_->response_destination_ = FILE; Here is another example where adding a new enum value would enhance clarity. http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... File content/public/common/url_fetcher.h (right): http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... content/public/common/url_fetcher.h:173: // |file_message_loop_proxy| will be used for all file operations. Add something like: // To save to a temporary path, use SaveResponseToTemporaryFile(). http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... content/public/common/url_fetcher.h:240: virtual bool GetResponseAsFilePath(bool take_ownership, One tricky aspect of fetching a file is deleting it in error cases. The current logic is designed for the typical user of a temp file. We should think about how it ought to work for writing to a path. For temp files, the file is owned by the URLFetcher (and will be deleted on error) until the following method is called: GetResponseAsFilePath(..., true); At that point, the URLFetcher does not own the file, and will not write to it or delete it. If this method is not called, the temp file gets removed when the URLFetcher is destructed. The simplest thing you could do is to use the same semantics for files downloaded to a path, but you need to point this out because it might be surprising that the file gets deleted when the URLFetcher is destroyed. A comment above the prototype of SaveResponseToFile(), pointing out that a user must take ownership to avoid the removal of the file. A comment on the delegate base class URLFetcherDelegate would also be a good thing.
http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... File content/common/net/url_fetcher_impl.h (right): http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.h:60: scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy) OVERRIDE; can u add stop_caching boolean as a parameter? otherwise, we'll need to add one more method to the interface just to stop caching.
http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:357: base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_WRITE, Just doublechecking...is this what we want? We always create the file even if it already exists? Sounds potentially dangerous. http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... File content/common/net/url_fetcher_impl.h (right): http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.h:60: scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy) OVERRIDE; On 2012/03/06 02:01:33, kuan wrote: > can u add stop_caching boolean as a parameter? > otherwise, we'll need to add one more method to the interface just to stop > caching. What caching are you talking about? Browser caching? There's already a SetLoadFlags() interface, and there is a net::LoadFlag to control caching.
http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:357: base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_WRITE, On 2012/03/06 02:33:04, willchan wrote: > Just doublechecking...is this what we want? We always create the file even if it > already exists? Sounds potentially dangerous. I think this is OK because zip::ZipReader is doing the same thing. But if this is not an encouraged thing to do in code under content/, I would like to 1. Always make error when the file already exists, instead of always overwriting it, or 2. Add |file_flags| argument to SaveResponseToFile to allow user code to determine the behavior. What do you think? http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:625: if (!response_destination_file_path_.empty()) On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > Looks like the signal that you want a specific file (and not a temp file) is the > emptiness of response_destination_file_path_ . This if block is clear today, > but I worry that as we make the code more complex, this will no longer be easy > to understand. > > Could you make response_destination_'s type one of { STRING, TEMP_FILE, > PERMANENT_FILE}? This would make the differences between temp and permanent > paths easy to see. Sounds good, done. http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:1068: void URLFetcherImpl::SaveResponseToFile( On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > The name of this method should distinguish it from saving to a temp path. How > about SaveResponseToFileAtPath(const FilePath& ...) ? Sounds good, done. http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... content/common/net/url_fetcher_impl.cc:1072: core_->response_destination_ = FILE; On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > Here is another example where adding a new enum value would enhance clarity. Done. http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... File content/public/common/url_fetcher.h (right): http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... content/public/common/url_fetcher.h:173: // |file_message_loop_proxy| will be used for all file operations. On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > Add something like: > // To save to a temporary path, use SaveResponseToTemporaryFile(). Done. http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... content/public/common/url_fetcher.h:240: virtual bool GetResponseAsFilePath(bool take_ownership, On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > One tricky aspect of fetching a file is deleting it in error cases. The current > logic is designed for the typical user of a temp file. We should think about > how it ought to work for writing to a path. > > > For temp files, the file is owned by the URLFetcher (and will be deleted on > error) until the following method is called: > > GetResponseAsFilePath(..., true); > > At that point, the URLFetcher does not own the file, and will not write to it or > delete it. > > If this method is not called, the temp file gets removed when the URLFetcher is > destructed. > > > The simplest thing you could do is to use the same semantics for files > downloaded to a path, but you need to point this out because it might be > surprising that the file gets deleted when the URLFetcher is destroyed. A > comment above the prototype of SaveResponseToFile(), pointing out that a user > must take ownership to avoid the removal of the file. A comment on the delegate > base class URLFetcherDelegate would also be a good thing. Good point, added comments for SaveResponseToFileAtPath and SaveResponseToTemporaryFile. BTW, it looks like all of GetResponseAsFilePath's users are passing true as |take_ownership| now. (http://code.google.com/p/chromium/source/search?q=GetResponseAsFilePath) Can we remove this argument? I think it's good to remove this argument and switch to a simpler way where the file is removed only when a file error occurs. How about doing this in a following patch?
On 2012/03/06 02:33:03, willchan wrote: > http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... > File content/common/net/url_fetcher_impl.cc (right): > > http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... > content/common/net/url_fetcher_impl.cc:357: base::PLATFORM_FILE_CREATE_ALWAYS | > base::PLATFORM_FILE_WRITE, > Just doublechecking...is this what we want? We always create the file even if it > already exists? Sounds potentially dangerous. > > http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... > File content/common/net/url_fetcher_impl.h (right): > > http://codereview.chromium.org/9585009/diff/6009/content/common/net/url_fetch... > content/common/net/url_fetcher_impl.h:60: scoped_refptr<base::MessageLoopProxy> > file_message_loop_proxy) OVERRIDE; > On 2012/03/06 02:01:33, kuan wrote: > > can u add stop_caching boolean as a parameter? > > otherwise, we'll need to add one more method to the interface just to stop > > caching. > > What caching are you talking about? Browser caching? There's already a > SetLoadFlags() interface, and there is a net::LoadFlag to control caching. yes, i'm referring to browser caching. will the flag be LOAD_DISABLE_CACHE? pardon my ignorance - i was looking at download manager code which uses StopCaching, so i thought that's the only way to not have 2 copies of the same contents in cache and in specified file path.
Two nits, otherwise it looks good. Because this code has had some gnarly bugs, I want to give another reviewer a chance to chime in before approving. http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... File content/public/common/url_fetcher.h (right): http://codereview.chromium.org/9585009/diff/6009/content/public/common/url_fe... content/public/common/url_fetcher.h:240: virtual bool GetResponseAsFilePath(bool take_ownership, On 2012/03/06 10:16:01, hashimoto wrote: > On 2012/03/06 00:23:06, Sam Kerner (Chrome) wrote: > > One tricky aspect of fetching a file is deleting it in error cases. The > current > > logic is designed for the typical user of a temp file. We should think about > > how it ought to work for writing to a path. > > > > > > For temp files, the file is owned by the URLFetcher (and will be deleted on > > error) until the following method is called: > > > > GetResponseAsFilePath(..., true); > > > > At that point, the URLFetcher does not own the file, and will not write to it > or > > delete it. > > > > If this method is not called, the temp file gets removed when the URLFetcher > is > > destructed. > > > > > > The simplest thing you could do is to use the same semantics for files > > downloaded to a path, but you need to point this out because it might be > > surprising that the file gets deleted when the URLFetcher is destroyed. A > > comment above the prototype of SaveResponseToFile(), pointing out that a user > > must take ownership to avoid the removal of the file. A comment on the > delegate > > base class URLFetcherDelegate would also be a good thing. > > Good point, added comments for SaveResponseToFileAtPath and > SaveResponseToTemporaryFile. > > BTW, it looks like all of GetResponseAsFilePath's users are passing true as > |take_ownership| now. > (http://code.google.com/p/chromium/source/search?q=GetResponseAsFilePath) > Can we remove this argument? > I think it's good to remove this argument and switch to a simpler way where the > file is removed only when a file error occurs. > How about doing this in a following patch? Sounds good to me. When I created his method, the extensions updater code needed to call it without taking ownership. I see the code has been cleaned up and no longer does this. http://codereview.chromium.org/9585009/diff/15001/content/common/net/url_fetc... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/15001/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:401: const bool created = true; Should be kCreated. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names http://codereview.chromium.org/9585009/diff/15001/content/public/common/url_f... File content/public/common/url_fetcher.h (right): http://codereview.chromium.org/9585009/diff/15001/content/public/common/url_f... content/public/common/url_fetcher.h:176: // take the ownership. Please say how you take ownership: "take ownership by calling GetResponseAsFilePath()."
http://codereview.chromium.org/9585009/diff/15001/content/common/net/url_fetc... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/15001/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:401: const bool created = true; On 2012/03/06 19:44:31, Sam Kerner (Chrome) wrote: > Should be kCreated. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names Done. http://codereview.chromium.org/9585009/diff/15001/content/public/common/url_f... File content/public/common/url_fetcher.h (right): http://codereview.chromium.org/9585009/diff/15001/content/public/common/url_f... content/public/common/url_fetcher.h:176: // take the ownership. On 2012/03/06 19:44:31, Sam Kerner (Chrome) wrote: > Please say how you take ownership: "take ownership by calling > GetResponseAsFilePath()." Done.
wtc, could you review this change as a content/public owner?
wtc isn't in content/public/OWNERS. let me add jam.
adding jam for content/public changes.
On 2012/03/07 18:13:44, satorux1 wrote: > adding jam for content/public changes. I'll defer to wtc or willchan on the content changes, since they're the URLFetcherImpl owners (see content/common/net/OWNERS). once they're fine you have my lgtm
On 2012/03/07 20:13:34, John Abd-El-Malek wrote: > On 2012/03/07 18:13:44, satorux1 wrote: > > adding jam for content/public changes. > > I'll defer to wtc or willchan on the content changes, since they're the > URLFetcherImpl owners (see content/common/net/OWNERS). once they're fine you > have my lgtm willchan, Could you take a look for this change?
Only have a few nits. http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:368: bool created) { Is this just ignored? It's sorta weird that DidCreateTempFile() is calling DidCreateFile() with kCreated = true, when created is just ignored. http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:1079: core_->file_message_loop_proxy_ = file_message_loop_proxy; Please DCHECK that this executes on the delegate message loop. Same with the SaveResponseToTemporaryFile(). It'd be great to DCHECK() that Start() hasn't been called yet too, but I don't see a good way to do that. http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... File content/common/net/url_fetcher_impl_unittest.cc (right): http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl_unittest.cc:951: const char data[] = "abcdefghijklmnopqrstuvwxyz"; kData and static? http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl_unittest.cc:952: const int data_size = sizeof(data); arraysize
http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... File content/common/net/url_fetcher_impl.cc (right): http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:368: bool created) { On 2012/03/08 16:38:39, willchan wrote: > Is this just ignored? It's sorta weird that DidCreateTempFile() is calling > DidCreateFile() with kCreated = true, when created is just ignored. Added DidCreateFileInternal http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl.cc:1079: core_->file_message_loop_proxy_ = file_message_loop_proxy; On 2012/03/08 16:38:39, willchan wrote: > Please DCHECK that this executes on the delegate message loop. Same with the > SaveResponseToTemporaryFile(). Done. > It'd be great to DCHECK() that Start() hasn't > been called yet too, but I don't see a good way to do that. Can we do this in another change? Maybe we should have a bool member in |core_| to remember if Start() was already called? http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... File content/common/net/url_fetcher_impl_unittest.cc (right): http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl_unittest.cc:951: const char data[] = "abcdefghijklmnopqrstuvwxyz"; On 2012/03/08 16:38:39, willchan wrote: > kData and static? Done. http://codereview.chromium.org/9585009/diff/19005/content/common/net/url_fetc... content/common/net/url_fetcher_impl_unittest.cc:952: const int data_size = sizeof(data); On 2012/03/08 16:38:39, willchan wrote: > arraysize Done.
Will, could you take another look? This patch is blocking some gdata related tasks. If needed, I can address remaining issues on behalf of hashimoto so we can check this in today.
Apologies, will take a look now. Was out all yesterday in meetings. On Fri, Mar 9, 2012 at 10:29 AM, <satorux@chromium.org> wrote: > Will, could you take another look? This patch is blocking some gdata > related > tasks. > > If needed, I can address remaining issues on behalf of hashimoto so we can > check > this in today. > > http://codereview.chromium.**org/9585009/<http://codereview.chromium.org/9585... >
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9585009/31001
Try job failure for 9585009-31001 (retry) on android for steps "Compile, build". It's a second try, previously, steps "Compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9585009/31001
Change committed as 125937 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
