Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
560313002:
Use ExternalFileSystemBackend in the DriveURLRequestJob. (Closed)
|
Created:
6 years, 3 months ago by hirono Modified:
6 years, 3 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, tzik, tfarina, nhiroki, oshima+watch_chromium.org, stevenjb+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse ExternalFileSystemBackend in the DriveURLRequestJob.
This is the preparation for introducing other types of file system supported by
external file system backend into the drive protocol handler.
The CL does not change the format of drive URL.
BUG=367027
TEST=Open HTML, MHTML, PDF, GDOC, OGG, MP4 files on the drive volume.
Committed: https://crrev.com/10020e1813e6eb9edf6ff94bb702f781d87edc38
Cr-Commit-Position: refs/heads/master@{#296353}
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 6
Patch Set 3 : Fixed. #
Total comments: 10
Patch Set 4 : Fixed the test. #Patch Set 5 : Add TODO comment. #Patch Set 6 : Fixed. #
Total comments: 18
Patch Set 7 : Rebased. #Patch Set 8 : Remove debug log. #Patch Set 9 : Fixed. #Patch Set 10 : Fixed. #Patch Set 11 : Fix test. #
Total comments: 6
Patch Set 12 : Fixed. #
Total comments: 8
Patch Set 13 : Fixed #
Total comments: 10
Patch Set 14 : Fixed. #Patch Set 15 : Fixed. #
Total comments: 4
Patch Set 16 : Fixed. #
Total comments: 8
Patch Set 17 : Fixed. #
Total comments: 2
Patch Set 18 : Fixed. #Messages
Total messages: 33 (2 generated)
hirono@chromium.org changed reviewers: + kinaba@chromium.org, mtomasz@chromium.org
PTAL the CL? Thank you! @kinaba - chrome/browser/chromeos/drive/* @mtomasz - chrome/browser/chromeos/fileapi/file_system_backend.cc
https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:119: if (!backend) { Can it ever happen? If not, then DCHECK is enough. https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:141: storage::kFileSystemTypeExternal, We're creating a URL like: filesystem:drive:foobar while your doc specifies drive:foobar (or externalfile:foobar). Two different urls for the same thing. Is it all right?
https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:119: if (!backend) { On 2014/09/12 05:41:23, mtomasz wrote: > Can it ever happen? If not, then DCHECK is enough. Done. https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:141: storage::kFileSystemTypeExternal, On 2014/09/12 05:41:23, mtomasz wrote: > We're creating a URL like: filesystem:drive:foobar while your doc specifies > drive:foobar (or externalfile:foobar). Two different urls for the same thing. Is > it all right? This is a difficult point. But we need to generate filesystem URL internally because the FileSystemBackend takes the URL to refer a file.
https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:141: storage::kFileSystemTypeExternal, On 2014/09/12 07:02:15, hirono wrote: > On 2014/09/12 05:41:23, mtomasz wrote: > > We're creating a URL like: filesystem:drive:foobar while your doc specifies > > drive:foobar (or externalfile:foobar). Two different urls for the same thing. > Is > > it all right? > > This is a difficult point. But we need to generate filesystem URL internally > because the FileSystemBackend takes the URL to refer a file. Is it secure? What if an extension which doesn't have access to some drive files uses such malformed url? Can it skip permission checks?
https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:141: storage::kFileSystemTypeExternal, On 2014/09/12 08:24:54, mtomasz wrote: > On 2014/09/12 07:02:15, hirono wrote: > > On 2014/09/12 05:41:23, mtomasz wrote: > > > We're creating a URL like: filesystem:drive:foobar while your doc specifies > > > drive:foobar (or externalfile:foobar). Two different urls for the same > thing. > > Is > > > it all right? > > > > This is a difficult point. But we need to generate filesystem URL internally > > because the FileSystemBackend takes the URL to refer a file. > > Is it secure? What if an extension which doesn't have access to some drive files > uses such malformed url? Can it skip permission checks? This is secure because: 1. If the opener does not have drive: scheme, the operations (XHR, resolving URL) are forbidden by cross domain policy. 2. Besides filesystem:drive:/ is not parsed from GURL directly because it does not have standard scheme. So we cannot do the operation even from drive:xxxx files. https://code.google.com/p/chromium/codesearch#chromium/src/url/third_party/mo...
https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:38: if (!g_browser_process->profile_manager()->IsValidProfile(profile)) As far as I understand, this check and the conversion always has to be done on the UI thread ("Profile" is a resident of the UI thread.) https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:46: content::BrowserContext::GetDefaultStoragePartition(profile); GetDefaultStoragePartition() is marked as "DON't USE THIS" in the method comment. You might want to consider using a specific partition for this purpose. https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:150: // Detect mime type from the extension. c/b/cros/fm/filesystem_api_util.h has a utility function GetNonNativeLocalPathMimeType() that generally retrieves mime types. Could you consider reusing that? https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:155: mime_type_ = "multipart/related"; Why not FixupMimeType()?
On 2014/09/16 02:15:15, hirono wrote: > https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... > File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): > > https://codereview.chromium.org/560313002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/drive/drive_url_request_job.cc:141: > storage::kFileSystemTypeExternal, > On 2014/09/12 08:24:54, mtomasz wrote: > > On 2014/09/12 07:02:15, hirono wrote: > > > On 2014/09/12 05:41:23, mtomasz wrote: > > > > We're creating a URL like: filesystem:drive:foobar while your doc > specifies > > > > drive:foobar (or externalfile:foobar). Two different urls for the same > > thing. > > > Is > > > > it all right? > > > > > > This is a difficult point. But we need to generate filesystem URL internally > > > because the FileSystemBackend takes the URL to refer a file. > > > > Is it secure? What if an extension which doesn't have access to some drive > files > > uses such malformed url? Can it skip permission checks? > > This is secure because: > > 1. If the opener does not have drive: scheme, the operations (XHR, resolving > URL) are forbidden by cross domain policy. > 2. Besides filesystem:drive:/ is not parsed from GURL directly because it does > not have standard scheme. So we cannot do the operation even from drive:xxxx > files. > https://code.google.com/p/chromium/codesearch#chromium/src/url/third_party/mo... As discussed offline, my concern is that with this CL we would have two different url formats for basically the same thing: externalfile:foobar and filesystem:externalfile:foobar. If we could use *always* filesystem:externalfile:foobar, then it would be much more logical.
@mtomasz, @kinaba - I fixed the unit test. PTAL again? Thanks!
https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:38: if (!g_browser_process->profile_manager()->IsValidProfile(profile)) On 2014/09/16 06:46:52, kinaba wrote: > As far as I understand, this check and the conversion always has to be done on > the UI thread ("Profile" is a resident of the UI thread.) Done. https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:46: content::BrowserContext::GetDefaultStoragePartition(profile); On 2014/09/16 06:46:52, kinaba wrote: > GetDefaultStoragePartition() is marked as "DON't USE THIS" in the method > comment. > You might want to consider using a specific partition for this purpose. Done. https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:150: // Detect mime type from the extension. On 2014/09/16 06:46:52, kinaba wrote: > c/b/cros/fm/filesystem_api_util.h has a utility function > GetNonNativeLocalPathMimeType() that generally retrieves mime types. Could you > consider reusing that? Done. https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:155: mime_type_ = "multipart/related"; On 2014/09/16 06:46:52, kinaba wrote: > Why not FixupMimeType()? Done.
https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:33: // Obtains Profile from |profile_id|. Returns NULL on failure. nit: How about \n after namespace { and before closing it? Either if sine, but please check other files to be consistent. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:52: // Check if the |url| points a valid position or not. nit: position -> file/place/location? https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:71: class DriveURLRequestJob::GetFileSystemContextDelegate { Class comment is missing. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:73: GetFileSystemContextDelegate(void* profile_id, Can this class be moved to an anonymous namespace? https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:85: scoped_refptr<storage::FileSystemContext> file_system_context; Shall it be public? How about making private and add getters? https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:125: // drive: scheme and use filesystem: URL directly. Please add a crbug.com link. This is a large hack which we must not forget about. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:132: extensions::app_file_handler_util::GetMimeTypeForLocalPath( How about using https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... which does more advanced mime detection depending on a file system type? https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:56: // Called after Start method. OnGotDelegate -> OnDelegateObtained to be consistent with other methods below? https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:59: // Called after OnGotDelegate method. I think this comment is not very helpful. We should rather say which method is invoking this callback.
Thank you! https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_url_request_job.cc:33: // Obtains Profile from |profile_id|. Returns NULL on failure. On 2014/09/18 07:21:06, mtomasz wrote: > nit: How about \n after namespace { and before closing it? Either if sine, but > please check other files to be consistent. Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:52: // Check if the |url| points a valid position or not. On 2014/09/18 07:21:07, mtomasz wrote: > nit: position -> file/place/location? Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:71: class DriveURLRequestJob::GetFileSystemContextDelegate { On 2014/09/18 07:21:07, mtomasz wrote: > Class comment is missing. Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:73: GetFileSystemContextDelegate(void* profile_id, On 2014/09/18 07:21:07, mtomasz wrote: > Can this class be moved to an anonymous namespace? Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:85: scoped_refptr<storage::FileSystemContext> file_system_context; On 2014/09/18 07:21:07, mtomasz wrote: > Shall it be public? How about making private and add getters? Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:125: // drive: scheme and use filesystem: URL directly. On 2014/09/18 07:21:07, mtomasz wrote: > Please add a http://crbug.com link. This is a large hack which we must not forget > about. Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:132: extensions::app_file_handler_util::GetMimeTypeForLocalPath( On 2014/09/18 07:21:07, mtomasz wrote: > How about using > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > which does more advanced mime detection depending on a file system type? I'm not sure. It looks the function called GetMimeTypeForLocalPath internally. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:56: // Called after Start method. On 2014/09/18 07:21:07, mtomasz wrote: > OnGotDelegate -> OnDelegateObtained to be consistent with other methods below? Done. https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:59: // Called after OnGotDelegate method. On 2014/09/18 07:21:07, mtomasz wrote: > I think this comment is not very helpful. We should rather say which method is > invoking this callback. Done.
https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:132: extensions::app_file_handler_util::GetMimeTypeForLocalPath( On 2014/09/18 08:18:55, hirono wrote: > On 2014/09/18 07:21:07, mtomasz wrote: > > How about using > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > which does more advanced mime detection depending on a file system type? > > I'm not sure. It looks the function called GetMimeTypeForLocalPath internally. Ah, you're right. However, according to the comment in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... the function checks also for the extension as a fallback. So we shouldn't call GetMimeTypeFromExtension manually in #142.
On 2014/09/18 09:22:23, mtomasz wrote: > https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... > File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): > > https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... > chrome/browser/chromeos/drive/drive_url_request_job.cc:132: > extensions::app_file_handler_util::GetMimeTypeForLocalPath( > On 2014/09/18 08:18:55, hirono wrote: > > On 2014/09/18 07:21:07, mtomasz wrote: > > > How about using > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > which does more advanced mime detection depending on a file system type? > > > > I'm not sure. It looks the function called GetMimeTypeForLocalPath internally. > > Ah, you're right. However, according to the comment in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > the function checks also for the extension as a fallback. So we shouldn't call > GetMimeTypeFromExtension manually in #142. Sorry, the correct link is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:132: extensions::app_file_handler_util::GetMimeTypeForLocalPath( On 2014/09/18 09:22:23, mtomasz wrote: > On 2014/09/18 08:18:55, hirono wrote: > > On 2014/09/18 07:21:07, mtomasz wrote: > > > How about using > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > which does more advanced mime detection depending on a file system type? > > > > I'm not sure. It looks the function called GetMimeTypeForLocalPath internally. > > Ah, you're right. However, according to the comment in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > the function checks also for the extension as a fallback. So we shouldn't call > GetMimeTypeFromExtension manually in #142. Exactly. I removed the fallback. Thank you!
https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:132: extensions::app_file_handler_util::GetMimeTypeForLocalPath( On 2014/09/18 09:22:23, mtomasz wrote: > On 2014/09/18 08:18:55, hirono wrote: > > On 2014/09/18 07:21:07, mtomasz wrote: > > > How about using > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > > > which does more advanced mime detection depending on a file system type? > > > > I'm not sure. It looks the function called GetMimeTypeForLocalPath internally. > > Ah, you're right. However, according to the comment in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > the function checks also for the extension as a fallback. So we shouldn't call > GetMimeTypeFromExtension manually in #142. Exactly. I removed the fallback. Thank you!
https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:64: // Delegate for obtaining FileSystemContext, FileSystemURL, and mime type on I think this is not a delegate. It's more like a helper. Also mime type seems to be not related to context, so I'm not sure if GetFileSystemContextDelegate is a correct name. It seems that this class does three different things. Maybe UrlParser would be a better name, or just UrlHelper? The key responsibility of this class is to provide information (FileSystemURL, mime type and related context) for the input url. https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:130: base::Bind(&GetFileSystemContextDelegate::OnGotMimeTypeOnUIThread, Is it safe for shutdown? During shutdown the callback may not be called, because message loops maybe closed. As a result, we would never reach #157 and hence we would have a memory leak. I'm not 100% sure, but could you double check? To avoid that I used to keep an instance to "this" in a scoped_ptr and bind to a callback. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... But it would only make sense, if we theoretically can have a leak on shutdown here. WDYT? https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:76: void* const profile_id_; Just a random note. This is a very tricky design. I'm wondering why we don't pass the profile as weak pointer instead of passing it as void*. But this is done widely in Chromium, so we can go with it for consistency.
https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:64: // Delegate for obtaining FileSystemContext, FileSystemURL, and mime type on On 2014/09/19 00:43:49, mtomasz wrote: > I think this is not a delegate. It's more like a helper. Also mime type seems to > be not related to context, so I'm not sure if GetFileSystemContextDelegate is a > correct name. > > It seems that this class does three different things. Maybe UrlParser would be a > better name, or just UrlHelper? The key responsibility of this class is to > provide information (FileSystemURL, mime type and related context) for the input > url. I prefer URLHepler (Parser does not sound to access external resources). Done! https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:130: base::Bind(&GetFileSystemContextDelegate::OnGotMimeTypeOnUIThread, On 2014/09/19 00:43:48, mtomasz wrote: > Is it safe for shutdown? During shutdown the callback may not be called, because > message loops maybe closed. As a result, we would never reach #157 and hence we > would have a memory leak. I'm not 100% sure, but could you double check? > > To avoid that I used to keep an instance to "this" in a scoped_ptr and bind to a > callback. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > But it would only make sense, if we theoretically can have a leak on shutdown > here. WDYT? Let me do a safer way. Done. https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:76: void* const profile_id_; On 2014/09/19 00:43:49, mtomasz wrote: > Just a random note. This is a very tricky design. I'm wondering why we don't > pass the profile as weak pointer instead of passing it as void*. But this is > done widely in Chromium, so we can go with it for consistency. Acknowledged.
https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:68: typedef scoped_ptr<URLHelper> Lifetime; nit: Please add a comment describing how it works.
https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:68: typedef scoped_ptr<URLHelper> Lifetime; On 2014/09/19 05:10:34, mtomasz wrote: > nit: Please add a comment describing how it works. Done.
https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:8: #include <string> nit: #include <string> is already in the header. Please remove. https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:206: DriveIntegrationService* GetDriveIntegrationService(Profile* profile) { nit: Since it creates, how about renaming to CreateDriveIntegrationService? If called twice, then we'll return two different objects, so it's not a getter. https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:184: if (url.origin().scheme() == chrome::kDriveScheme) nit: We should either have a test that such url can't be passed from an extension, or at least a comment why it's safe. https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:144: for (size_t i = 0; i < arraysize(kMimeTypeReplacements); i++) { It seems that this entire mime replacement logic is just for one case "message/rfc822 -> multipart/related" (crbug.com/126955). Since the code is being rewritten, how about just doing a simple if-statement? We'd have 2 lines instead of 15. https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc (right): https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:46: class MockProfileManager : public ProfileManagerWithoutInit { Is this \n after namespace { and before closing consistent with other Chromium files? https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:48: explicit MockProfileManager(const base::FilePath& user_data_dir) I think this is not a mock. It's rather some kind of TestingProfileManager or FakeProfileManager. Per: http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:52: virtual Profile* CreateProfileHelper( Why is this needed? Do we need method? If so, then why? Please add comments. https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:60: }; In general, is this entire class needed? Isn't TestingProfileManager enough?
https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:8: #include <string> On 2014/09/19 05:30:53, mtomasz wrote: > nit: #include <string> is already in the header. Please remove. Done. https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:206: DriveIntegrationService* GetDriveIntegrationService(Profile* profile) { On 2014/09/19 05:30:53, mtomasz wrote: > nit: Since it creates, how about renaming to CreateDriveIntegrationService? If > called twice, then we'll return two different objects, so it's not a getter. Done. https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/560313002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:184: if (url.origin().scheme() == chrome::kDriveScheme) On 2014/09/19 05:30:53, mtomasz wrote: > nit: We should either have a test that such url can't be passed from an > extension, or at least a comment why it's safe. I added a comment. Let me add a test to prohibit parsing filesystem:externalfile:/xxx, in the future CL. I'm planning to add it to external_file_url_util.cc. https://codereview.chromium.org/580023003/patch/20001/30017 https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:144: for (size_t i = 0; i < arraysize(kMimeTypeReplacements); i++) { On 2014/09/19 05:30:53, mtomasz wrote: > It seems that this entire mime replacement logic is just for one case > "message/rfc822 -> multipart/related" (crbug.com/126955). Since the code is > being rewritten, how about just doing a simple if-statement? We'd have 2 lines > instead of 15. Done. https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc (right): https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:46: class MockProfileManager : public ProfileManagerWithoutInit { On 2014/09/19 05:30:53, mtomasz wrote: > Is this \n after namespace { and before closing consistent with other Chromium > files? Done. https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:48: explicit MockProfileManager(const base::FilePath& user_data_dir) On 2014/09/19 05:30:53, mtomasz wrote: > I think this is not a mock. It's rather some kind of TestingProfileManager or > FakeProfileManager. > > Per: > http://stackoverflow.com/questions/346372/whats-the-difference-between-faking... I removed the class as you suggested. Thank you! https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:52: virtual Profile* CreateProfileHelper( On 2014/09/19 05:30:53, mtomasz wrote: > Why is this needed? Do we need method? If so, then why? Please add comments. I removed the class as you suggested. Thank you! https://codereview.chromium.org/560313002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job_unittest.cc:60: }; On 2014/09/19 05:30:53, mtomasz wrote: > In general, is this entire class needed? Isn't TestingProfileManager enough? I found the class is not needed. Removed.
lgtm with two nits! https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:37: const char kCorrectMimeTypeForMHTML[] = "multipart/related"; I think it's not wrong. It's just another mime type for mhtml. How about: kMimeTypeForRFC822[] and kMimeTypeForMHTML? https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:185: // can not be parsed directly. The URL is created only by DriveURLRequestJob. nit: can not -> cannot
Thank you! https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:37: const char kCorrectMimeTypeForMHTML[] = "multipart/related"; On 2014/09/19 06:33:45, mtomasz wrote: > I think it's not wrong. It's just another mime type for mhtml. How about: > kMimeTypeForRFC822[] and kMimeTypeForMHTML? Done. https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/560313002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:185: // can not be parsed directly. The URL is created only by DriveURLRequestJob. On 2014/09/19 06:33:45, mtomasz wrote: > nit: can not -> cannot Done.
> TEST=Open HTML, MHTML, PDF, GDOC files on the drive volume. Could you also try .ogg audio and seek some video files? That will test byte range implementation touched in the current CL. https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:282: if (byte_range_.ComputeBounds(file_info.size)) { When can this ComputeBounds fail? I mean, may either DCHECK or NotifyStartError() be more suitable? https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:342: std::min(static_cast<int64>(buf_size), remaining_bytes_), Maybe: std::min<int64>(buf_size, remaining_bytes_) https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:38: const std::string& mime_type)> HelperCallback; "Helper" here and below (OnHelperResultObtained) is not an explained term at this point until you see .cc file, but IMO comments in a header file should be self-complete. Could you mention as a comment that this is for internal helper class defined in drive_url_request_job.cc? https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:60: callback.Run(GURL()); It looks you have to post to IO thread (as the !file_system case above does.)
Checked with ogg and webm files. Thank you! https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.cc (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:282: if (byte_range_.ComputeBounds(file_info.size)) { On 2014/09/24 02:11:13, kinaba wrote: > When can this ComputeBounds fail? > I mean, may either DCHECK or NotifyStartError() be more suitable? > The failure can be caused by invalid range requests. So I chose NotifyStartError. Done. https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.cc:342: std::min(static_cast<int64>(buf_size), remaining_bytes_), On 2014/09/24 02:11:13, kinaba wrote: > Maybe: std::min<int64>(buf_size, remaining_bytes_) Done. https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:38: const std::string& mime_type)> HelperCallback; On 2014/09/24 02:11:13, kinaba wrote: > "Helper" here and below (OnHelperResultObtained) is not an explained term at > this point until you see .cc file, but IMO comments in a header file should be > self-complete. > Could you mention as a comment that this is for internal helper class defined in > drive_url_request_job.cc? Done. https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/560313002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:60: callback.Run(GURL()); On 2014/09/24 02:11:13, kinaba wrote: > It looks you have to post to IO thread (as the !file_system case above does.) Thank you for catching it. Done.
lgtm https://codereview.chromium.org/560313002/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:62: // which running on the UI thread. nit: which is
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/560313002/340001
Thank you!
https://codereview.chromium.org/560313002/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/drive_url_request_job.h (right): https://codereview.chromium.org/560313002/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/drive/drive_url_request_job.h:62: // which running on the UI thread. On 2014/09/24 03:46:18, kinaba wrote: > nit: which is Done.
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as 32f8278258f81721102add64136f75c844c49d12
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/10020e1813e6eb9edf6ff94bb702f781d87edc38 Cr-Commit-Position: refs/heads/master@{#296353} |
