Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com

Committed: https://crrev.com/d58c42982f7f11450533250d8deedea8afc89f9e
Cr-Commit-Position: refs/heads/master@{#363008}

[jsbell, 2015-12-01 00:24:43]

jsbell@chromium.org changed reviewers:
+ philipj@opera.com
- philipj@chromium.org

[jsbell, 2015-12-01 00:24:44]

philipj@ - please take a look?

[philipj_slow, 2015-12-01 08:45:59]

Description was changed from

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

BUG=561338
R=philipj@chromium.org
==========

to

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

BUG=561338
R=philipj@opera.com
==========

[philipj_slow, 2015-12-01 09:00:25]

Yay, thanks for fixing! LGTM with small nits.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html (right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html:12:
debug("Error occured:" + error.name);
space after comma makes output nicer

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html:24:
window.entry = entry;
entry isn't used in any later should*() so I don't think it needs to sit on the
global object.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt:7:
PASS [Worker] fileSystem.root.moveTo(null) threw exception TypeError: Failed to
execute 'moveTo' on 'EntrySync': 2 arguments required, but only 1 present..
This isn't actually testing the new TypeError, needs the second argument. To
make sure nothing was overlooked, maybe verify that all new tests fail without
the IDL change.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/DevToolsHostFileSystem.idl
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/DevToolsHostFileSystem.idl:9: void
upgradeDraggedFileSystemPermissions(DOMFileSystem domFileSystem);
Well this was odd, would this previously just crash at
DevToolsHostFileSystem.cpp:30?

[jsbell, 2015-12-01 18:19:44]

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt:7:
PASS [Worker] fileSystem.root.moveTo(null) threw exception TypeError: Failed to
execute 'moveTo' on 'EntrySync': 2 arguments required, but only 1 present..
On 2015/12/01 09:00:24, philipj wrote:
> This isn't actually testing the new TypeError, needs the second argument. To
> make sure nothing was overlooked, maybe verify that all new tests fail without
> the IDL change.

Thanks for catching this... the IDL doesn't match the (dead) spec there;
EntrySync's second arg is supposed to be `optional` too. And non-nullable - this
is really crufty code. :P

[philipj_slow, 2015-12-01 19:00:58]

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt:7:
PASS [Worker] fileSystem.root.moveTo(null) threw exception TypeError: Failed to
execute 'moveTo' on 'EntrySync': 2 arguments required, but only 1 present..
On 2015/12/01 18:19:44, jsbell wrote:
> On 2015/12/01 09:00:24, philipj wrote:
> > This isn't actually testing the new TypeError, needs the second argument. To
> > make sure nothing was overlooked, maybe verify that all new tests fail
without
> > the IDL change.
> 
> Thanks for catching this... the IDL doesn't match the (dead) spec there;
> EntrySync's second arg is supposed to be `optional` too. And non-nullable -
this
> is really crufty code. :P

FileSystem sure needs some love to either revive or kill it, but to be fair code
all over the place gets optionality and nullability wrong :)

[jsbell, 2015-12-01 19:50:32]

So there's a wrinkle...

The moveTo/copyTo methods on Entry (not EntrySync) report invalid arguments
asynchronously today (via the errorCallback), so this would be a behavior change
if we move the validation to the bindings layer.

Three options:

(1) Just stick with [LegacyInterfaceTypeChecking] here
(2) Make the change per this CL, and risk breaking sites
(3) Change the IDL to be DirectoryEntry? (nullable) to maintain compatibility
(and add tests!)

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html (right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html:12:
debug("Error occured:" + error.name);
On 2015/12/01 09:00:24, philipj wrote:
> space after comma makes output nicer

Done.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/null-arguments.html:24:
window.entry = entry;
On 2015/12/01 09:00:24, philipj wrote:
> entry isn't used in any later should*() so I don't think it needs to sit on
the
> global object.

Done.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/filesystem/workers/null-arguments-expected.txt:7:
PASS [Worker] fileSystem.root.moveTo(null) threw exception TypeError: Failed to
execute 'moveTo' on 'EntrySync': 2 arguments required, but only 1 present..
On 2015/12/01 09:00:24, philipj wrote:
> This isn't actually testing the new TypeError, needs the second argument. To
> make sure nothing was overlooked, maybe verify that all new tests fail without
> the IDL change.

Done.

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/filesystem/DevToolsHostFileSystem.idl
(right):

https://codereview.chromium.org/1480363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/filesystem/DevToolsHostFileSystem.idl:9: void
upgradeDraggedFileSystemPermissions(DOMFileSystem domFileSystem);
On 2015/12/01 09:00:24, philipj wrote:
> Well this was odd, would this previously just crash at
> DevToolsHostFileSystem.cpp:30?

Presumably. This is internal inspector code (not web-facing), and the only
actual call appears to be at devtools/front_end/sources/SourcesPanel.js:1215 and
it looks like it would never pass in null.

[philipj_slow, 2015-12-01 20:12:20]

On 2015/12/01 19:50:32, jsbell wrote:
> So there's a wrinkle...
> 
> The moveTo/copyTo methods on Entry (not EntrySync) report invalid arguments
> asynchronously today (via the errorCallback), so this would be a behavior
change
> if we move the validation to the bindings layer.
> 
> Three options:
> 
> (1) Just stick with [LegacyInterfaceTypeChecking] here
> (2) Make the change per this CL, and risk breaking sites
> (3) Change the IDL to be DirectoryEntry? (nullable) to maintain compatibility
> (and add tests!)

Making it nullable and removing [LegacyInterfaceTypeChecking] would actually
also change behavior, it would start throwing if you pass in {} or any object of
the wrong type.

Indeed it looks like moveTo/copyTo would previously never throw, except I
suppose if the second argument is `{ toString: function() { throw new
TypeError() } }`.

Since the other changes are low-risk, how about landing those first? For
moveTo/copyTo, there's also the option of making it type `any` in the IDL, if
this ends up being the only thing blocking the removal of
[LegacyInterfaceTypeChecking].

[philipj_slow, 2015-12-01 22:13:29]

Um, actually, moveTo/copyTo already throw exceptions if the callbacks are
anything other than null, undefined or a function. Given this, I think I'd favor
making this throw in bindings as well, but probably still as a separate CL for
revertability.

[jsbell, 2015-12-02 22:51:13]

This removes the changes for the async moveTo/copyTo methods.

Without the code changes:

Sync moveTo() fails w/ throwing InvalidModificationError
Sync copyTo() fails w/ throwing InvalidModificationError
Sync write() fails w/ TypeMismatchError
Async write() fails w/ TypeMismatchError

With the code change, all of the above fail w/ TypeError (as expected)

There is still one other behavior change: in the async write case the writer's
`error` attribute is (synchronously) set to the TypeMismatchError, so the test
reports:

FAIL writer.error should be null. Was [object FileError].

Should we keep that in this CL or break that out as well?

[philipj_slow, 2015-12-03 09:21:37]

On 2015/12/02 22:51:13, jsbell wrote:
> This removes the changes for the async moveTo/copyTo methods.
> 
> Without the code changes:
> 
> Sync moveTo() fails w/ throwing InvalidModificationError
> Sync copyTo() fails w/ throwing InvalidModificationError
> Sync write() fails w/ TypeMismatchError
> Async write() fails w/ TypeMismatchError
> 
> With the code change, all of the above fail w/ TypeError (as expected)
> 
> There is still one other behavior change: in the async write case the writer's
> `error` attribute is (synchronously) set to the TypeMismatchError, so the test
> reports:
> 
> FAIL writer.error should be null. Was [object FileError].
> 
> Should we keep that in this CL or break that out as well?

OK, so this is the removed `setError(FileError::TYPE_MISMATCH_ERR,
exceptionState)` in FileWriter::write. I think that change in behavior seems
very low risk, it wouldn't make lot of sense to pass in a non-Blob, catch the
exception *and* depend on writer.error being set to figure out what went wrong.
Exception+error attribute is an odd mix. LGTM to land together, but if you want
to split it out to isolate the risk, that WFM too.

[jsbell, 2015-12-03 17:04:57]

On 2015/12/03 09:21:37, philipj wrote:
> On 2015/12/02 22:51:13, jsbell wrote:
> > This removes the changes for the async moveTo/copyTo methods.
> > 
> > Without the code changes:
> > 
> > Sync moveTo() fails w/ throwing InvalidModificationError
> > Sync copyTo() fails w/ throwing InvalidModificationError
> > Sync write() fails w/ TypeMismatchError
> > Async write() fails w/ TypeMismatchError
> > 
> > With the code change, all of the above fail w/ TypeError (as expected)
> > 
> > There is still one other behavior change: in the async write case the
writer's
> > `error` attribute is (synchronously) set to the TypeMismatchError, so the
test
> > reports:
> > 
> > FAIL writer.error should be null. Was [object FileError].
> > 
> > Should we keep that in this CL or break that out as well?
> 
> OK, so this is the removed `setError(FileError::TYPE_MISMATCH_ERR,
> exceptionState)` in FileWriter::write. I think that change in behavior seems
> very low risk, it wouldn't make lot of sense to pass in a non-Blob, catch the
> exception *and* depend on writer.error being set to figure out what went
wrong.
> Exception+error attribute is an odd mix. LGTM to land together, but if you
want
> to split it out to isolate the risk, that WFM too.

Agreed. Landing as-is.

[jsbell, 2015-12-03 17:05:48]

Description was changed from

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

BUG=561338
R=philipj@opera.com
==========

to

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com
==========

[jsbell, 2015-12-03 17:05:55]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-12-03 17:06:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1480363003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1480363003/60001

[commit-bot: I haz the power, 2015-12-03 18:10:07]

Description was changed from

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com
==========

to

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com
==========

[commit-bot: I haz the power, 2015-12-03 18:10:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-03 18:10:47]

Description was changed from

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com
==========

to

==========
Eliminate [LegacyInterfaceTypeChecking] for modules/filesystem bindings

Let bindings generate a TypeError, rather than the module code
producing a TypeMismatchError or InvalidModificationError, if null is
passed for non-nullable interface arguments.

This was previously untested, so new tests were added for the async
API and sync API (in worker) cases.

(Some async cases are not modified in this CL, as there is a
behavior change we want to tackle separately.)

BUG=561338
R=philipj@opera.com

Committed: https://crrev.com/d58c42982f7f11450533250d8deedea8afc89f9e
Cr-Commit-Position: refs/heads/master@{#363008}
==========

[commit-bot: I haz the power, 2015-12-03 18:10:48]

Patchset 4 (id:??) landed as
https://crrev.com/d58c42982f7f11450533250d8deedea8afc89f9e
Cr-Commit-Position: refs/heads/master@{#363008}