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: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 110556, 110558, 110841, 110938 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Zachary Kuznia
2013-02-19 02:04:38 PST
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 *** |