Bug 147153

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.

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.

Created attachment 258613 [details] Patch Fixed problems applying to tree.

Any review, please?

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.

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!

Ping. Any review?

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.

Created attachment 262064 [details] Patch Added Byte Length Queuing Strategy as per spec and using JSBuiltins in WebCore as for CQS.

(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.

Created attachment 262089 [details] Patch Rebased and fixed according Youenn's comments.

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

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

(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.

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

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

(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?

Created attachment 262146 [details] Patch Updated global constructor attributes tests.

Comment on attachment 262146 [details] Patch Clearing flags on attachment: 262146 Committed r190394: <http://trac.webkit.org/changeset/190394>

All reviewed patches have been landed. Closing bug.