Summary: | Support typed arrays in workers | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||
Component: | WebCore JavaScript | Assignee: | Jian Li <jianli> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, ap, eric, levin | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jian Li
2010-10-13 13:28:55 PDT
Created attachment 70656 [details]
Proposed Patch
Attachment 70656 [details] did not build on mac: Build output: http://queues.webkit.org/results/4393008 Created attachment 70671 [details]
Proposed Patch
Fixed mac build error.
Attachment 70671 [details] did not build on mac: Build output: http://queues.webkit.org/results/4466012 Created attachment 70678 [details]
Proposed Patch
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? > 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.
(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? Yes, I also didn't spot anything suspicious in the implementation. Unlike a lot of WebCore code, it's not using any static data. Fixed as suggested and committed as http://trac.webkit.org/changeset/69782. (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. |