Bug 47616 – Support typed arrays in workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Since ArrayBuffer will be used in FileReader/FileReaderSync, we need to support adding typed arrays in workers. This involves:
1) Exposing constructors in WorkerContext.
2) Adding NoStaticTables attributes.

[Jian Li]

Created attachment 70656 [details]
Proposed Patch

[Eric Seidel (no email)]

Attachment 70656 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4393008

[Jian Li]

Created attachment 70671 [details]
Proposed Patch

Fixed mac build error.

[Eric Seidel (no email)]

Attachment 70671 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4466012

[Jian Li]

Created attachment 70678 [details]
Proposed Patch

[David Levin]

Comment on attachment 70678 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70678&action=review

Consider removing the 3D_CANVAS in the Conditional in workers.

Also I see that the attributes are already out of sort order, but I think we can keep them mostly sorted with the moves that I suggested.

> WebCore/html/canvas/ArrayBuffer.idl:33
> +        NoStaticTables

I'd move NoStaticTables to be sorted.

Following the general idea: If ordering doesn't matter, sort.

> WebCore/html/canvas/ArrayBufferView.idl:27
> +    interface [Conditional=3D_CANVAS|BLOB, CustomToJS, OmitConstructor, NoStaticTables] ArrayBufferView {

Consider moving before OmitConstructor.

> WebCore/html/canvas/Float32Array.idl:33
> +        NoStaticTables,

After Generate...

> WebCore/workers/WorkerContext.idl:117
> +        attribute [Conditional=3D_CANVAS|BLOB,EnabledAtRuntime] ArrayBufferConstructor ArrayBuffer; // Usable with new operator

Why are we exposing these in Workers when 3d canvas is enabled?

[Alexey Proskuryakov]

> need to support adding typed arrays in workers. This involves:
> 1) Exposing constructors in WorkerContext.
> 2) Adding NoStaticTables attributes.

A general comment: exposing any feature in workers involves making sure that its implementation is thread safe. This is by far the most important and complicated step.

Watching the bugs, I'm not convinced that such analysis is being performed for each feature that's being added to workers.

[Jian Li]

(In reply to comment #7)
> > need to support adding typed arrays in workers. This involves:
> > 1) Exposing constructors in WorkerContext.
> > 2) Adding NoStaticTables attributes.
> 
> A general comment: exposing any feature in workers involves making sure that its implementation is thread safe. This is by far the most important and complicated step.
> 
> Watching the bugs, I'm not convinced that such analysis is being performed for each feature that's being added to workers.

Thanks a lot for reminding. We're not supporting passing type array objects between main thread and worker thread. We only allow them to be constructed and used in one single thread. I've checked the implementations and it seems to be fine for sole use in single thread. Anything else I need to analyze?

[Alexey Proskuryakov]

Yes, I also didn't spot anything suspicious in the implementation. Unlike a lot of WebCore code, it's not using any static data.

[Jian Li]

Fixed as suggested and committed as http://trac.webkit.org/changeset/69782.

[David Levin]

(In reply to comment #7)
> A general comment: exposing any feature in workers involves making sure that its implementation is thread safe. This is by far the most important and complicated step.
> 
> Watching the bugs, I'm not convinced that such analysis is being performed for each feature that's being added to workers.

Thanks for the reminder Alexey!

I've been trying to watch that (but in honestly for this one in particular I didn't look at closely as I should have at all of the under lying implementation until your reminder -- even thought it turned out alright).

I'll make a special note each time I review one of them that I did this, so anyone scanning reviews can see what outside of the sparse comments was done.