[webkit-reviews] review denied: [Bug 89981] Add tests to verify Write operation is disallowed in dropped filesystem : [Attachment 149535] Patch

[bugzilla-daemon at webkit.org]

Yuta Kitamura <yutak at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 89981: Add tests to verify Write operation is disallowed in dropped
filesystem
https://bugs.webkit.org/show_bug.cgi?id=89981

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

------- Additional Comments from Yuta Kitamura <yutak at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149535&action=review


It seems the test is well written. I just have a bunch of style issues. r-
because of them.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:43
> +function expect(value, msg)

nit: msg -> message?

A few other functions that also take |msg| should be fixed, too.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:65
> +function startTest(e)

nit: e -> event, maybe?

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:74
> +	 expect('file' == items[i].kind, 'kind is "file"');

Wrong indent.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:117
> +    window[test](entry, function() { testFile(entry, testIndex+1, callback);
});

nit: spaces needed around a "+". Also in line 128.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:143
> +    entry.getFile('child-file', {create:true}, unexpectSuccess(),
expectError(callback, 'getFile(create:true)'));

"{ create: true }" may be better (spaces within braces). I'm not really sure if
this is a common WebKit style, but at least you need to be self-consistent with
your code in line 11-12.

This also happens below (in line 148).

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:189
> +		 expect(progress.target.error.code == 2, 'writer: Got expected
security error');

Indent.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:201
> +	   expect(error.code == FileError.SECURITY_ERR || error.code ==
FileError.INVALID_MODIFICATION_ERR, msg + ': got expected error');

Are these error codes (SECURITY_ERR or INVALID_MODIFICATION_ERR) returned in a
deterministic way? If so, it's probably better to log the value of
|error.code|, too.

> LayoutTests/fast/filesystem/dropped-filesystem-non-writable.html:206
> +function unexpectSuccess()

"successUnexpectedly()" or "unexpectedSuccess()" may sound better. "Unexpect"
does not seem a valid verb.