Bug 174552 – [WebIDL] Remove custom bindings that require non-caching JS strings

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

[WebIDL] Remove custom bindings that require non-caching JS strings

[Sam Weinig]

Created attachment 315578 [details]
Patch

[Sam Weinig]

Created attachment 315579 [details]
Patch

[Darin Adler]

Comment on attachment 315579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315579&action=review

Not setting the review flag yet since EWS bubbles are red so maybe there is some more significant change coming and I may want to re-review.

> Source/WebCore/html/HTMLCanvasElement.cpp:553
> +    std::optional<double> quality;
> +    if (qualityValue.isNumber())
> +        quality = qualityValue.asNumber();

Excellent fix/insight here to use asNumber instead of toNumber!

On the other hand, not quite sure what the code was doing is 100% right. Is it important to pass along NaN and infinity, for example, rather than treating them as unspecified quality? Also surprised that this is not toNumberFromPrimitive or just a normal numeric argument. Maybe we can take a compatibility hint at some point and make this a more normal argument.

> Source/WebCore/html/HTMLCanvasElement.cpp:579
>      std::optional<double> quality;
>      if (qualityValue.isNumber())
> -        quality = qualityValue.toNumber(context.execState());
> +        quality = qualityValue.asNumber();

Ditto.

> Source/WebCore/html/HTMLCanvasElement.h:33
> +#include "StringAdaptors.h"

Could we just forward-declare UncachedString instead?

> Source/WebCore/xml/XMLHttpRequest.h:28
> +#include "StringAdaptors.h"

Could we just forward-declare OwnedString instead?

> Source/WebCore/xml/XMLHttpRequest.idl:84
> +    // FIMXE: this should not be nullable.
> +    [GetterMayThrowException] readonly attribute USVString? responseText;

Reading over the code, It seems clear the responseText function can never actually return a null string, since it returns the result of StringBuilder::toStringPreserveCapacity and that function can never return the null string. Given that and given that this is a read-only attribute you can just take out the "?" here instead of adding the comment.

[Sam Weinig]

Created attachment 315585 [details]
Patch

[Sam Weinig]

Created attachment 315587 [details]
Patch

[Sam Weinig]

Created attachment 315588 [details]
Patch

[Sam Weinig]

Created attachment 315589 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 315579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315579&action=review
> 
> Not setting the review flag yet since EWS bubbles are red so maybe there is
> some more significant change coming and I may want to re-review.
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:553
> > +    std::optional<double> quality;
> > +    if (qualityValue.isNumber())
> > +        quality = qualityValue.asNumber();
> 
> Excellent fix/insight here to use asNumber instead of toNumber!
> 
> On the other hand, not quite sure what the code was doing is 100% right. Is
> it important to pass along NaN and infinity, for example, rather than
> treating them as unspecified quality? Also surprised that this is not
> toNumberFromPrimitive or just a normal numeric argument. Maybe we can take a
> compatibility hint at some point and make this a more normal argument.

The spec is quite weird here, so I think what I am doing is actually correct (https://html.spec.whatwg.org/multipage/canvas.html#a-serialisation-of-the-bitmap-as-a-file)

The relevant text is:

"If type is an image format that supports variable quality (such as "image/jpeg") and quality is given, then, if Type(quality) is Number, and quality is in the range 0.0 to 1.0 inclusive, the user agent must treat quality as the desired quality level. If Type(quality) is not Number, or if quality is outside that range, the user agent must use its default quality value, as if the quality argument had not been given.

NOTE: The use of type-testing here, instead of simply declaring quality as a Web IDL double, is a historical artifact."

In my latest patch, I made it even more to spec by adding the >0 and <1 tests. 

> 
> > Source/WebCore/html/HTMLCanvasElement.h:33
> > +#include "StringAdaptors.h"
> 
Done.

> 
> > Source/WebCore/xml/XMLHttpRequest.h:28
> > +#include "StringAdaptors.h"
> 
> Could we just forward-declare OwnedString instead?

Done.

> 
> > Source/WebCore/xml/XMLHttpRequest.idl:84
> > +    // FIMXE: this should not be nullable.
> > +    [GetterMayThrowException] readonly attribute USVString? responseText;
> 
> Reading over the code, It seems clear the responseText function can never
> actually return a null string, since it returns the result of
> StringBuilder::toStringPreserveCapacity and that function can never return
> the null string. Given that and given that this is a read-only attribute you
> can just take out the "?" here instead of adding the comment.

Good catch. Thanks. Fixed.

[Sam Weinig]

Created attachment 315591 [details]
Patch

[Sam Weinig]

Created attachment 315609 [details]
Patch

[Build Bot]

Comment on attachment 315609 [details]
Patch

Attachment 315609 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4131352

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315612 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

I don't believe the new failure is my doing. That test has shown up on other patches I have reviewed lately, so it seems a bit flakey.

[Chris Dumez]

Comment on attachment 315609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315609&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:555
> +ExceptionOr<UncachedString> HTMLCanvasElement::toDataURL(const String& mimeType, JSC::JSValue qualityValue)

Looks like a layering violation to me. UncachedString is a bindings concept and should not be exposed to the implementation IMHO. Why isn't the implementation returning a regular string and we would use an IDL extended attribute to have the bindings do the right thing?

[Chris Dumez]

Comment on attachment 315609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315609&action=review

> Source/WebCore/xml/XMLHttpRequest.h:90
> +    ExceptionOr<OwnedString> responseText();

Ditto here.

[Sam Weinig]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 315609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315609&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:555
> > +ExceptionOr<UncachedString> HTMLCanvasElement::toDataURL(const String& mimeType, JSC::JSValue qualityValue)
> 
> Looks like a layering violation to me. UncachedString is a bindings concept
> and should not be exposed to the implementation IMHO. Why isn't the
> implementation returning a regular string and we would use an IDL extended
> attribute to have the bindings do the right thing?

We no longer have a strict layering between the bindings and the implementation (e.g. any -> JSC::JSValue). I don't see a particularly strong reason to add more extended attributes when we can use the type system just fine.

[Chris Dumez]

(In reply to Sam Weinig from comment #16)
> (In reply to Chris Dumez from comment #14)
> > Comment on attachment 315609 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315609&action=review
> > 
> > > Source/WebCore/html/HTMLCanvasElement.cpp:555
> > > +ExceptionOr<UncachedString> HTMLCanvasElement::toDataURL(const String& mimeType, JSC::JSValue qualityValue)
> > 
> > Looks like a layering violation to me. UncachedString is a bindings concept
> > and should not be exposed to the implementation IMHO. Why isn't the
> > implementation returning a regular string and we would use an IDL extended
> > attribute to have the bindings do the right thing?
> 
> We no longer have a strict layering between the bindings and the
> implementation (e.g. any -> JSC::JSValue). I don't see a particularly strong
> reason to add more extended attributes when we can use the type system just
> fine.

I don't like that it makes the implementation look worse. It is not uncommon for methods supporting bindings API to also be called internally. Our native code should not have to know about owned JS strings or JS caching.

[Darin Adler]

What’s the best argument *against* using an IDL extended attribute?

[Sam Weinig]

(In reply to Darin Adler from comment #18)
> What’s the best argument *against* using an IDL extended attribute?

Probably more code to add a new extended attribute, both in perl and in c++.

That said, I think it's really a six-to-one situation. I like that people can discover this when reading the code, and I don't think people tend to checkout the IDL files when debugging or exploring. I also wanted to move away from these annotated string types (e.g. IDLAtomicStringAdaptor and IDLRequiresExistingAtomicStringAdaptor) and use the type system to deduce it. 

My main goal here is getting rid of the custom bindings. I thought this was a neat way to do it, that had minimal impact, but I don't want it to halt progress on that goal.

[Darin Adler]

I have no objection to landing this version even if you later want to take Chris’s suggestion and use an IDL attribute instead.

Ignoring the necessary evil of using JSValue for any, I am not sure how I feel about various binding concepts in the C++ code. Clearly the "owned string" concept is exposing something that the DOM implementation itself knows to be true, not something about the interface, so it seems more appropriate that the C++ DOM expose this information rather than relying on someone specifying a flag in the IDL even if it’s a concept that is for use to properly implement bindings.

So I think I prefer this even though as Chris says, the "implementation looks worse". This seems like a statement that the C++ DOM should be making about its implementation. I think it’s another necessary evil that the IDL files have so much information about how to implement the bindings, not about the interface. If there was a way to express more of that in the C++ I think I would prefer it.

In either case, we can definitely return and redo this, and we should not let this debate hold up this step in the right direction.

[WebKit Commit Bot]

Comment on attachment 315609 [details]
Patch

Clearing flags on attachment: 315609

Committed r219599: <http://trac.webkit.org/changeset/219599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.