Bug 110194 – [Meta] Implement Stream API

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Zachary Kuznia]

WIP-Prototype implmentation of Stream API

[Zachary Kuznia]

Created attachment 189030 [details]
WIP Patch

[WebKit Review Bot]

Attachment 189030 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.features.am.in', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/bindings/js/JSStreamReaderCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8StreamCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8StreamReaderCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp', u'Source/WebCore/dom/EventTarget.h', u'Source/WebCore/dom/EventTargetFactory.in', u'Source/WebCore/fileapi/Stream.cpp', u'Source/WebCore/fileapi/Stream.h', u'Source/WebCore/fileapi/Stream.idl', u'Source/WebCore/fileapi/StreamError.h', u'Source/WebCore/fileapi/StreamError.idl', u'Source/WebCore/fileapi/StreamReader.cpp', u'Source/WebCore/fileapi/StreamReader.h', u'Source/WebCore/fileapi/StreamReader.idl', u'Source/WebCore/page/DOMWindow.idl', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/XMLHttpRequest.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/features.gypi']" exit_code: 1
Source/WebCore/fileapi/StreamReader.h:66:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamReader.h:67:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamReader.h:68:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:44:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:45:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:52:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:53:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:54:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:55:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamError.h:56:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/bindings/js/JSStreamReaderCustom.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/JSStreamReaderCustom.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 18 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16611699

[Early Warning System Bot]

Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16612657

[EFL EWS Bot]

Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16557073

[Build Bot]

Comment on attachment 189030 [details]
WIP Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16613700

[Adam Barth]

Comment on attachment 189030 [details]
WIP Patch

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

> Source/WebCore/bindings/v8/custom/V8StreamCustom.cpp:56
> +v8::Handle<v8::Object> wrap(Stream* impl, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
> +{
> +    ASSERT(impl);
> +    return V8Stream::createWrapper(impl, creationContext, isolate);
> +}
> +
> +v8::Handle<v8::Value> V8Stream::constructorCallbackCustom(const v8::Arguments& args)
> +{
> +    // ScriptExecutionContext* context = getScriptExecutionContext();
> +
> +    if (!args.Length()) {
> +        RefPtr<Stream> stream = Stream::create();
> +        return toV8(stream.get(), args.Holder(), args.GetIsolate());
> +    }
> +
> +    RefPtr<Stream> stream = Stream::create();
> +    return toV8(stream.get(), args.Holder(), args.GetIsolate());
> +}

You shouldn't need any of this custom code.

> Source/WebCore/bindings/v8/custom/V8StreamReaderCustom.cpp:47
> +    if (imp->readType() == StreamReader::ReadAsArrayBuffer)

This one does look needed, however.  :)

> Source/WebCore/fileapi/Stream.cpp:69
> +    unsigned long long remaining = m_buffer->size() - m_readHead;
> +    unsigned long long length = remaining;

Should we use size_t for these?

> Source/WebCore/fileapi/Stream.cpp:80
> +    return data;

data.release()

See http://www.webkit.org/coding/RefPtr.html

> Source/WebCore/fileapi/Stream.idl:32
> +    JSGenerateIsReachable=Impl,

This seems unlikely

> Source/WebCore/fileapi/Stream.idl:34
> +    CustomToJSObject,
> +    JSNoStaticTables,

These shouldn't be needed either.

> Source/WebCore/fileapi/Stream.idl:44
> +#if !defined(LANGUAGE_OBJECTIVE_C)
> +#if defined(ENABLE_STREAM) && ENABLE_STREAM
> +    void close();
> +#endif
> +#endif

Shouldn't the whole interface have Conditional=STREAM ?

I doubt you need these LANGUAGE_OBJECTIVE_C ifdefs.

> Source/WebCore/fileapi/StreamError.h:64
> +    StreamError(ErrorCode code)

please add the "explicit" keyword to one argument constructors.

> Source/WebCore/fileapi/StreamError.h:66
> +    { }

These should be a on separate lines.

> Source/WebCore/fileapi/StreamError.idl:33
> +    JSNoStaticTables,

JSNoStaticTables probably isn't needed.

> Source/WebCore/fileapi/StreamError.idl:49
> +    const unsigned short NOT_FOUND_ERR = 1;
> +    const unsigned short SECURITY_ERR = 2;
> +    const unsigned short ABORT_ERR = 3;
> +    const unsigned short NOT_READABLE_ERR = 4;
> +    const unsigned short ENCODING_ERR = 5;
> +    const unsigned short NO_MODIFICATION_ALLOWED_ERR = 6;
> +    const unsigned short INVALID_STATE_ERR = 7;
> +    const unsigned short SYNTAX_ERR = 8;
> +    const unsigned short INVALID_MODIFICATION_ERR = 9;
> +    const unsigned short QUOTA_EXCEEDED_ERR = 10;
> +    const unsigned short TYPE_MISMATCH_ERR = 11;
> +    const unsigned short PATH_EXISTS_ERR = 12;

Yuck!  This isn't the modern way to do errors.  We should provide feedback to the working group that we should use DOM4-style errors.

> Source/WebCore/fileapi/StreamReader.cpp:155
> +    fireEvent(eventNames().errorEvent);
> +    fireEvent(eventNames().abortEvent);
> +    fireEvent(eventNames().loadendEvent);

Does this fire these event synchronously?

> Source/WebCore/fileapi/StreamReader.h:113
> +    StreamReader(ScriptExecutionContext*);

Please mark one-argument constructors explicit.

> Source/WebCore/fileapi/StreamReader.idl:37
> +    JSNoStaticTables

This probably isn't needed.

[Adam Barth]

Comment on attachment 189030 [details]
WIP Patch

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

> Source/WebCore/Configurations/FeatureDefines.xcconfig:147
> +ENABLE_STREAM = ENABLE_STREAM;

You can skip these changes to xcconfig.  The default is for the feature to be disabled, which is what you probably want.

[Adam Barth]

The comments above are all pretty shallow.  Would you be willing to break this patch into smaller pieces so that it is easier to review?  For example, we could try to do one interface at a time.

[Zachary Kuznia]

(In reply to comment #10)
> The comments above are all pretty shallow.  Would you be willing to break this patch into smaller pieces so that it is easier to review?  For example, we could try to do one interface at a time.

I'm more than happy to split this up!  I'm changing it to share more with the FileReader and Blob implementation before it's ready for review, but once that's done I'll divide it into more manageable chunks.

[Build Bot]

Comment on attachment 189030 [details]
WIP Patch

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

[Build Bot]

Comment on attachment 189030 [details]
WIP Patch

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

[Build Bot]

Comment on attachment 189030 [details]
WIP Patch

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

[Build Bot]

Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16654337

[Zachary Kuznia]

Created attachment 189676 [details]
WIP Patch

[Zachary Kuznia]

Created attachment 190196 [details]
WIP Patch

[Early Warning System Bot]

Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16745744

[Early Warning System Bot]

Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16749698

[EFL EWS Bot]

Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16752695

[Build Bot]

Comment on attachment 190196 [details]
WIP Patch

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

[Build Bot]

Comment on attachment 190196 [details]
WIP Patch

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

[Build Bot]

Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16760678

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 190196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platform/chromium/public/WebBlobRegistry.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/bindings/js/JSStreamReaderCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8StreamReaderCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp', u'Source/WebCore/dom/EventTarget.h', u'Source/WebCore/dom/EventTargetFactory.in', u'Source/WebCore/fileapi/FileReaderLoader.cpp', u'Source/WebCore/fileapi/FileReaderLoader.h', u'Source/WebCore/fileapi/Stream.cpp', u'Source/WebCore/fileapi/Stream.h', u'Source/WebCore/fileapi/Stream.idl', u'Source/WebCore/fileapi/StreamReader.cpp', u'Source/WebCore/fileapi/StreamReader.h', u'Source/WebCore/fileapi/StreamReader.idl', u'Source/WebCore/fileapi/ThreadableBlobRegistry.cpp', u'Source/WebCore/fileapi/ThreadableBlobRegistry.h', u'Source/WebCore/page/DOMWindow.idl', u'Source/WebCore/platform/network/BlobRegistry.h', u'Source/WebCore/platform/network/chromium/BlobRegistryProxy.cpp', u'Source/WebCore/platform/network/chromium/BlobRegistryProxy.h', u'Source/WebCore/xml/XMLHttpRequest.cpp', u'Source/WebCore/xml/XMLHttpRequest.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/features.gypi']" exit_code: 1
Source/WebCore/fileapi/StreamReader.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamReader.h:61:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/fileapi/StreamReader.h:62:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Note that this topic is current being discussed in webapp:

http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0569.html

We should participate in that discussion to ensure that other vendors are on board with implementing this API.

[Build Bot]

Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/16781563

[youenn fablet]

I plan to close that bug in favor of bug 138561

[youenn fablet]

*** This bug has been marked as a duplicate of bug 138561 ***