[webkit-reviews] review denied: [Bug 60218] [fileapi/Chromium] Clean up filesystem base path code : [Attachment 93206] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 16 15:03:07 PDT 2011
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Eric U.
<ericu at chromium.org>'s request for review:
Bug 60218: [fileapi/Chromium] Clean up filesystem base path code
https://bugs.webkit.org/show_bug.cgi?id=60218
Attachment 93206: Patch
https://bugs.webkit.org/attachment.cgi?id=93206&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93206&action=review
> Source/WebCore/platform/AsyncFileSystem.cpp:58
> + callbacks->didFail(NOT_SUPPORTED_ERR);
isn't it risky to run this callback synchronously? i'd worry about re-entrancy
bugs,
but maybe this is all dead code anyways?
> Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:225
> +KURL WorkerAsyncFileSystemChromium::virtualPathToFileSystemURL(const String&
virtualPath) const
this looks like duplicated code. is the intent to eventually delete this
function and its replica?
> Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:85
> + return WebKit::WebURL(url);
indentation nit.
also, please note that the copy-constructor performs a shallow copy of the
underlying
string containing the URL text. that shallow copy is implemented using a
reference
counting that is not thread safe! are you sure this is the proper way to copy
the
WebURL? it seems like you should use KURL::copy() here.
> Source/WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:58
> + return WebKit::WebURL(url);
indentation nit.
duplication of code nit.
same question about thread unsafe reference counting.
More information about the webkit-reviews
mailing list