Don't expose nonstandard FileSystem API to Service Workers

Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
Committed: https://crrev.com/8da767715a0035b7cd47e0cc8336c46db89295c7
Cr-Commit-Position: refs/heads/master@{#373842}

[jsbell, 2016-02-04 21:12:56]

Description was changed from

==========
Don't expose deprecated FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
R=michaeln@chromium.org,kinuko@chromium.org,haraken@chromium.org
==========

to

==========
Don't expose deprecated FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

[jsbell, 2016-02-04 21:12:56]

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

[jsbell, 2016-02-04 21:18:10]

Please take a look? I could split the bindings codegen and FileError change out
into a separate CL and land that first, if desired, but it needed some context.

haraken@ - can you review bindings codegen changes? it works but it could
probably be improved

kinuko@ - can you review FileSystem and ServiceWorker changes/tests?

philipj@ - does this IDL twiddling make sense? (i.e. scoping down what's exposed
where)

michaeln@ - general sanity checking?

[jsbell, 2016-02-04 21:18:32]

Description was changed from

==========
Don't expose deprecated FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

to

==========
Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

[haraken, 2016-02-04 23:58:29]

bindings/ LGTM

[michaeln, 2016-02-05 01:10:27]

this has never worked in serviceworkers, right?
sanity modulo bindings voodoo generally lgtm ;)

[kinuko, 2016-02-05 04:26:31]

Whoa, a bit of hassle.  Thanks for fixing! lgtm

[philipj_slow, 2016-02-05 07:12:36]

LGTM to unexposing this wherever possible. But shouldn't
[Exposed=(Window,Worker)] expose on *Worker? It looks like that from
v8_utilities.py, at least. Would using
[Exposed=(Window,DedicatedWorker,SharedWorker)] achieve the same thing?

[jsbell, 2016-02-05 17:10:10]

On 2016/02/05 07:12:36, philipj_UTC7 wrote:
> LGTM to unexposing this wherever possible. But shouldn't
> [Exposed=(Window,Worker)] expose on *Worker? It looks like that from
> v8_utilities.py, at least. Would using
> [Exposed=(Window,DedicatedWorker,SharedWorker)] achieve the same thing?

For FileError ? That's actually part of "File API" (although it's been removed
from the spec in favor of just DOMError), and used in FileReader which is
exposed to SW (for reading blobs, etc)

It should have been exposed to Worker all along; my guess is our impl predated
[Exposed] since it's a VERY old Worker API.

[jsbell, 2016-02-05 17:10:19]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-02-05 17:11:37]

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

[commit-bot: I haz the power, 2016-02-05 17:56:55]

Description was changed from

==========
Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

to

==========
Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

[commit-bot: I haz the power, 2016-02-05 17:56:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-05 17:58:17]

Description was changed from

==========
Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052
==========

to

==========
Don't expose nonstandard FileSystem API to Service Workers

Chrome's FileSystem API is deprecated/nonstandard and we should not
expose it to new contexts like Service Workers.

The fix ended up being a bit involved:

* [Exposed] can't be used on members of partial interfaces defined in
modules/ when the base interface is in core/ (see
https://goo.gl/DfPWTV)

* So instead split WorkerFileSystemGlobalScope into DedicateWorker...
and SharedWorker... versions.

* But that revealed a subtle bindings generator bug where an unused
function would be generated for
WorkerGlobalScopeConstructorAttributeSetterCallback due to the
FileErrorConstructor attribute disappearing from WorkerGlobalScope. So
fix that.

* And that pointed out that the FileErrorConstructor shouldn't be
exposed that way anyway, so expose that with [Exposed=(Window,Worker)]
instead.

* ... which correctly removes it from the CompositorWorker, which
shouldn't have inherited it anyway because it has
[Global=CompositorWorker]

BUG=584052

Committed: https://crrev.com/8da767715a0035b7cd47e0cc8336c46db89295c7
Cr-Commit-Position: refs/heads/master@{#373842}
==========

[commit-bot: I haz the power, 2016-02-05 17:58:18]

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

[philipj_slow, 2016-02-05 19:28:40]

On 2016/02/05 17:10:10, jsbell wrote:
> On 2016/02/05 07:12:36, philipj_UTC7 wrote:
> > LGTM to unexposing this wherever possible. But shouldn't
> > [Exposed=(Window,Worker)] expose on *Worker? It looks like that from
> > v8_utilities.py, at least. Would using
> > [Exposed=(Window,DedicatedWorker,SharedWorker)] achieve the same thing?
> 
> For FileError ? That's actually part of "File API" (although it's been removed
> from the spec in favor of just DOMError), and used in FileReader which is
> exposed to SW (for reading blobs, etc)
> 
> It should have been exposed to Worker all along; my guess is our impl predated
> [Exposed] since it's a VERY old Worker API.

Oh, right, FileError is used elsewhere :)