Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
527773003:
Add GetURLForBrowserTab method to the external file system backend. (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, darin-cc_chromium.org, 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. |
DescriptionAdd GetURLForBrowserTab method to the external file system backend.
The method will be called from FileSystemURLRequestJob class to redirect requests for the drive hosted documents.
BUG=367027
TEST=manually; Add the method call to FileSystemURLRequestJob and open FileSystemURL in browser tab.
Committed: https://crrev.com/d9cf42f666d09f57c4592f945fe5085b88eb38de
Cr-Commit-Position: refs/heads/master@{#293494}
Patch Set 1 #Patch Set 2 : Remove GURL header. #
Total comments: 5
Patch Set 3 : Fixed. #Patch Set 4 : Fix comment. #
Total comments: 4
Patch Set 5 : . #Patch Set 6 : . #
Total comments: 10
Patch Set 7 : Handled @kinaba's comments #
Total comments: 22
Patch Set 8 : . #
Total comments: 12
Patch Set 9 : Fixed. #Patch Set 10 : . #
Total comments: 6
Patch Set 11 : #Patch Set 12 : Fixed. #Messages
Total messages: 28 (3 generated)
hirono@chromium.org changed reviewers: + hashimoto@chromium.org, tzik@chromium.org
PTAL the CL? Thank you! @tzik - webkit/browser/fileapi/file_system_backend.h @hashimoto - chrome/browser/chromeos/drive/* @kinaba - chrome/browser/chromeos/fileapi/* @mtomasz - chrome/browser/chromeos/file_system_provider/*
hirono@chromium.org changed reviewers: + kinaba@chromium.org, mtomasz@chromium.org
PTAL the CL? Thank you! @kinaba - chrome/browser/chromeos/fileapi/* @mtomasz - chrome/browser/chromeos/file_system_provider/*
https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:162: virtual void GetAlternativeURL(const storage::FileSystemURL& url, I'm wondering if the alternative URL is generic enough to be in the interface for every file system backend. Is this used by Files app only? If so, then why not using chrome.fileBrowserPrivate.getDriveEntryProperties() which provides urls such as share url? I'm currently working on making chrome.fileBrowserPrivate.getDriveEntryProperties() work for other backends than Drive. https://codereview.chromium.org/515033002/ WDYT?
https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:162: virtual void GetAlternativeURL(const storage::FileSystemURL& url, On 2014/09/02 07:37:49, mtomasz wrote: > I'm wondering if the alternative URL is generic enough to be in the interface > for every file system backend. > > Is this used by Files app only? If so, then why not using > chrome.fileBrowserPrivate.getDriveEntryProperties() which provides urls such as > share url? I'm currently working on making > chrome.fileBrowserPrivate.getDriveEntryProperties() work for other backends than > Drive. > > https://codereview.chromium.org/515033002/ > > WDYT? We have another path to open drive files without using getDriveEntryProperties. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... The code is used when a user presses Ctrl+O in the browser and opens the drive hosted documents. The file selector dialog is Files.app, but Files.app does not know how to use the selected file after closing the dialog, thus Files.app cannot return drive URL here.
https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/20001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:162: virtual void GetAlternativeURL(const storage::FileSystemURL& url, On 2014/09/02 07:46:18, hirono wrote: > On 2014/09/02 07:37:49, mtomasz wrote: > > I'm wondering if the alternative URL is generic enough to be in the interface > > for every file system backend. > > > > Is this used by Files app only? If so, then why not using > > chrome.fileBrowserPrivate.getDriveEntryProperties() which provides urls such > as > > share url? I'm currently working on making > > chrome.fileBrowserPrivate.getDriveEntryProperties() work for other backends > than > > Drive. > > > > https://codereview.chromium.org/515033002/ > > > > WDYT? > > We have another path to open drive files without using getDriveEntryProperties. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > The code is used when a user presses Ctrl+O in the browser and opens the drive > hosted documents. > The file selector dialog is Files.app, but Files.app does not know how to use > the selected file after closing the dialog, thus Files.app cannot return drive > URL here. > I see. How about adding a GURL field to SelectedFileInfo? We could pass the url from Files app easily. In browser.cc we are converting a local path to GURL anyway, so we could just skip this step if a url is provided in SelectedFileInfo. I'm thinking more, and actually GetAlternativeURL method in the backend sounds not so bad. Would we use it to do an actual *redirection*? If so, then we shouldn't skip csp/origin restrictions. Eg. when loading the file with XMLHttpReader. There may be other side effects, though. When we read from the hosted file using fileapi, we would read real contents of the file, but if we use XMLHttpRequest, then we will get a redirection, and read from the web. I'm not sure if we actually permit to read from a hosted document, and what would happen. If we decide to go with the GetAlternativeURL() then I think we should clean it up. First of all it doesn't make sense for any backends other than Drive and FSP. How about renaming to ResolveBrowserURL() which would generate file:///... for local files, and http://... for hosted files? I'm still not very sure, so let's see what other folks think about it. WDYT?
> I see. How about adding a GURL field to SelectedFileInfo? We could pass the url > from Files app easily. In browser.cc we are converting a local path to GURL > anyway, so we could just skip this step if a url is provided in > SelectedFileInfo. It is one possible way. The concern is that other classes using SelectedFileInfo (e.g. <input type="file">, print preview dialog, extension loader...) do not need the field. If possible, I'd like to avoid adding the specific filed to the class. (Although I'm planning to add FileSystemURL to the class for streaming file upload crbug.com/126902...) > I'm thinking more, and actually GetAlternativeURL method in the backend sounds > not so bad. Would we use it to do an actual *redirection*? If so, then we > shouldn't skip csp/origin restrictions. Eg. when loading the file with > XMLHttpReader. Yes it will do redirection by HTML 302 status. Maybe we need more specific name for the method as you mentioned below. > There may be other side effects, though. When we read from the hosted file using > fileapi, we would read real contents of the file, but if we use XMLHttpRequest, > then we will get a redirection, and read from the web. Yes, this is the non-straightforward point of the plan. But the rule is simple. Only FileSystemURLRequestJob cares about the redirection before reading the file. If we access the file in other ways, we can read the file contents directly. > I'm not sure if we actually permit to read from a hosted document, and what would happen. Let me check it. I'm guessing the current implementation provides redirection for drive:// URL, and provides actual file contents (JSON file) for File System API. > If we decide to go with the GetAlternativeURL() then I think we should clean it > up. First of all it doesn't make sense for any backends other than Drive and > FSP. How about renaming to ResolveBrowserURL() which would generate file:///... > for local files, and http://... for hosted files? After enabling filesystem:file/// URL, we can open all the files located in an external file system by using file system URLs. So we don't need to resolve the file with file:/// URL. We redirect the filesystem:file/// URLs to other URLs if the other URL provides a better interface for the file. More specific name is good for readability. GetBrowserURL, GetURLForBrowserTab, GetAlternativeURLForBrowser...
https://codereview.chromium.org/527773003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.h:46: base::Callback<void(const GURL& url)> callback) OVERRIDE; const ref, here and elsewhere?
Thanks! https://codereview.chromium.org/527773003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.h:46: base::Callback<void(const GURL& url)> callback) OVERRIDE; On 2014/09/03 06:56:35, tzik wrote: > const ref, here and elsewhere? Done.
https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:160: // Gets an alternative URL for browser tab. e.g. Google Drive URL for hosted nit: "alternative" sounds strange. It means that either is fine (you have an alternative). How about: 1. alternative -> fallback (since the browser fallbacks to it if the file contents are not available locally) 2. alternative -> content (since it's an URL for contents) 3. alternative -> redirect (since it's used for redirection)? WDYT? https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:164: const base::Callback<void(const GURL& url)>& callback) = 0; nit: How about typedefing this callback like in #46?
https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:160: // Gets an alternative URL for browser tab. e.g. Google Drive URL for hosted On 2014/09/03 09:44:42, mtomasz wrote: > nit: "alternative" sounds strange. It means that either is fine (you have an > alternative). How about: > 1. alternative -> fallback (since the browser fallbacks to it if the file > contents are not available locally) > 2. alternative -> content (since it's an URL for contents) > 3. alternative -> redirect (since it's used for redirection)? > > WDYT? I chose "redirect" URL. https://codereview.chromium.org/527773003/diff/60001/webkit/browser/fileapi/f... webkit/browser/fileapi/file_system_backend.h:164: const base::Callback<void(const GURL& url)>& callback) = 0; On 2014/09/03 09:44:42, mtomasz wrote: > nit: How about typedefing this callback like in #46? Done.
lgtm for c/b/cros/drive/* with several nits. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:26: namespace { put a blank line between namespace { the following declaration. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:27: void PostURLOnIOThread(const FileSystemBackendDelegate::URLCallback& callback, Maybe: PostURLToIOThread? Or I think you can remove this function and in line the BrowserThread::PostTask(BrowserThread::IO, ...) call, which is descriptive and concise enough. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:59: base::Bind(GetURLForBrowserTabOnUIThreadWithResourceEntry, callback)); &GetURLFor... https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:133: base::Bind(GetURLForBrowserTabOnUIThread, url, callback)); &GetURLFor... https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:9: #include "base/callback.h" callback_forward.h
Thanks! https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:26: namespace { On 2014/09/04 04:17:04, kinaba wrote: > put a blank line between namespace { the following declaration. Done. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:27: void PostURLOnIOThread(const FileSystemBackendDelegate::URLCallback& callback, On 2014/09/04 04:17:04, kinaba wrote: > Maybe: PostURLToIOThread? > > Or I think you can remove this function and in line the > BrowserThread::PostTask(BrowserThread::IO, ...) call, which is descriptive and > concise enough. Removed the function. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:59: base::Bind(GetURLForBrowserTabOnUIThreadWithResourceEntry, callback)); On 2014/09/04 04:17:04, kinaba wrote: > &GetURLFor... Done. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:133: base::Bind(GetURLForBrowserTabOnUIThread, url, callback)); On 2014/09/04 04:17:04, kinaba wrote: > &GetURLFor... Done. https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:9: #include "base/callback.h" On 2014/09/04 04:17:04, kinaba wrote: > callback_forward.h Done.
https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:28: void GetURLForBrowserTabOnUIThreadWithResourceEntry( nit: Comments are missing. Especially we should comment that callback is called on IO thread. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:57: nit: The empty line should be one line lower? https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc:75: DCHECK_CURRENTLY_ON(BrowserThread::IO); nit: Could you add a check for url type like in #60? https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:393: switch (url.type()) { I think we should add a DCHECK for url_is_valid(), and also check for permissions by calling IsAccessAllowed(). Similarly to other methods. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:410: return; In another switch-case statements in this file we call the default callback outside the switch. Can we do that here too for consistency? See: #316, #347, #380. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:38: typedef base::Callback<void(const GURL&)> URLCallback; This callback is same as in storage::FileSystemBackend. How about moving it outside of the class to the storage namespace and reusing in other places? Especially than URLCallback sounds very generic. WDYT? https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc:62: callback.Run(GURL()); How about checking for the url.type like in #47? Note, that I don't do that in #55, because the method is not implemented. However, this method is implemented. Basically there is never redirection for mtp. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:50: typedef base::Callback<void(const GURL& url)> URLCallback; nit: Comment is missing, but there is one for the callback above. I think we should be consistent. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:161: // Gets an redirect URL for browser tab. e.g. Google Drive URL for hosted nit: an -> a https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:163: virtual void GetURLForBrowserTab( Naming of this method is still confusing to me. Looking at the name I expect GetURLForBrowserTab to return the url for browser tab. If empty string is returned then I'm expecting that there is no URL for browser tab. I'd expect that it returns always exactly that url which is used for browser url. However, the actual behavior is different. As in the comment, it *redirects* the default URL for browser tab. How about renaming to GetRedirectURLForBrowserTab? https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:165: const URLCallback& callback) = 0; I just realized one thing. Is this safe? What if a FSP provider returns a "chrome://evil" URL? Is it safe?
https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:28: void GetURLForBrowserTabOnUIThreadWithResourceEntry( On 2014/09/04 05:30:01, mtomasz wrote: > nit: Comments are missing. Especially we should comment that callback is called > on IO thread. Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:57: On 2014/09/04 05:30:01, mtomasz wrote: > nit: The empty line should be one line lower? Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc:75: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2014/09/04 05:30:01, mtomasz wrote: > nit: Could you add a check for url type like in #60? Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:393: switch (url.type()) { On 2014/09/04 05:30:01, mtomasz wrote: > I think we should add a DCHECK for url_is_valid(), and also check for > permissions by calling IsAccessAllowed(). Similarly to other methods. Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend.cc:410: return; On 2014/09/04 05:30:01, mtomasz wrote: > In another switch-case statements in this file we call the default callback > outside the switch. Can we do that here too for consistency? > > See: #316, #347, #380. Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:38: typedef base::Callback<void(const GURL&)> URLCallback; On 2014/09/04 05:30:01, mtomasz wrote: > This callback is same as in storage::FileSystemBackend. How about moving it > outside of the class to the storage namespace and reusing in other places? > Especially than URLCallback sounds very generic. WDYT? I agree. Done. https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/mtp_file_system_backend_delegate.cc:62: callback.Run(GURL()); On 2014/09/04 05:30:01, mtomasz wrote: > How about checking for the url.type like in #47? Note, that I don't do that in > #55, because the method is not implemented. However, this method is implemented. > Basically there is never redirection for mtp. Done. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:50: typedef base::Callback<void(const GURL& url)> URLCallback; On 2014/09/04 05:30:01, mtomasz wrote: > nit: Comment is missing, but there is one for the callback above. I think we > should be consistent. Done. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:161: // Gets an redirect URL for browser tab. e.g. Google Drive URL for hosted On 2014/09/04 05:30:01, mtomasz wrote: > nit: an -> a Done. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:163: virtual void GetURLForBrowserTab( On 2014/09/04 05:30:01, mtomasz wrote: > Naming of this method is still confusing to me. Looking at the name I expect > GetURLForBrowserTab to return the url for browser tab. If empty string is > returned then I'm expecting that there is no URL for browser tab. I'd expect > that it returns always exactly that url which is used for browser url. > > However, the actual behavior is different. As in the comment, it *redirects* the > default URL for browser tab. > > How about renaming to GetRedirectURLForBrowserTab? Done. https://codereview.chromium.org/527773003/diff/120001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:165: const URLCallback& callback) = 0; On 2014/09/04 05:30:01, mtomasz wrote: > I just realized one thing. Is this safe? What if a FSP provider returns a > "chrome://evil" URL? Is it safe? I added a comment to ExternalFileSystemBackendDelegate::GetRedirectURLForBrowserTab.
https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:29: // Obtains the browser URL from |entry|. nit: Can we add that callback is called on IO thread? This is very important. https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:46: // Requestes to obtain ResourceEntry for the |url|. nit: Can we add that the callback is called on IO thread? https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:26: } // namespace storage nit: Not realted to this CL, but can be easily merged with #28? https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:30: typedef base::Callback<void(const GURL&)> URLCallback; nit: This is already declared in webkit/browser/fileapi/file_system_backend.h. Shall we add an include instead? https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:65: // in a browser tab. nit: Actually not only browser tab. It can be also opened in <img>, <video> in apps, extensions, as well as via XmlHttpRequest. Right? If so, then maybe GetRedirectURLForContents? WDYT? https://codereview.chromium.org/527773003/diff/140001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/140001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:24: } nit: Not related to this patch, but these namespaces can be merged. #22 and #26.
chrome/browser/chromeos/drive/fileapi/ lgtm (though changes there already approved by kinaba@)
https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc (right): https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:29: // Obtains the browser URL from |entry|. On 2014/09/04 06:18:51, mtomasz wrote: > nit: Can we add that callback is called on IO thread? This is very important. Done. https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/drive/fileapi/file_system_backend_delegate.cc:46: // Requestes to obtain ResourceEntry for the |url|. On 2014/09/04 06:18:51, mtomasz wrote: > nit: Can we add that the callback is called on IO thread? Done. https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:26: } // namespace storage On 2014/09/04 06:18:51, mtomasz wrote: > nit: Not realted to this CL, but can be easily merged with #28? Done. https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:30: typedef base::Callback<void(const GURL&)> URLCallback; On 2014/09/04 06:18:51, mtomasz wrote: > nit: This is already declared in webkit/browser/fileapi/file_system_backend.h. > > Shall we add an include instead? Done. https://codereview.chromium.org/527773003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:65: // in a browser tab. On 2014/09/04 06:18:51, mtomasz wrote: > nit: Actually not only browser tab. It can be also opened in <img>, <video> in > apps, extensions, as well as via XmlHttpRequest. Right? > > If so, then maybe GetRedirectURLForContents? WDYT? Done. https://codereview.chromium.org/527773003/diff/140001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/140001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:24: } On 2014/09/04 06:18:51, mtomasz wrote: > nit: Not related to this patch, but these namespaces can be merged. #22 and #26. Done.
lgtm with a nit https://codereview.chromium.org/527773003/diff/180001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/180001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:160: // Gets a redirect URL for browser. e.g. Google Drive URL for hosted nit: browser -> contents
https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:61: // or referred from <img>, <video>, XmlHttpRequest, etc... What does "secure" mean in this context? For the third sentence, I thought the redirected URL is only for a main resource in a tab, and not for <img> or others. Is it right? s/Xml/XML/?
https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:61: // or referred from <img>, <video>, XmlHttpRequest, etc... On 2014/09/04 11:22:05, tzik wrote: > What does "secure" mean in this context? It means there is no problem if the URL is opened in a browser tab. Currently only drive hosted files have the redirection. But if we use the method for other backends or files, we may need further consideration. > For the third sentence, I thought the redirected URL is only for a main resource > in a tab, and not for <img> or others. Is it right? Currently the redirection is set for only drive hosted files. But if the backend returns the URL for images, and other HTML file refers the image, the redirected contents will be shown. > s/Xml/XML/? Done. https://codereview.chromium.org/527773003/diff/180001/webkit/browser/fileapi/... File webkit/browser/fileapi/file_system_backend.h (right): https://codereview.chromium.org/527773003/diff/180001/webkit/browser/fileapi/... webkit/browser/fileapi/file_system_backend.h:160: // Gets a redirect URL for browser. e.g. Google Drive URL for hosted On 2014/09/04 11:10:37, mtomasz wrote: > nit: browser -> contents Done.
lgtm https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:61: // or referred from <img>, <video>, XmlHttpRequest, etc... On 2014/09/05 06:09:05, hirono wrote: > On 2014/09/04 11:22:05, tzik wrote: > > What does "secure" mean in this context? > It means there is no problem if the URL is opened in a browser tab. > Currently only drive hosted files have the redirection. But if we use the method > for other backends or files, we may need further consideration. > > > > For the third sentence, I thought the redirected URL is only for a main > resource > > in a tab, and not for <img> or others. Is it right? > Currently the redirection is set for only drive hosted files. But if the backend > returns the URL for images, and other HTML file refers the image, the redirected > contents will be shown. > > > > s/Xml/XML/? > Done. > Ah, so the third sentence explains for what usage it should be secure. Make sense. How about combining these two by removing ". The URL is"?
Thank you! https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/fileapi/file_system_backend_delegate.h (right): https://codereview.chromium.org/527773003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/fileapi/file_system_backend_delegate.h:61: // or referred from <img>, <video>, XmlHttpRequest, etc... On 2014/09/05 07:09:23, tzik wrote: > On 2014/09/05 06:09:05, hirono wrote: > > On 2014/09/04 11:22:05, tzik wrote: > > > What does "secure" mean in this context? > > It means there is no problem if the URL is opened in a browser tab. > > Currently only drive hosted files have the redirection. But if we use the > method > > for other backends or files, we may need further consideration. > > > > > > > For the third sentence, I thought the redirected URL is only for a main > > resource > > > in a tab, and not for <img> or others. Is it right? > > Currently the redirection is set for only drive hosted files. But if the > backend > > returns the URL for images, and other HTML file refers the image, the > redirected > > contents will be shown. > > > > > > > s/Xml/XML/? > > Done. > > > > Ah, so the third sentence explains for what usage it should be secure. Make > sense. > How about combining these two by removing ". The URL is"? Done.
The CQ bit was checked by hirono@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/527773003/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 03549c6ba4ee831c8c4f8508409c6dd6b1666567
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d9cf42f666d09f57c4592f945fe5085b88eb38de Cr-Commit-Position: refs/heads/master@{#293494} |
