RESOLVED FIXED 147153
[Streams API] Create ByteLengthQueuingStrategy object as per spec
https://bugs.webkit.org/show_bug.cgi?id=147153
Summary [Streams API] Create ByteLengthQueuingStrategy object as per spec
Xabier Rodríguez Calvar
Reported 2015-07-21 09:58:33 PDT
[Streams API] Create ByteLenghQueuingStrategy object as per spec
Attachments
Patch (37.94 KB, patch)
2015-08-06 05:54 PDT, Xabier Rodríguez Calvar
no flags
Patch (50.30 KB, patch)
2015-08-10 02:35 PDT, Xabier Rodríguez Calvar
no flags
Patch (50.00 KB, patch)
2015-08-20 02:56 PDT, Xabier Rodríguez Calvar
no flags
Patch (28.93 KB, patch)
2015-09-29 04:26 PDT, Xabier Rodríguez Calvar
no flags
Patch (23.45 KB, patch)
2015-09-29 12:11 PDT, Xabier Rodríguez Calvar
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (904.00 KB, application/zip)
2015-09-29 12:41 PDT, Build Bot
no flags
Archive of layout-test-results from ews101 for mac-mavericks (703.77 KB, application/zip)
2015-09-29 12:46 PDT, Build Bot
no flags
Patch (34.17 KB, patch)
2015-09-30 02:00 PDT, Xabier Rodríguez Calvar
no flags
Xabier Rodríguez Calvar
Comment 1 2015-08-06 05:54:02 PDT
Created attachment 258362 [details] Patch It is similar to the one proposed for bug 146594. This creates a copyProperty function to abstract a bit more code between the two classes.
Xabier Rodríguez Calvar
Comment 2 2015-08-06 05:56:37 PDT
This patch will not pass on EWS because it depends on bug 146594 and therefore on bug 146593. It would be nice to get it reviewed after, of course, its dependencies.
Xabier Rodríguez Calvar
Comment 3 2015-08-10 02:35:52 PDT
Created attachment 258613 [details] Patch Fixed problems applying to tree.
Xabier Rodríguez Calvar
Comment 4 2015-08-13 09:40:27 PDT
Any review, please?
Darin Adler
Comment 5 2015-08-19 15:39:18 PDT
Comment on attachment 258613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258613&action=review > Source/WebCore/bindings/js/JSByteLengthQueuingStrategyCustom.cpp:44 > + if (chunk.isUndefined() || chunk.isNull()) There’s a possbly-more-efficient isUndefinedOrNull() that you should use in cases like this. Not sure if the compiler if smart enough to optimize. > Source/WebCore/bindings/js/JSByteLengthQueuingStrategyCustom.cpp:49 > + if (!chunk.isObject()) > + return JSValue::encode(jsUndefined()); > + > + return JSValue::encode(chunk.getObject()->get(exec, exec->vm().propertyNames->byteLength)); I don’t think we need the isObject/getObject. That’s just an unnecessary optimization. We should just use JSValue::get directly: return JSValue::encode(chunk.get(exec, exec->vm().propertyNames->byteLength)); > Source/WebCore/bindings/js/JSByteLengthQueuingStrategyCustom.cpp:54 > + Ref<ByteLengthQueuingStrategy> byteLengthQueuingStrategy = adoptRef(*new ByteLengthQueuingStrategy()); We typically omit the () when there are no arguments like this. > Source/WebCore/bindings/js/JSDOMBinding.cpp:649 > +JSValue copyProperty(JSC::ExecState &exec, JSC::JSValue source, const char* propertyName, JSC::JSValue destination) Incorrect formatting here. Should be ExecState& with the space after the &. In new code we have been using the name state for ExecState& rather than "exec". > Source/WebCore/bindings/js/JSDOMBinding.cpp:652 > + return JSC::createTypeError(&exec, ASCIILiteral("copying property from null or undefined value")); Not worded the way other exceptions are. Would be nice to be consistent in style. > Source/WebCore/bindings/js/JSDOMBinding.cpp:655 > + return JSC::JSValue(); Could be return { }; Instead. I like that better. > Source/WebCore/bindings/js/JSDOMBinding.cpp:657 > + JSC::JSObject* argumentObject = source.getObject(); No need for this local variable. > Source/WebCore/bindings/js/JSDOMBinding.cpp:658 > + JSC::JSValue property = getPropertyFromObject(exec, *argumentObject, propertyName); I think this reads better with auto. > Source/WebCore/bindings/js/JSDOMBinding.cpp:660 > + JSC::JSValue exception = exec.exception()->value(); I think this reads better with auto. > Source/WebCore/bindings/js/JSDOMBinding.cpp:664 > + setPropertyToObject(exec, *destination.getObject(), propertyName, property); Not sure this is really what we want. This could run a setter if there’s a setter by this name in the object’s prototype chain. Would we want to do that? Maybe we want to use putDirect instead? > Source/WebCore/bindings/js/JSDOMBinding.cpp:666 > + return JSC::JSValue(); Could be return { }; Instead. I like that better. > Source/WebCore/bindings/js/JSDOMBinding.h:647 > +JSC::JSValue copyProperty(JSC::ExecState&, JSC::JSValue, const char*, JSC::JSValue); Need argument names here. It’s not at all clear what the two JSValue arguments are, or even what the const char* is.
Xabier Rodríguez Calvar
Comment 6 2015-08-20 02:56:49 PDT
Created attachment 259464 [details] Patch Darin, thanks a lot for the review. I honored all your comments. For the isUndefinedOrNull I changed it in all places I found and same for exec->state. Some of these changes I will apply them to CQS class for consistency as they are like twins. I'd really appreciate another review for this new patch. Thanks again!
Xabier Rodríguez Calvar
Comment 7 2015-08-27 09:51:53 PDT
Ping. Any review?
Xabier Rodríguez Calvar
Comment 8 2015-09-23 06:34:43 PDT
I added a dependency with bug 149497 because it looks like a better way to implement BLQS. Let's wait for that to be ready and then we can change this.
Xabier Rodríguez Calvar
Comment 9 2015-09-29 04:26:40 PDT
Created attachment 262064 [details] Patch Added Byte Length Queuing Strategy as per spec and using JSBuiltins in WebCore as for CQS.
youenn fablet
Comment 10 2015-09-29 07:57:08 PDT
(In reply to comment #9) > Created attachment 262064 [details] > Patch > > Added Byte Length Queuing Strategy as per spec and using JSBuiltins in > WebCore as for CQS. Revision 190309 should remove the need to touch Xcode files. Instead, you can modify Source/WebCore/bindings/js/WebCoreJSBuiltins.cpp.
Xabier Rodríguez Calvar
Comment 11 2015-09-29 12:11:48 PDT
Created attachment 262089 [details] Patch Rebased and fixed according Youenn's comments.
Build Bot
Comment 12 2015-09-29 12:41:40 PDT
Comment on attachment 262089 [details] Patch Attachment 262089 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/224406 New failing tests: js/dom/global-constructors-attributes.html
Build Bot
Comment 13 2015-09-29 12:41:44 PDT
Created attachment 262091 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Xabier Rodríguez Calvar
Comment 14 2015-09-29 12:45:24 PDT
(In reply to comment #12) > Comment on attachment 262089 [details] > Patch > > Attachment 262089 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/224406 > > New failing tests: > js/dom/global-constructors-attributes.html I can fix this at landing time.
Build Bot
Comment 15 2015-09-29 12:46:48 PDT
Comment on attachment 262089 [details] Patch Attachment 262089 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/224410 New failing tests: js/dom/global-constructors-attributes.html
Build Bot
Comment 16 2015-09-29 12:46:51 PDT
Created attachment 262092 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Darin Adler
Comment 17 2015-09-29 22:09:03 PDT
(In reply to comment #14) > (In reply to comment #12) > > New failing tests: > > js/dom/global-constructors-attributes.html > > I can fix this at landing time. You could, but it would be even better to upload a patch with this fixed. Any ideas about why the iOS build failed?
Xabier Rodríguez Calvar
Comment 18 2015-09-30 02:00:28 PDT
Created attachment 262146 [details] Patch Updated global constructor attributes tests.
WebKit Commit Bot
Comment 19 2015-10-01 00:39:18 PDT
Comment on attachment 262146 [details] Patch Clearing flags on attachment: 262146 Committed r190394: <http://trac.webkit.org/changeset/190394>
WebKit Commit Bot
Comment 20 2015-10-01 00:39:24 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.