[webkit-reviews] review denied: [Bug 47680] Add FileWriter test utilities. : [Attachment 73315] 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 47680: Add FileWriter test utilities.
https://bugs.webkit.org/show_bug.cgi?id=47680

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

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

It looks like the .html files should be generated like what happens normally
for script tests. Is this a future change?

Please consider addressing the successfully parsed issue (and removing alert :)
), and this will be good to go.

> LayoutTests/ChangeLog:8
> +	   Cleaning up shared test utility files caused the includes and output
of a bunch of existing tests change slightly, so I've rebuilt the expectation
files.

We usually wrap lines in ChangeLog before they get this long.

> LayoutTests/fast/filesystem/resources/file-from-file-entry-sync.js:20
> +finishJSTest();

This looks wrong here and in the other places where this happened.

Typically 
 var successfullyParsed = true;
is last so that it is clear the whole file was parsed correctly (and it would
be checked in js-test-post.js).

> LayoutTests/fast/filesystem/resources/fs-worker-common.js:8
> +    shouldBeTrue("successfullyParsed");

As mentioned before, I think this should be done elsewhere.

> LayoutTests/fast/filesystem/resources/fs-worker-test-util.js:22
> +	   alert(event);

alert?