Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371883003:
Files.app: Add an private API to get a download URL (Closed)
|
Created:
6 years, 5 months ago by yoshiki Modified:
6 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, yoshiki+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, hashimoto Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFiles.app: Add an private API to get a download URL
This patch just adds the API to get a download URL with an drive read only token.
BUG=305511
TEST=manually teste
R=asargent@chromium.org, hashimoto@chromium.org, kinaba@chromium.org
TBR=isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283671
Patch Set 1 #
Total comments: 15
Patch Set 2 : Addressed the comment #
Total comments: 4
Patch Set 3 : Addressed the comments #Patch Set 4 : rebase #
Messages
Total messages: 17 (0 generated)
@kinaba-san, could you take a look? Thanks.
+hashimoto for newly added fileds paramter of Drive API. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/drive_readonly_token_fetcher.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/drive_readonly_token_fetcher.h:20: class DriveReadonlyTokenFetcher : public OAuth2TokenService::Consumer { This class looks almost functionally identical to google_apis::AuthRequest. Can we merge them into one? At, least, I think the class can be in google_apis/drive rather than c/b/drive. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:928: drive::DriveServiceInterface* const drive_service = Looks unused. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:953: const std::string& access_token) { Please check the error |code|. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... File chrome/browser/drive/drive_api_service.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... chrome/browser/drive/drive_api_service.cc:112: "kind,items(kind,id,title,createdDate,sharedWithMeDate,downloadUrl," If we don't store downloadUrl in ResourceMetadata DB, I think we can omit including the field in FileList and ChangeList. Well, but on the other hand, if this inclusion does not affect performance, we should not add divergence. @hashimoto, do you have any thoughts? https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... chrome/browser/drive/drive_api_service.cc:118: "kind,items(file(kind,id,title,createdDate,sharedWithMeDate,downloadUrl," ditto
https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/file_system.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/file_system.h:241: // Part of GetDownloadUrl. Resolves the resource entry to get the resource it, s/it/ID/? https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/file_system_interface.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/file_system_interface.h:394: // Fetches the url to download the file directry. |callback| must not be null. nit: s/directry/directly/? https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/drive/job_scheduler.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/drive/job_scheduler.h:86: void GetFileResource(const std::string& resource_id, GetFileResource() is already present. No need to add this. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.h:243: // Implements the chrome.fileBrowserPrivate.getShareUrl method. s/getShareUrl/getDownloadUrl/ https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... File chrome/browser/drive/drive_api_service.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... chrome/browser/drive/drive_api_service.cc:112: "kind,items(kind,id,title,createdDate,sharedWithMeDate,downloadUrl," On 2014/07/09 06:02:48, kinaba wrote: > If we don't store downloadUrl in ResourceMetadata DB, > I think we can omit including the field in FileList and ChangeList. > > Well, but on the other hand, if this inclusion does not affect performance, we > should not add divergence. @hashimoto, do you have any thoughts? When last time I investigated this, "downloadUrl" was one of the "slow" fields which makes the request about 50% slower. But today it seems "downloadUrl" does not affect the performance much. https://docs.google.com/spreadsheets/d/1Gi2NcoTkTfNjY8HYSuAjVZPj3BKTXkki2y4yI... Getting a FileList with 300 entries takes 1.7 seconds both with and without "downloadUrl". Maybe the Drive server implementation has improved since then? https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... File chrome/browser/drive/drive_api_util.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... chrome/browser/drive/drive_api_util.cc:176: scoped_ptr<google_apis::FileResource> ConvertResourceEntryToFileResource( This function is unused now. Could you remove this function? (it can be in a separate patch)
Thank you for good comments. The patch got so simple! Could you take a look again? https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:928: drive::DriveServiceInterface* const drive_service = On 2014/07/09 06:02:47, kinaba wrote: > Looks unused. Done. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:953: const std::string& access_token) { On 2014/07/09 06:02:47, kinaba wrote: > Please check the error |code|. Done. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.h (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_manager/private_api_drive.h:243: // Implements the chrome.fileBrowserPrivate.getShareUrl method. On 2014/07/09 07:48:57, hashimoto wrote: > s/getShareUrl/getDownloadUrl/ Done. https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... File chrome/browser/drive/drive_api_util.cc (right): https://codereview.chromium.org/371883003/diff/80001/chrome/browser/drive/dri... chrome/browser/drive/drive_api_util.cc:176: scoped_ptr<google_apis::FileResource> ConvertResourceEntryToFileResource( On 2014/07/09 07:48:57, hashimoto wrote: > This function is unused now. > Could you remove this function? (it can be in a separate patch) I'll do it in the separate patch. https://codereview.chromium.org/381423002/
lgtm with a nit https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:939: std::string("https://www.googledrive.com/host/" + entry->resource_id()); std::string() is not necessary. What about using DriveApiUrlGenerator::kBaseDownloadUrlForProduction?
lgtm https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:952: g_browser_process->system_request_context(), How about getting a RequestContext from Profile instead of the browser process? According to kinaba@, we are using the system context for creating DriveAPIService in drive_integration_service.cc because using the profile's context may result in background Drive tasks blocking a page loading on a tab opened by the user. In this place, however, IIUC this API is called as a result of the user's action, not as a background task, so that blocking other user-initiated activities is not so problematic.
https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc (right): https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:939: std::string("https://www.googledrive.com/host/" + entry->resource_id()); On 2014/07/14 04:22:22, kinaba wrote: > std::string() is not necessary. > > What about using DriveApiUrlGenerator::kBaseDownloadUrlForProduction? Done. https://codereview.chromium.org/371883003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc:952: g_browser_process->system_request_context(), On 2014/07/14 04:46:35, hashimoto wrote: > How about getting a RequestContext from Profile instead of the browser process? > > According to kinaba@, we are using the system context for creating > DriveAPIService in drive_integration_service.cc because using the profile's > context may result in background Drive tasks blocking a page loading on a tab > opened by the user. > In this place, however, IIUC this API is called as a result of the user's > action, not as a background task, so that blocking other user-initiated > activities is not so problematic. Thanks for information. Done.
@asargent: could you take a look at change in file_browser_private.idl? Thanks.
lgtm
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/371883003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29433)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/371883003/160001
Message was sent while issue was closed.
Committed patchset #4 manually as r283671 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
