WIP-Prototype implmentation of Stream API
Created attachment 189030 [details] WIP Patch
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 on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16611699
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 on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16557073
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 on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16613700
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 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.
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.
(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 on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16657133
Comment on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16646191
Comment on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16654185
Comment on attachment 189030 [details] WIP Patch Attachment 189030 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16654337
Created attachment 189676 [details] WIP Patch
Created attachment 190196 [details] WIP Patch
Comment on attachment 190196 [details] WIP Patch Attachment 190196 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16745744
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 on attachment 190196 [details] WIP Patch Attachment 190196 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16752695
Comment on attachment 190196 [details] WIP Patch Attachment 190196 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16752686
Comment on attachment 190196 [details] WIP Patch Attachment 190196 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16756719
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
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.
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.
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 on attachment 190196 [details] WIP Patch Attachment 190196 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/16781563
I plan to close that bug in favor of bug 138561
*** This bug has been marked as a duplicate of bug 138561 ***