[webkit-reviews] review denied: [Bug 47681] [Chromium] implementation of async FileWriter for workers : [Attachment 70899] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 47681: [Chromium] implementation of async FileWriter for workers
https://bugs.webkit.org/show_bug.cgi?id=47681

Attachment 70899: Patch
https://bugs.webkit.org/attachment.cgi?id=70899&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70899&action=review

Meta comment:
Pulling things into smaller patches is better.

You get things in faster because the reviewer can often approve simple things
quickly and the complicated things get the attention they need without the
added distraction of other stuff.

Also when you go through multiple rounds of reviews, it means that the reviewer
needs to look over a bunch of cruft everytime that while simple -- still needs
to be looked at everytime. (We don't have nice intra-patch diffing in WebKit
land).

So please consider making patches as small as possible and breaking up
independent pieces.

Initial brief comments on the patch.

> WebCore/fileapi/FileInfo.h:38
> +struct FileInfo {

WebKit tries to avoid abbreviations "Info".

FileMetadata ?

> WebCore/fileapi/FileInfo.h:62
> +#endif // DOMFileSystem_h

This comment is incorrect.

> WebCore/fileapi/FileSystemCallbacks.h:37
> +#include "FileInfo.h"

Don't include. Use a fwd decl.

> WebCore/platform/AsyncFileSystemCallbacks.h:36
> +#include "FileInfo.h"

Avoid include. Use fwd decl.

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.h:69
> +class WorkerFileWriterCallbacksBridge : public
ThreadSafeShared<WorkerFileWriterCallbacksBridge>, public
WebCore::WorkerContext::Observer, public WebFileWriterClient {

Any ThreadSafeShared class that owns strings bothers me.

ThreadSafeShared basically means that it can be destructed on multiple threads.


String is not designed to be used on multiple threads. It takes very very
careful coding to get this right.

Then even if you get it right, any maintainance patches need to get it right
(and they won't because it is too subtle).

In short, I think this is fundamentally flawed.