Bug 110194

Summary:

[Meta] Implement Stream API

Product:

WebKit

Reporter:

Zachary Kuznia <zork>

Component:

New Bugs

Assignee:

Zachary Kuznia <zork>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

abarth, acolwell, buildbot, calvaris, dglazkov, dominicc, dstockwell, eojin.ham, esprehn+autocc, fishd, gtk-ews, haraken, jamesr, japhet, kinuko, levin+threading, mike, ojan.autocc, rego+ews, rniwa, s.choi, syoichi, webkit.review.bot, xan.lopez, youennf, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

110556, 110558, 110841, 110938

Bug Blocks:

Attachments:

Description	Flags
WIP Patch	none
WIP Patch	none
WIP Patch	webkit-ews: commit-queue-

Zachary Kuznia

Reported 2013-02-19 02:04:38 PST

WIP-Prototype implmentation of Stream API

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Zachary Kuznia

Comment 1 2013-02-19 02:05:59 PST

Created attachment 189030 [details] WIP Patch

WebKit Review Bot

Comment 2 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.

Early Warning System Bot

Comment 3 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

Early Warning System Bot

Comment 4 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

EFL EWS Bot

Comment 5 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

Build Bot

Comment 6 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

kov's GTK+ EWS bot

Comment 7 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

Adam Barth

Comment 8 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.

Adam Barth

Comment 9 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.

Adam Barth

Comment 10 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.

Zachary Kuznia

Comment 11 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.

Build Bot

Comment 12 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

Build Bot

Comment 13 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

Build Bot

Comment 14 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

Build Bot

Comment 15 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

Zachary Kuznia

Comment 16 2013-02-21 21:36:02 PST

Created attachment 189676 [details] WIP Patch

Zachary Kuznia

Comment 17 2013-02-25 21:02:14 PST

Created attachment 190196 [details] WIP Patch

Early Warning System Bot

Comment 18 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

Early Warning System Bot

Comment 19 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

EFL EWS Bot

Comment 20 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

Build Bot

Comment 21 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

Build Bot

Comment 22 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

Build Bot

Comment 23 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

WebKit Review Bot

Comment 24 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.

WebKit Review Bot

Comment 25 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.

Adam Barth

Comment 26 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.

Build Bot

Comment 27 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

youenn fablet

Comment 28 2014-11-10 00:17:36 PST

I plan to close that bug in favor of bug 138561

youenn fablet

Comment 29 2014-11-21 09:07:44 PST

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

Note You need to log in before you can comment on or make changes to this bug.