Bug 147153 – [Streams API] Create ByteLengthQueuingStrategy object as per spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Xabier Rodríguez Calvar]

[Streams API] Create ByteLenghQueuingStrategy object as per spec

[Xabier Rodríguez Calvar]

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]

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]

Created attachment 258613 [details]
Patch

Fixed problems applying to tree.

[Xabier Rodríguez Calvar]

Any review, please?

[Darin Adler]

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]

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]

Ping. Any review?

[Xabier Rodríguez Calvar]

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]

Created attachment 262064 [details]
Patch

Added Byte Length Queuing Strategy as per spec and using JSBuiltins in WebCore as for CQS.

[youenn fablet]

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

Created attachment 262089 [details]
Patch

Rebased and fixed according Youenn's comments.

[Build Bot]

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]

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]

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

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]

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

Created attachment 262146 [details]
Patch

Updated global constructor attributes tests.

[WebKit Commit Bot]

Comment on attachment 262146 [details]
Patch

Clearing flags on attachment: 262146

Committed r190394: <http://trac.webkit.org/changeset/190394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.