Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11659005:
Added common function to check for supported syncable file system service names. (Closed)
|
Created:
8 years ago by calvinlo Modified:
7 years, 11 months ago CC:
chromium-reviews, tzik+watch_chromium.org, Aaron Boodman, kinuko+watch, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded common function to check for supported syncable file system service names.
BUG=160496
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175731
Patch Set 1 #
Total comments: 4
Patch Set 2 : tzik review #1 #
Total comments: 2
Patch Set 3 : tzik review #2 #
Total comments: 2
Patch Set 4 : Kinuko review #1 #
Total comments: 2
Patch Set 5 : Removed TODO #Messages
Total messages: 16 (0 generated)
Hi Taiju San, can you please take a quick look at this before I send to benwells for ownership review? Thanks.
https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string& error) { Our coding style doesn't allow non-const reference. Please use pointer for |error|. https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:76: // TODO(calvinlo): For now only gDrive cloud service is supported. We don't call it gDrive. Maybe, it's just Drive or Google Drive.
https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string& error) { On 2012/12/25 05:53:28, tzik wrote: > Our coding style doesn't allow non-const reference. > Please use pointer for |error|. Done. https://codereview.chromium.org/11659005/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:76: // TODO(calvinlo): For now only gDrive cloud service is supported. On 2012/12/25 05:53:28, tzik wrote: > We don't call it gDrive. Maybe, it's just Drive or Google Drive. Done.
lgtm https://codereview.chromium.org/11659005/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string* error) { Could you add DCHECK for |error| here?
https://codereview.chromium.org/11659005/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string* error) { On 2012/12/25 06:14:26, tzik wrote: > Could you add DCHECK for |error| here? Done.
Hi Ben, can you please check this patch for extension/ ownership. Thanks.
lgtm
https://codereview.chromium.org/11659005/diff/8001/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string* error) { naming: please capitalize function names in c++/chrome
Updated to capitalize function names.
Updated to capitalize function names. https://codereview.chromium.org/11659005/diff/8001/chrome/browser/extensions/... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:75: bool isValidServiceName(const std::string& service_name, std::string* error) { On 2013/01/04 10:45:53, kinuko wrote: > naming: please capitalize function names in c++/chrome Done.
lgtm https://codereview.chromium.org/11659005/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:77: // TODO(calvinlo): For now only Google Drive cloud service is supported. nit: Probably this doesn't need to be a TODO comment as it doesn't have a clear action item.
https://codereview.chromium.org/11659005/diff/14001/chrome/browser/extensions... File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc (right): https://codereview.chromium.org/11659005/diff/14001/chrome/browser/extensions... chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:77: // TODO(calvinlo): For now only Google Drive cloud service is supported. Removed TODO
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/11659005/16003
Retried try job too often on win_aura for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/11659005/16003
Message was sent while issue was closed.
Change committed as 175731 |
