HTMLCanvasElement.toDataURL should use IDL 'any' to avoid using custom binding.

HTMLCanvasElement.toDataURL should use IDL 'any' to avoid using custom binding.

The specifications [1] and [2] define toDataURL's second argument to be a variable typed argument.
Utilize IDL supported 'any' to get rid of the custom binding used.

R=jl@opera.com, junov@chromium.org, haraken@chromium.org

BUG=345519
[1] http://www.w3.org/TR/html5/scripting-1.html#dom-canvas-todataurl
[2] https://html.spec.whatwg.org/multipage/scripting.html#dom-canvas-todataurl

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190370

[vivekg, 2015-02-11 14:03:48]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-02-11 14:03:48]

PTAL, thanks.

[haraken, 2015-02-11 14:14:56]

LGTM

[Jens Widell, 2015-02-11 14:15:33]

The spec doesn't say it should be a (double or DOMString) union, it says
"any...".

Technically, this means we're not really allowed to call either valueOf() or
toString() on the argument, if it is an object. We should simply ignore the
argument unless it is a number.

[vivekg, 2015-02-11 14:24:30]

On 2015/02/11 at 14:15:33, jl wrote:
> The spec doesn't say it should be a (double or DOMString) union, it says
"any...".
> 
> Technically, this means we're not really allowed to call either valueOf() or
toString() on the argument, if it is an object. We should simply ignore the
argument unless it is a number.

Yes you are right. I wanted to use something similar to <any> but couldn't find
any relevant data type for it. What is the general practice for using <any> in
the blink idl files?

[Jens Widell, 2015-02-11 14:29:07]

On 2015/02/11 14:24:30, vivekg_ wrote:
> On 2015/02/11 at 14:15:33, jl wrote:
> > The spec doesn't say it should be a (double or DOMString) union, it says
> "any...".
> > 
> > Technically, this means we're not really allowed to call either valueOf() or
> toString() on the argument, if it is an object. We should simply ignore the
> argument unless it is a number.
> 
> Yes you are right. I wanted to use something similar to <any> but couldn't
find
> any relevant data type for it. What is the general practice for using <any> in
> the blink idl files?

Well, IDL "any" gives you a ScriptValue. You could also have "any..." like the
spec, which gives you a Vector<ScriptValue>, but since we're only going to look
at the first, that's a bit unnecessary (and the difference wouldn't be
observable to a script.)

[vivekg_samsung, 2015-02-11 15:04:14]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[vivekg, 2015-02-11 15:05:46]

On 2015/02/11 at 14:29:07, jl wrote:
> On 2015/02/11 14:24:30, vivekg_ wrote:
> > On 2015/02/11 at 14:15:33, jl wrote:
> > > The spec doesn't say it should be a (double or DOMString) union, it says
> > "any...".
> > > 
> > > Technically, this means we're not really allowed to call either valueOf()
or
> > toString() on the argument, if it is an object. We should simply ignore the
> > argument unless it is a number.
> > 
> > Yes you are right. I wanted to use something similar to <any> but couldn't
find
> > any relevant data type for it. What is the general practice for using <any>
in
> > the blink idl files?
> 
> Well, IDL "any" gives you a ScriptValue. You could also have "any..." like the
spec, which gives you a Vector<ScriptValue>, but since we're only going to look
at the first, that's a bit unnecessary (and the difference wouldn't be
observable to a script.)

Done, PTAL. Thanks for the pointer, it's really helpful. :)

[Jens Widell, 2015-02-11 15:13:08]

LGTM

Don't forget to update the issue description. :-)

https://codereview.chromium.org/917733002/diff/40001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/917733002/diff/40001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.cpp:483: quality =
v8Value.As<v8::Number>()->Value();
quality = v8Value->NumberValue();

[vivekg_samsung, 2015-02-11 15:16:41]

New patchsets have been uploaded after l-g-t-m from jl@opera.com

[vivekg, 2015-02-11 15:17:52]

On 2015/02/11 at 15:13:08, jl wrote:
> LGTM
> 
> Don't forget to update the issue description. :-)
> 

Done :)

>
https://codereview.chromium.org/917733002/diff/40001/Source/core/html/HTMLCan...
> File Source/core/html/HTMLCanvasElement.cpp (right):
> 
>
https://codereview.chromium.org/917733002/diff/40001/Source/core/html/HTMLCan...
> Source/core/html/HTMLCanvasElement.cpp:483: quality =
v8Value.As<v8::Number>()->Value();
> quality = v8Value->NumberValue();

Done. Thank you.

[Jens Widell, 2015-02-11 15:18:55]

On 2015/02/11 15:17:52, vivekg_ wrote:
> On 2015/02/11 at 15:13:08, jl wrote:
> > LGTM
> > 
> > Don't forget to update the issue description. :-)
> > 
> 
> Done :)

It still says "use IDL union types" on the first line.

[vivekg, 2015-02-11 15:20:31]

On 2015/02/11 at 15:18:55, jl wrote:
> On 2015/02/11 15:17:52, vivekg_ wrote:
> > On 2015/02/11 at 15:13:08, jl wrote:
> > > LGTM
> > > 
> > > Don't forget to update the issue description. :-)
> > > 
> > 
> > Done :)
> 
> It still says "use IDL union types" on the first line.

Oops...Done now! (was in a hurry to rush home :) sorry about that)

[vivekg, 2015-02-11 16:27:39]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-02-11 16:29:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917733002/60001

[Justin Novosad, 2015-02-11 20:05:24]

On 2015/02/11 16:29:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/917733002/60001

This cl is missing a layout test

[Justin Novosad, 2015-02-11 20:05:29]

The CQ bit was unchecked by junov@chromium.org

[Justin Novosad, 2015-02-11 20:09:35]

On 2015/02/11 20:05:24, junov wrote:
> 
> This cl is missing a layout test

I say this because this change does not just affect the use of custom bindings,
it also affects the legal ways of calling this method, does it not?

[Jens Widell, 2015-02-11 20:17:25]

On 2015/02/11 20:09:35, junov wrote:
> On 2015/02/11 20:05:24, junov wrote:
> > 
> > This cl is missing a layout test
> 
> I say this because this change does not just affect the use of custom
bindings,
> it also affects the legal ways of calling this method, does it not?

I think this change is behaviorally neutral. In what way do you think the legal
ways of calling the method changes?

[Jens Widell, 2015-02-11 20:18:55]

https://codereview.chromium.org/917733002/diff/60001/Source/core/html/HTMLCan...
File Source/core/html/HTMLCanvasElement.idl (right):

https://codereview.chromium.org/917733002/diff/60001/Source/core/html/HTMLCan...
Source/core/html/HTMLCanvasElement.idl:34: [RaisesException] DOMString
toDataURL([TreatUndefinedAs=NullString, Default=Undefined] optional DOMString?
type, optional any arguments);
BTW, the first argument could equivalently be declared as "optional DOMString?
type = null" (without any extended attributes.)

[vivekg, 2015-02-12 03:54:44]

On 2015/02/11 at 20:09:35, junov wrote:
> On 2015/02/11 20:05:24, junov wrote:
> > 
> > This cl is missing a layout test
> 
> I say this because this change does not just affect the use of custom
bindings, it also affects the legal ways of calling this method, does it not?

Thanks junov@, here [1] are some of the cases which already verify this. Would
this be sufficient as there is no semantic or syntactic difference introduced
with this change.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[vivekg, 2015-02-13 00:26:35]

junov@: friendly ping.

[vivekg, 2015-02-16 18:40:46]

junov@, can you please let me know your feedback? I have rebased the patch on
the latest ToT.

[Justin Novosad, 2015-02-17 20:26:12]

On 2015/02/16 18:40:46, vivekg_ wrote:
> junov@, can you please let me know your feedback? I have rebased the patch on
> the latest ToT.

Sorry for the delay. lgtm

[vivekg, 2015-02-18 00:46:27]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-02-18 00:47:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917733002/100001

[commit-bot: I haz the power, 2015-02-18 01:02:31]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190370