Bug 149951 – [Streams API] Add skeleton for initial WritableStream support

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[Streams API] Add skeleton for initial WritableStream support

[Xabier Rodríguez Calvar]

Created attachment 262763 [details]
Patch

I am uploading this patch though I know compilation will fail in Mac because the generators can't handle an empty js file. At this point I could just add a phony function or fix the generators so that we don't hit those errors in Mac. Darin, wdyt? Besides this, patch will need rebase after Youenn lands bug 149751.

[Xabier Rodríguez Calvar]

Adding initialization already would require some more refactorings and patch would be much bigger. That's why I thought it was better to keep it small.

[Darin Adler]

Looks OK. Not sure I know what question you are asking me, though. I’d prefer not to review something until it’s compiling and OK to land.

I don’t see anything obviously wrong here except for all the places we have to modify every time we add a class, and I am hoping the work in bug 149751 takes care of most if not all of that.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> Looks OK. Not sure I know what question you are asking me, though. I’d
> prefer not to review something until it’s compiling and OK to land.

Yes, I didn't explain it properly. The issue is that there's a bug in the generators that creates some warnings because of unused attributes and parameters that in Mac become errors, hence making the build fail. The bug happens when you add to a .js file which is supposed to have internal functions but it is still empty (as in this case we we are adding a skeleton.

We have several choices to overcome this:

1) Add an empty phony function that I would remove later that avoids these errors.
2) Fix the generators so that you can have empty files for internals
3) Exclude this file for the first skeleton. This seems to be the most logical, so I'll take it for now unless you tell me otherwise.

> I don’t see anything obviously wrong here except for all the places we have
> to modify every time we add a class, and I am hoping the work in bug 149751
> takes care of most if not all of that.

Me too, I am just about to have a look at the status now.

[Xabier Rodríguez Calvar]

Created attachment 263006 [details]
Patch

Patch without the internals building in Mac too.

[Build Bot]

Comment on attachment 263006 [details]
Patch

Attachment 263006 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/280243

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263008 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263006 [details]
Patch

Attachment 263006 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/280247

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 263010 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Xabier Rodríguez Calvar]

(In reply to comment #8)
> Comment on attachment 263006 [details]
> Patch
> 
> Attachment 263006 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/280247
> 
> New failing tests:
> js/dom/global-constructors-attributes.html

Would it be OK if I fix this at landing time?

[Alex Christensen]

(In reply to comment #10)
> Would it be OK if I fix this at landing time?
What is the fix?  It's not entirely obvious to me how this test is related.

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> (In reply to comment #10)
> > Would it be OK if I fix this at landing time?
> What is the fix?  It's not entirely obvious to me how this test is related.

The fix is putting WritableStream in the constructor list as it is a new object.

[Xabier Rodríguez Calvar]

Created attachment 263067 [details]
Patch

Updated global-constructors-attributes expectations.

[youenn fablet]

Comment on attachment 263067 [details]
Patch

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

It is good to see WS going live, even if not yet full of potato chunks ;)
Some "curiosity" questions below.

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/Models/NativeFunctionParameters.js: Added support for WritableStream.

What is the purpose of this addition?
Should we keep ReadableStream in sync as well?

> Source/WebCore/Modules/streams/WritableStream.idl:33
> +] interface WritableStream { 

Maybe good to fill the whole WritableStream IDL file in one patch (with notimplemented js functions in the back) in a follow-up patch?

> LayoutTests/streams/reference-implementation/brand-checks-expected.txt:5
> +PASS ReadableStream.prototype.getReader enforces a brand check 

It is surprising to need WritableStream to pass these tests.
Should these tests be written differently so that they pass even if WS is not implemented?
No need to do that within WebKit.
I might file a bug on streams github.

> LayoutTests/streams/reference-implementation/brand-checks-expected.txt:8
> +PASS ReadableStream.prototype.tee enforces a brand check 

Ditto.

[Xabier Rodríguez Calvar]

(In reply to comment #14)
> It is good to see WS going live, even if not yet full of potato chunks ;)
> Some "curiosity" questions below.

There'll be potatos, no doubt!

> > Source/WebInspectorUI/ChangeLog:8
> > +        * UserInterface/Models/NativeFunctionParameters.js: Added support for WritableStream.
> 
> What is the purpose of this addition?
> Should we keep ReadableStream in sync as well?

I think it is something related to the WebInspector to give information about the parameters taken by the functions.

I think RS should be in sync. Actually I assumed it was, though I haven't checked it.

> > Source/WebCore/Modules/streams/WritableStream.idl:33
> > +] interface WritableStream { 
> 
> Maybe good to fill the whole WritableStream IDL file in one patch (with
> notimplemented js functions in the back) in a follow-up patch?

Sure, it can be done.

> > LayoutTests/streams/reference-implementation/brand-checks-expected.txt:5
> > +PASS ReadableStream.prototype.getReader enforces a brand check 
> 
> It is surprising to need WritableStream to pass these tests.
> Should these tests be written differently so that they pass even if WS is
> not implemented?
> No need to do that within WebKit.
> I might file a bug on streams github.

I also thought it was weird, but I didn't question that. Yeah, feel free to file the bug and put me as watcher (or whatever).

Btw, it seems that tests have been migrated from tape to testharness so I guess we'll have to keep in mind a big sync soon.

[Xabier Rodríguez Calvar]

Created attachment 263144 [details]
Patch

Added a skeleton for methods and attributes. Those raise an exception to prevent any false PASS that I observed that could happen if I left them empty.

[youenn fablet]

Comment on attachment 263144 [details]
Patch

LGTM.
Using EvalError for 'not implemented' seems a bit strange.
But using @TypeError as well I guess, and this is temporary.

[Joseph Pecoraro]

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

> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:1267
> +        close: null,

This line can just be removed if close takes no parameters, or if you wish the empty string may be better.

--


> > > Source/WebInspectorUI/ChangeLog:8
> > > +        * UserInterface/Models/NativeFunctionParameters.js: Added support for WritableStream.
> > 
> > What is the purpose of this addition?
> > Should we keep ReadableStream in sync as well?
> 
> I think it is something related to the WebInspector to give information about the parameters taken by the functions.

Correct. This way, if these Prototype functions ever appears in an ObjectTree in the inspector (e.g. the Console), we will be able to show the parameters this function takes in a rich way. Basically API documentation in the console. This is automatic for "user-space" functions which can be toString'ed and have their parameters parsed, but native functions don't have that information. We maintain have this list to provide that data.

[Xabier Rodríguez Calvar]

Created attachment 263250 [details]
Patch

Fixed closed paramters for the inspector.

[youenn fablet]

Albeit EvalError that we already discussed, there are two editorial nits that could be changed at landing time:

In WritableStream.idl: remove [Default=Undefined] in abort function.
In Source/WebCore/bindings/js/WebCoreJSBuiltins.h, might want to add a space before the '}' for all builtin getters.

LGTM then :)

(In reply to comment #18)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263144&action=review
> 
> > Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:1267
> > +        close: null,
> 
> This line can just be removed if close takes no parameters, or if you wish
> the empty string may be better.

I see.
ReadableStream is still in sync with the IDL then although we could use the empty string also for missing functions.
ReadableStreamReader and ReadableStreamController do not seem to be there yet.

> Correct. This way, if these Prototype functions ever appears in an
> ObjectTree in the inspector (e.g. the Console), we will be able to show the
> parameters this function takes in a rich way. Basically API documentation in
> the console. This is automatic for "user-space" functions which can be
> toString'ed and have their parameters parsed, but native functions don't
> have that information. We maintain have this list to provide that data.

It seems most (if not all of) the WebCore part of this information would be easily generated from WebIDL files.
Is it worth doing so?

[Xabier Rodríguez Calvar]

(In reply to comment #20)
> Albeit EvalError that we already discussed, there are two editorial nits
> that could be changed at landing time:

Yes, still waiting for others' opinion on this.

> In WritableStream.idl: remove [Default=Undefined] in abort function.

I don't agree. I think we should keep it the same way cancel has it in RS.

> In Source/WebCore/bindings/js/WebCoreJSBuiltins.h, might want to add a space
> before the '}' for all builtin getters.

I could do this, yes.

> LGTM then :)

LGTM too :-p

[Darin Adler]

Comment on attachment 263250 [details]
Patch

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

Looks like my comments overlap with the ones from Youenn!

Comments generally are for the project overall and not necessarily pointed at you, Xabier, or something we necessarily have to deal with in this particular patch.

> Source/WebCore/Modules/streams/WritableStream.idl:41
> +[
> +    Conditional=STREAMS_API,
> +    JSBuiltinConstructor
> +] interface WritableStream { 
> +    [JSBuiltin] Promise abort([Default=Undefined] optional any reason);
> +    [JSBuiltin] Promise close();
> +    [JSBuiltin] Promise write(any chunk);
> +
> +    [JSBuiltin] readonly attribute Promise closed;
> +    [JSBuiltin] readonly attribute Promise ready;
> +    [JSBuiltin] readonly attribute DOMString state;
> +};

We should probably consider syntax that makes the entire class, constructor, methods, and attribute getters and setters all JavaScript by default (a class attribute or whatever we call those things). It seems likely that there will be classes that are mostly JavaScript and classes that are mostly C++, and in both cases it would be unfortunate to have to decorate every single line like this.

> Source/WebCore/bindings/js/WebCoreJSBuiltins.h:37
> +#include "WritableStreamBuiltinsWrapper.h"

Are we working on something that will eliminate the need for all this boilerplate? It continues to vex me!

> Source/WebCore/bindings/js/WebCoreJSBuiltins.h:70
> +    WritableStreamBuiltinsWrapper& writableStreamBuiltins() { return m_writableStreamBuiltins;}

A tiny bit sloppy that this line and the others just above it have no space after the semicolon.

> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:1270
> +    WritableStream: {
> +        abort: "reason",
> +        close: "",
> +        write: "chunk",
> +        __proto__: null,
> +    },

We should figure out a way to generate what’s needed in this file from IDL instead of having yet another thing we have to modify every time we change a class. How would a future contributor know they had to come here if they were adding to IDL.

[youenn fablet]

> > Source/WebCore/bindings/js/WebCoreJSBuiltins.h:37
> > +#include "WritableStreamBuiltinsWrapper.h"
> 
> Are we working on something that will eliminate the need for all this
> boilerplate? It continues to vex me!

Waiting for bug 149929 to be landed...

[Blaze Burg]

Comment on attachment 263250 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:1270
>> +    },
> 
> We should figure out a way to generate what’s needed in this file from IDL instead of having yet another thing we have to modify every time we change a class. How would a future contributor know they had to come here if they were adding to IDL.

It is not critical if they are out of date since they are only used for documentation.  That said, it shouldn't be too hard to accomplish this. I agree that we should look for a way to generate these hints from the WebIDL specification. I will look at this after the builtins generator work has settled down.

[Xabier Rodríguez Calvar]

Created attachment 263457 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 263457 [details]
Patch for landing

Clearing flags on attachment: 263457

Committed r191283: <http://trac.webkit.org/changeset/191283>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

(In reply to comment #24)
> Comment on attachment 263250 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263250&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:1270
> >> +    },
> > 
> > We should figure out a way to generate what’s needed in this file from IDL instead of having yet another thing we have to modify every time we change a class. How would a future contributor know they had to come here if they were adding to IDL.
> 
> It is not critical if they are out of date since they are only used for
> documentation.  That said, it shouldn't be too hard to accomplish this. I
> agree that we should look for a way to generate these hints from the WebIDL
> specification. I will look at this after the builtins generator work has
> settled down.

I originally generated these from IDL. The work for that is saved on bug 142760.