Bug 72783 - Get rid of WebCore dependencies from TypedArray implementation types
Summary: Get rid of WebCore dependencies from TypedArray implementation types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-18 18:19 PST by Oliver Hunt
Modified: 2011-11-23 12:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.32 KB, patch)
2011-11-18 18:19 PST, Oliver Hunt
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Oliver's patch restructured (13.24 KB, patch)
2011-11-22 16:19 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-11-18 18:19:12 PST
Get rid of WebCore dependencies from TypedArray implementation types
Comment 1 Oliver Hunt 2011-11-18 18:19:59 PST
Created attachment 115922 [details]
Patch
Comment 2 Oliver Hunt 2011-11-18 18:20:43 PST
Needs V8 side to be implemented and i can't work out how that should be done.
Comment 3 Adam Barth 2011-11-18 18:33:27 PST
I'm happy to help.  What needs to be done?  Should I apply this patch and try to make it build?
Comment 4 Oliver Hunt 2011-11-18 18:48:53 PST
(In reply to comment #3)
> I'm happy to help.  What needs to be done?  Should I apply this patch and try to make it build?

Making it build would be easy.  Making it correct less so, it really needs dslomov to do the v8 side i think.
Comment 5 Adam Barth 2011-11-18 18:57:16 PST
@dslomov: Would you be willing to help Oliver with this patch?
Comment 6 WebKit Review Bot 2011-11-18 20:00:08 PST
Comment on attachment 115922 [details]
Patch

Attachment 115922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10518300
Comment 7 Oliver Hunt 2011-11-18 20:21:30 PST
(In reply to comment #6)
> (From update of attachment 115922 [details])
> Attachment 115922 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10518300

Least surprising build failure ever :D
Comment 8 Dmitry Lomov 2011-11-21 10:43:28 PST
Comment on attachment 115922 [details]
Patch

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

I think it is a reasonable approach. Some nits below. Once they are fixed, I'll be happy to implement a V8 side of these changes.

> Source/WebCore/ChangeLog:19
> +        (WTF::ArrayBufferBinding::poison):

Spec name for this operation is "neuter" (http://dev.w3.org/html5/spec/common-dom-interfaces.html#transferable-objects) - let's keep it!

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:10650
> +		A70E3252147744E7005E2988 /* CodeGeneratorV8.pm */ = {isa = PBXFileReference; lastKnownFileType = text.script.perl; name = CodeGeneratorV8.pm; path = scripts/CodeGeneratorV8.pm; sourceTree = "<group>"; };

CodeGeneratorV8.pm is not used in apple port of webkit - do not reference it from project files.

> Source/WebCore/html/canvas/ArrayBuffer.cpp:141
>          ArrayBufferView* current = m_firstView;

There is no more reason to keep a list of ArrayBufferViews - we should just remove them.
Comment 9 Dmitry Lomov 2011-11-22 13:56:13 PST
Hmm I have thought on this more, and I really do not see a good way to make this approach work for V8. The problem is, V8 bindings does not really have an equivalent of JS* classes to stick an m_binding member on.

For example, JUint8Array is a generated class that represents JSC object corresponding to Uint8Array. V8 just have a generic v8::JSObject and a bunch of callbacks for this. There is no instances of 'V8Uint8Array'.

So it looks like the better solution would be this:
- expose the list of views for array buffer on ArrayBuffer class
- ArrayBuffer::transfer shouldn't attempt to neuter bindings (it should still neuter views by setting everything to zero)
- let the callers to ArrayBuffer (namely SerializedScriptValue) deal with neutering the bindings, iteration over exposed list of views.

I will prepare a patch along these lines.
Comment 10 Dmitry Lomov 2011-11-22 16:19:51 PST
Created attachment 116289 [details]
Oliver's patch restructured

Forego trying to sneak any callbacks from WebCore into WTF's ArrayBuffer.
Instead, return list of neutered views from ArrayBuffer::transfer and let clients (WebCore's SerializedScriptValue) deal with neutering bindings as they see fit.
Comment 11 WebKit Review Bot 2011-11-23 04:07:07 PST
Comment on attachment 116289 [details]
Oliver's patch restructured

Clearing flags on attachment: 116289

Committed r101064: <http://trac.webkit.org/changeset/101064>
Comment 12 WebKit Review Bot 2011-11-23 04:07:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Oliver Hunt 2011-11-23 09:26:16 PST
How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?
Comment 14 Dmitry Lomov 2011-11-23 10:53:13 PST
(In reply to comment #13)
> How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?

The neutering will be handled by a caller to ArrayBuffer::transfer, as in

SerializedScriptValue::create(...)
{
    ...
    Vector<ArrayBufferView*> viewsToNeuter;
    arrayBuffer.transfer(..., viewsToNeuter);
    for(view in viewsToNeuter) {
        get a binding for view and neuter it
    }
    ...
}

SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.
Comment 15 Oliver Hunt 2011-11-23 12:32:07 PST
(In reply to comment #14)
> (In reply to comment #13)
> > How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?
> 
> The neutering will be handled by a caller to ArrayBuffer::transfer, as in
> 
> SerializedScriptValue::create(...)
> {
>     ...
>     Vector<ArrayBufferView*> viewsToNeuter;
>     arrayBuffer.transfer(..., viewsToNeuter);
>     for(view in viewsToNeuter) {
>         get a binding for view and neuter it
>     }
>     ...
> }
> 
> SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.

The thing to be careful of is that you have to neuter all bindings in all worlds that may reference the buffer.

Once again I'll say that i think this is a poorly conceived API, that will result in substantial programmer confusion in the long term.
Comment 16 Dmitry Lomov 2011-11-23 12:41:16 PST
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > How does this handle neutering of the binding object?  afaict you've simply removed the logic that handled bindings entirely -- what have i missed?
> > 
> > The neutering will be handled by a caller to ArrayBuffer::transfer, as in
> > 
> > SerializedScriptValue::create(...)
> > {
> >     ...
> >     Vector<ArrayBufferView*> viewsToNeuter;
> >     arrayBuffer.transfer(..., viewsToNeuter);
> >     for(view in viewsToNeuter) {
> >         get a binding for view and neuter it
> >     }
> >     ...
> > }
> > 
> > SerializedScriptValue lives in WebCore and knows all about bindings and JavaScript, so it is in better position to propagate neutering to bindings.
> 
> The thing to be careful of is that you have to neuter all bindings in all worlds that may reference the buffer.
> 
> Once again I'll say that i think this is a poorly conceived API, that will result in substantial programmer confusion in the long term.

Which do you think a poorly conceived API? The transfer method on ArrayBuffer or the postMessage transferrable API?

For the transfer method on ArrayBuffer, I think this is the best way forward if you want ArrayBuffer to live in WTF and not depend on WebCore.