Bug 110194 - [Meta] Implement Stream API
Summary: [Meta] Implement Stream API
Status: RESOLVED DUPLICATE of bug 138561
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zachary Kuznia
URL:
Keywords:
Depends on: 110556 110558 110841 110938
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-19 02:04 PST by Zachary Kuznia
Modified: 2014-11-24 19:00 PST (History)
26 users (show)

See Also:


Attachments
WIP Patch (57.29 KB, patch)
2013-02-19 02:05 PST, Zachary Kuznia
no flags Details | Formatted Diff | Diff
WIP Patch (72.41 KB, patch)
2013-02-21 21:36 PST, Zachary Kuznia
no flags Details | Formatted Diff | Diff
WIP Patch (61.42 KB, patch)
2013-02-25 21:02 PST, Zachary Kuznia
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zachary Kuznia 2013-02-19 02:04:38 PST
WIP-Prototype implmentation of Stream API
Comment 1 Zachary Kuznia 2013-02-19 02:05:59 PST
Created attachment 189030 [details]
WIP Patch
Comment 2 WebKit Review Bot 2013-02-19 02:08:19 PST
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.
Comment 3 Early Warning System Bot 2013-02-19 02:21:23 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16611699
Comment 4 Early Warning System Bot 2013-02-19 02:28:41 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16612657
Comment 5 EFL EWS Bot 2013-02-19 03:59:15 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16557073
Comment 6 Build Bot 2013-02-19 04:37:53 PST
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
Comment 7 kov's GTK+ EWS bot 2013-02-19 05:56:54 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16613700
Comment 8 Adam Barth 2013-02-19 11:16:31 PST
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.
Comment 9 Adam Barth 2013-02-19 11:16:58 PST
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.
Comment 10 Adam Barth 2013-02-19 11:17:57 PST
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.
Comment 11 Zachary Kuznia 2013-02-19 19:40:26 PST
(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.
Comment 12 Build Bot 2013-02-20 01:47:33 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16657133
Comment 13 Build Bot 2013-02-20 03:11:51 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16646191
Comment 14 Build Bot 2013-02-20 04:15:15 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16654185
Comment 15 Build Bot 2013-02-20 07:57:32 PST
Comment on attachment 189030 [details]
WIP Patch

Attachment 189030 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16654337
Comment 16 Zachary Kuznia 2013-02-21 21:36:02 PST
Created attachment 189676 [details]
WIP Patch
Comment 17 Zachary Kuznia 2013-02-25 21:02:14 PST
Created attachment 190196 [details]
WIP Patch
Comment 18 Early Warning System Bot 2013-02-25 21:14:58 PST
Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16745744
Comment 19 Early Warning System Bot 2013-02-25 21:15:40 PST
Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16749698
Comment 20 EFL EWS Bot 2013-02-25 22:01:41 PST
Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16752695
Comment 21 Build Bot 2013-02-25 22:24:10 PST
Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16752686
Comment 22 Build Bot 2013-02-25 23:17:03 PST
Comment on attachment 190196 [details]
WIP Patch

Attachment 190196 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16756719
Comment 23 Build Bot 2013-02-26 01:38:31 PST
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
Comment 24 WebKit Review Bot 2013-02-26 02:00:47 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 25 WebKit Review Bot 2013-02-26 02:01:04 PST
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.
Comment 26 Adam Barth 2013-02-26 14:59:41 PST
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.
Comment 27 Build Bot 2013-02-28 02:43:00 PST
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
Comment 28 youenn fablet 2014-11-10 00:17:36 PST
I plan to close that bug in favor of bug 138561
Comment 29 youenn fablet 2014-11-21 09:07:44 PST

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