Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
260943002:
[fsp] Extract common code to FileSystemProvidedInternalFunction. (Closed)
|
Created:
6 years, 7 months ago by mtomasz Modified:
6 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[fsp] Extract common code to FileSystemProvidedInternalFunction.
Since more internal functions are coming soon, and they are mostly the same,
a base class has been extracted to reuse the code.
TBR=benwells@chromium.org
TEST=Refactoring only. Covered by current tests.
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267896
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 20
Patch Set 3 : Simplified + addressed comments. #
Total comments: 14
Patch Set 4 : Fixed. #
Total comments: 5
Patch Set 5 : Addressed a comment. #Patch Set 6 : Rebased. #
Messages
Total messages: 19 (0 generated)
@hashimoto: PTAL. Thanks.
Sorry for being late to response. Seems something went wrong during the last rebase? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:73: NOTREACHED(); nit: How about removing "defualt" here and moving NOTREACHED to before the return statement at the end of this function? This way, when you add a new enum to ProviderError without updating this function in future, compiler will raise an error here to let you know what to update. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:82: bool FileSystemProviderInternalFunction::Parse() { nit: Our style guide says "Function declaration order should match function definition order." http://www.chromium.org/developers/coding-style/#Code_Formatting https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:83: if (!args_->GetInteger(0, &file_system_id_)) nit: !args_->GetInteger(0, &file_system_id_) && !args_->GetInteger(1, &request_id_) ? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:157: return false; You are repeating the same code to get RequestManager twice in RejectRequest and FullfillRequest. How about making this a separate function? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:177: is_valid_ = Parse(); Why saving the result to is_valid_ here? Do we still need to proceed to RunSync() when Parse() fails? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.h (right): https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:11: #include "base/values.h" nit: "class DictionaryValue;" is enough? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:45: class FileSystemProviderInternalFunction : public ChromeSyncExtensionFunction { Please add a class comment. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:53: // caller should also return false in such case. Since there are many parties (provider extension, user extension, extension function inheriting this class, and this class) involved here, could you make this comment a bit clearer? Please note that there can be two different meanings for "error" (i.e. errors returned to the user extension and errors returned to the provider extension) and three meanings for "return" here. (returning to the provider extension JS code, returning to the user extension JS code, and returning to the extension function) IIUC, an extremely lengthy version of this comment would be: (The extension function called by the provider extension uses this function to) reject the request (made by the user extension and sends the error to the user). If failed, (i.e. the reject message cannot be delivered to the user) sets an error (as the result of the extension function) and returns false (to the extension function). The caller (i.e. the extension function) should also return false (to let the provider extension know about the failure) in such case. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:57: // caller should also return false then. ditto. Also, please briefly describe what |value| and |has_net| are for. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:62: bool is_valid() { return is_valid_; } Please add a comment about what "valid" means.
https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:73: NOTREACHED(); On 2014/05/01 09:20:12, hashimoto wrote: > nit: How about removing "defualt" here and moving NOTREACHED to before the > return statement at the end of this function? > This way, when you add a new enum to ProviderError without updating this > function in future, compiler will raise an error here to let you know what to > update. Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:82: bool FileSystemProviderInternalFunction::Parse() { On 2014/05/01 09:20:12, hashimoto wrote: > nit: Our style guide says "Function declaration order should match function > definition order." > http://www.chromium.org/developers/coding-style/#Code_Formatting Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:83: if (!args_->GetInteger(0, &file_system_id_)) On 2014/05/01 09:20:12, hashimoto wrote: > nit: !args_->GetInteger(0, &file_system_id_) && !args_->GetInteger(1, > &request_id_) ? Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:157: return false; On 2014/05/01 09:20:12, hashimoto wrote: > You are repeating the same code to get RequestManager twice in RejectRequest and > FullfillRequest. > How about making this a separate function? I simplified the code. Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:177: is_valid_ = Parse(); On 2014/05/01 09:20:12, hashimoto wrote: > Why saving the result to is_valid_ here? > Do we still need to proceed to RunSync() when Parse() fails? I wanted the macro EXTENSION_FUNCTION_VALIDATE to be always executed if the request is malformed. However, for simplicity I changed as you suggested. Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.h (right): https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:11: #include "base/values.h" On 2014/05/01 09:20:12, hashimoto wrote: > nit: "class DictionaryValue;" is enough? Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:45: class FileSystemProviderInternalFunction : public ChromeSyncExtensionFunction { On 2014/05/01 09:20:12, hashimoto wrote: > Please add a class comment. Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:53: // caller should also return false in such case. On 2014/05/01 09:20:12, hashimoto wrote: > Since there are many parties (provider extension, user extension, extension > function inheriting this class, and this class) involved here, could you make > this comment a bit clearer? > Please note that there can be two different meanings for "error" (i.e. errors > returned to the user extension and errors returned to the provider extension) > and three meanings for "return" here. (returning to the provider extension JS > code, returning to the user extension JS code, and returning to the extension > function) > > IIUC, an extremely lengthy version of this comment would be: > (The extension function called by the provider extension uses this function to) > reject the request (made by the user extension and sends the error to the user). > If failed, (i.e. the reject message cannot be delivered to the user) sets an > error (as the result of the extension function) and returns false (to the > extension function). The caller (i.e. the extension function) should also return > false (to let the provider extension know about the failure) in such case. I reworked the code to make it simpler. Now the methods are much shorter, and do not return any value. WDYT? https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:57: // caller should also return false then. On 2014/05/01 09:20:12, hashimoto wrote: > ditto. > > Also, please briefly describe what |value| and |has_net| are for. Done. https://codereview.chromium.org/260943002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:62: bool is_valid() { return is_valid_; } On 2014/05/01 09:20:12, hashimoto wrote: > Please add a comment about what "valid" means. Removed.
https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:102: SendResponse(RunSync()); Seems RunSync always returns true. Isn't it necessary to call SendResponse with false to run the error callback given by the caller JS code on error cases? https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:126: SendResponse(true); Why true is passed to SendResponse here? It seems an error case to me. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:138: result->Append(CreateError(kNotFoundErrorName, kResponseFailedErrorMessage)); name and message are unused? https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.h (right): https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:14: class DictionaryValue; This should be in the base namespace. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:59: // a value to be returned by the caller (RunImpl). Seems this function returns nothing? https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:62: // Fulfils the request with parsed arguments of this API function encapsulated nit: s/Fulfils/Fulfills/ https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:64: // will be called again for this request. Returns a value to be returned by Seems this function returns nothing?
https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:102: SendResponse(RunSync()); On 2014/05/02 05:56:18, hashimoto wrote: > Seems RunSync always returns true. > Isn't it necessary to call SendResponse with false to run the error callback > given by the caller JS code on error cases? It returns false if the request is malformed (EXTENSION_FUNCTION_VALIDATE macro). Basically, this is confusing. In case of a single callback, returning false is used to return an error via chrome.runtime.lastError. However, in File System Provider API we have two-callback architecture: onSuccess and onError, like in fileapi methods. So, DOMError is used instead of chrome.runtime.lastError. This is the same way it works for fileapi functions. Eg. for webkitRequestLocalFileSystemURL a DOMError is returned via the error callback, and chrome.runtime.lastError *is not* set. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:126: SendResponse(true); On 2014/05/02 05:56:18, hashimoto wrote: > Why true is passed to SendResponse here? > It seems an error case to me. As above. DOMError vs. chrome.runtime.lastError problem. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:138: result->Append(CreateError(kNotFoundErrorName, kResponseFailedErrorMessage)); On 2014/05/02 05:56:18, hashimoto wrote: > name and message are unused? Done. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.h (right): https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:14: class DictionaryValue; On 2014/05/02 05:56:18, hashimoto wrote: > This should be in the base namespace. Done. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:59: // a value to be returned by the caller (RunImpl). On 2014/05/02 05:56:18, hashimoto wrote: > Seems this function returns nothing? Done. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:62: // Fulfils the request with parsed arguments of this API function encapsulated On 2014/05/02 05:56:18, hashimoto wrote: > nit: s/Fulfils/Fulfills/ Done. https://codereview.chromium.org/260943002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.h:64: // will be called again for this request. Returns a value to be returned by On 2014/05/02 05:56:18, hashimoto wrote: > Seems this function returns nothing? Done.
lgtm https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:107: int file_system_id; nit: Please don't leave this uninitialized. https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:109: if (!args_->GetInteger(0, &file_system_id) || nit: How about using EXTENSION_FUNCTION_VALIDATE here?
https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:107: int file_system_id; On 2014/05/02 08:10:11, hashimoto wrote: > nit: Please don't leave this uninitialized. Done. https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:109: if (!args_->GetInteger(0, &file_system_id) || On 2014/05/02 08:10:11, hashimoto wrote: > nit: How about using EXTENSION_FUNCTION_VALIDATE here? It's not the same. We need to call SendResponse before returning.
https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc (right): https://codereview.chromium.org/260943002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc:109: if (!args_->GetInteger(0, &file_system_id) || On 2014/05/02 08:14:44, mtomasz wrote: > On 2014/05/02 08:10:11, hashimoto wrote: > > nit: How about using EXTENSION_FUNCTION_VALIDATE here? > > It's not the same. We need to call SendResponse before returning. Hmn, EXTENSION_FUNCTION_VALIDATE(Parse()) in RunImpl() is not possible because sometimes SendResponse(true) is called... lgtm
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/260943002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
Hunk #1 FAILED at 23.
Hunk #2 FAILED at 34.
Hunk #3 FAILED at 44.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc.rej
Patch:
chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
Index: chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
diff --git
a/chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
b/chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
index
bf3f8b443d3d7e6245557c5346c3675aba3e297f..2b41fe33a39c33818fa7319f2d4347b8699268b4
100644
--- a/chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc
@@ -23,7 +23,7 @@ BackendDelegate::~BackendDelegate() {}
fileapi::AsyncFileUtil* BackendDelegate::GetAsyncFileUtil(
fileapi::FileSystemType type) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
DCHECK_EQ(fileapi::kFileSystemTypeProvided, type);
return async_file_util_.get();
}
@@ -34,7 +34,7 @@ BackendDelegate::CreateFileStreamReader(
int64 offset,
const base::Time& expected_modification_time,
fileapi::FileSystemContext* context) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
DCHECK_EQ(fileapi::kFileSystemTypeProvided, url.type());
NOTIMPLEMENTED();
return scoped_ptr<webkit_blob::FileStreamReader>();
@@ -44,7 +44,7 @@ scoped_ptr<fileapi::FileStreamWriter>
BackendDelegate::CreateFileStreamWriter(
const fileapi::FileSystemURL& url,
int64 offset,
fileapi::FileSystemContext* context) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
DCHECK_EQ(fileapi::kFileSystemTypeProvided, url.type());
NOTIMPLEMENTED();
return scoped_ptr<fileapi::FileStreamWriter>();
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/260943002/100001
The CQ bit was unchecked by mtomasz@chromium.org
On 2014/05/02 13:56:34, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/mtomasz@chromium.org/260943002/100001 @benwells: PTAL at the tiny change in IDL.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/260943002/100001
Message was sent while issue was closed.
Change committed as 267896 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
