Bindings for CSSOM View smooth scroll API

Bindings for CSSOM View smooth scroll API

This adds custom bindings for Window.scroll, Window.scrollTo
Window.scrollBy, Element.scrollLeft, and Element.scrollTop,
needed for the CSSOM View smooth scroll API.

More specifically:
1) An optional ScrollOptions argument is added to
Window.scroll, Window.scrollTo, and Window.scrollBy.
2) The setter for Element.scrollLeft can now take a
ScrollOptionsHorizontal dictionary instead of an int.
3) The setter for Element.scrollRight can now take a
ScrollOptionsVertical dictionary instead of an int.

These behavior changes are protected by a runtime flag.

Spec: http://www.w3.org/TR/cssom-view/

Intent-to-implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/YLWs9N46XjQ/mK7rCJ26BqAJ


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

[haraken, 2014-01-27 01:44:12]

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ElementCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ElementCustom.cpp:43: void
V8Element::scrollLeftAttributeSetterCustom(v8::Local<v8::Value> value, const
v8::PropertyCallbackInfo<void>& info)

Why do you need to write these methods in custom bindings?

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8WindowCustom.cpp:390: void
V8Window::scrollByMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)

Ditto. Can't you avoid writing custom bindings by adding some helper methods in
core/ ?

[ajuma, 2014-01-27 02:07:00]

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ElementCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ElementCustom.cpp:43: void
V8Element::scrollLeftAttributeSetterCustom(v8::Local<v8::Value> value, const
v8::PropertyCallbackInfo<void>& info)
On 2014/01/27 01:44:13, haraken wrote:
> 
> Why do you need to write these methods in custom bindings?

These setters need to be able to take either an int or a dictionary. I couldn't
find a way to get this to work without using custom bindings.

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8WindowCustom.cpp:390: void
V8Window::scrollByMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2014/01/27 01:44:13, haraken wrote:
> 
> Ditto. Can't you avoid writing custom bindings by adding some helper methods
in
> core/ ?

The problem is that specifying that a particular argument is runtime-enabled
doesn't seem to be supported right now. So when I tried this approach (that is,
adding code in core/ to ignore the third argument when the runtime flag is
disabled), the issue I ran into was that the type of the third argument was
being checked even when the runtime flag is disabled, which means that a call
like "window.scrollTo(1,2,3)" which works on ToT (since "3" is ignored) will
throw an error, even when the runtime flag is disabled.

[haraken, 2014-01-27 02:30:48]

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ElementCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ElementCustom.cpp:43: void
V8Element::scrollLeftAttributeSetterCustom(v8::Local<v8::Value> value, const
v8::PropertyCallbackInfo<void>& info)
On 2014/01/27 02:07:00, ajuma wrote:
> On 2014/01/27 01:44:13, haraken wrote:
> > 
> > Why do you need to write these methods in custom bindings?
> 
> These setters need to be able to take either an int or a dictionary. I
couldn't
> find a way to get this to work without using custom bindings.

Won't overloading IDL methods work?

void scrollLeft(long value);
void scrollLeft(Dictionary value);

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8WindowCustom.cpp:390: void
V8Window::scrollByMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2014/01/27 02:07:00, ajuma wrote:
> On 2014/01/27 01:44:13, haraken wrote:
> > 
> > Ditto. Can't you avoid writing custom bindings by adding some helper methods
> in
> > core/ ?
> 
> The problem is that specifying that a particular argument is runtime-enabled
> doesn't seem to be supported right now. So when I tried this approach (that
is,
> adding code in core/ to ignore the third argument when the runtime flag is
> disabled), the issue I ran into was that the type of the third argument was
> being checked even when the runtime flag is disabled, which means that a call
> like "window.scrollTo(1,2,3)" which works on ToT (since "3" is ignored) will
> throw an error, even when the runtime flag is disabled.

Probably we want to improve code_generator_v8.pm so that it can support
[RuntimeEnabled] on function arguments?

I understand your pain --- it's a tough task to improve the code generator.
However, adding more custom bindings leads to more bugs, so it seems to me it's
a right thing to do here.

[Nils Barth (inactive), 2014-01-27 02:38:23]

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ElementCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ElementCustom.cpp:43: void
V8Element::scrollLeftAttributeSetterCustom(v8::Local<v8::Value> value, const
v8::PropertyCallbackInfo<void>& info)
> On 2014/01/27 02:07:00, ajuma wrote:
> > These setters need to be able to take either an int or a dictionary. I
> couldn't
> > find a way to get this to work without using custom bindings.

Conveniently, long vs. Dictionary overloading is possible as of last week:

Add overload support for non-wrapper type vs. primitive type
https://codereview.chromium.org/131973012/

On 2014/01/27 02:30:48, haraken wrote:

> Won't overloading IDL methods work?
> 
> void scrollLeft(long value);
> void scrollLeft(Dictionary value);

Note that the order must be reversed:
(object before primitive)

void scrollLeft(Dictionary value);
void scrollLeft(long value);

This is b/c we add a test (IsObject) for objects,
but none for primitives.
(This will be fixed in future.)

[ajuma, 2014-01-27 02:52:15]

Thanks for the comments!

Note that scrollLeft is an attribute, not a method, so we'd need something like:

attribute Dictionary scrollLeft; // setter only
attribute long scrollLeft;

[Nils Barth (inactive), 2014-01-27 05:03:49]

On 2014/01/27 02:52:15, ajuma wrote:
> Thanks for the comments!
> 
> Note that scrollLeft is an attribute, not a method, so we'd need something
like:
> 
> attribute Dictionary scrollLeft; // setter only
> attribute long scrollLeft;

Oh, for that you'd want to use a union type instead:
http://www.w3.org/TR/WebIDL/#idl-union
attribute (Dictionary or long) scrollLeft;
(I don't believe that there is any way to say that one type is only valid for
setters.)

This *should* "just work", but we don't have any real examples of union type
attributes,
so this may not work - this is a fault of the code generator.

In that case, you could instead write:
[Custom] attribute (Dictionary or long) scrollLeft;
...and implement this via a custom binding until we improve the CG to support
union type attributes.

If even this doesn't work, the "completely generic" solution is:
[Custom] attribute any scrollLeft;

So:
* Could you try writing:
attribute (Dictionary or long) scrollLeft;
...and see if it works or is usable?

[Nils Barth (inactive), 2014-01-27 05:23:44]

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8WindowCustom.cpp (right):

https://codereview.chromium.org/146003002/diff/70001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8WindowCustom.cpp:390: void
V8Window::scrollByMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info)
On 2014/01/27 02:30:48, haraken wrote:
> Probably we want to improve code_generator_v8.pm so that it can support
> [RuntimeEnabled] on function arguments?

Ow! (>.<)
That's in principle possible, but would be really confusing:
the code inside the method would need to check RuntimeEnabled,
which get v. confusing and would add huge complexity to the CG.

Much simpler would be to just have two methods and use overloading, with the
second one RuntimeEnabled:
void a(x, y);
[RuntimeEnabled=Foo] void a(x, y, z);

This should work, and if not, it would be rather easier to extend the CG to
support this case.

Docs:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Overlo...

[ajuma, 2014-01-27 16:30:52]

On 2014/01/27 05:03:49, Nils Barth wrote:
> On 2014/01/27 02:52:15, ajuma wrote:
> > Thanks for the comments!
> > 
> > Note that scrollLeft is an attribute, not a method, so we'd need something
> like:
> > 
> > attribute Dictionary scrollLeft; // setter only
> > attribute long scrollLeft;
> 
> Oh, for that you'd want to use a union type instead:
> http://www.w3.org/TR/WebIDL/#idl-union
> attribute (Dictionary or long) scrollLeft;
> (I don't believe that there is any way to say that one type is only valid for
> setters.)
> 
> This *should* "just work", but we don't have any real examples of union type
> attributes,
> so this may not work - this is a fault of the code generator.

This makes the code generator produce the error "UnionType is not supported"
(see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).


> In that case, you could instead write:
> [Custom] attribute (Dictionary or long) scrollLeft;

This makes the code generator #include the non-existent file
“V8UnionType=HASH(0x2b87de8).h” in V8Element.cpp.

However, writing:
[Custom=Setter] attribute long scrollLeft
works, and avoids having to write custom bindings for the getter. Does this
approach seem reasonable? Or is the completely general
[Custom] attribute any scrollLeft
preferable, even though it means also writing a custom getter?


> Much simpler would be to just have two methods and use overloading, with the
> second one RuntimeEnabled:
> void a(x, y);
> [RuntimeEnabled=Foo] void a(x, y, z);

The RuntimeEnabled gets ignored in this situation. When the order is swapped so
only the first one is marked RuntimeEnabled, then both methods become
runtime-enabled. I'll look into extending the CG to support this case -- any
pointers would be very much appreciated!

[Nils Barth (inactive), 2014-01-29 06:54:41]

On 2014/01/27 16:30:52, ajuma wrote:
> On 2014/01/27 05:03:49, Nils Barth wrote:
> > On 2014/01/27 02:52:15, ajuma wrote:
> > > Thanks for the comments!
> > > 
> > > Note that scrollLeft is an attribute, not a method, so we'd need something
> > like:
> > > 
> > > attribute Dictionary scrollLeft; // setter only
> > > attribute long scrollLeft;
> > 
> > Oh, for that you'd want to use a union type instead:
> > http://www.w3.org/TR/WebIDL/#idl-union
> > attribute (Dictionary or long) scrollLeft;
> > (I don't believe that there is any way to say that one type is only valid
for
> > setters.)
> > 
> > This *should* "just work", but we don't have any real examples of union type
> > attributes,
> > so this may not work - this is a fault of the code generator.
> 
> This makes the code generator produce the error "UnionType is not supported"
> (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

Oh, good point. There's an existing bug for this.

IDL: Support union types
https://code.google.com/p/chromium/issues/detail?id=240176


> > In that case, you could instead write:
> > [Custom] attribute (Dictionary or long) scrollLeft;
> 
> This makes the code generator #include the non-existent file
> “V8UnionType=HASH(0x2b87de8).h” in V8Element.cpp.

Ow, that's really unsupported.


> However, writing:
> [Custom=Setter] attribute long scrollLeft
> works, and avoids having to write custom bindings for the getter. Does this
> approach seem reasonable? Or is the completely general
> [Custom] attribute any scrollLeft
> preferable, even though it means also writing a custom getter?

Yes, good point!
Best would be:
// FIXME: should be: attribute (Dictionary or long) scrollLeft;
http://crbug.com/240176
[Custom=Setter] attribute long scrollLeft
...as we want to minimize custom code; thanks!

> > Much simpler would be to just have two methods and use overloading, with the
> > second one RuntimeEnabled:
> > void a(x, y);
> > [RuntimeEnabled=Foo] void a(x, y, z);
> 
> The RuntimeEnabled gets ignored in this situation. When the order is swapped
so
> only the first one is marked RuntimeEnabled, then both methods become
> runtime-enabled. I'll look into extending the CG to support this case -- any
> pointers would be very much appreciated!

Thanks, good point!
I've opened a new bug for this:
[RuntimeEnabled] on overloaded methods
https://code.google.com/p/chromium/issues/detail?id=339000

For now, easiest is probably:
// FIXME: http://crbug.com/339000 should be:
// void a(x, y);
// [RuntimeEnabled=Foo] void a(x, y, z);
void a(x, y, optional z);
...and then add the runtime features check in the Blink code.

Does this work?

[ajuma, 2014-01-29 16:52:17]

On 2014/01/29 06:54:41, Nils Barth wrote:

> Best would be:
> // FIXME: should be: attribute (Dictionary or long) scrollLeft;
> http://crbug.com/240176
> [Custom=Setter] attribute long scrollLeft
> ...as we want to minimize custom code; thanks!

Done.


> For now, easiest is probably:
> // FIXME: http://crbug.com/339000 should be:
> // void a(x, y);
> // [RuntimeEnabled=Foo] void a(x, y, z);
> void a(x, y, optional z);
> ...and then add the runtime features check in the Blink code.
> 
> Does this work?

Done. There's only one issue with this: when a third argument is provided, its
type gets checked (by the automatically generated bindings code) regardless of
whether the runtime flag is set (but we do correctly ignore this argument in
core/ when the flag isn't set, so the feature isn't actually exposed to the
web). More concretely, calling window.scrollTo(2, 3, 4) results in "TypeError:
Failed to execute 'scrollTo' on 'Window': parameter 3 ('scrollOptions') is not
an object" even when the runtime flag isn't set. Is this behavior a reasonable
price to pay for avoiding custom bindings?

As another alternative, we could use type 'any' for the third argument, and then
add type-checking inside core/ that only gets executed when the runtime flag is
set, but such an approach doesn't really seem that much better custom bindings.

[ajuma, 2014-01-29 16:53:20]

On 2014/01/29 16:52:17, ajuma wrote:
> On 2014/01/29 06:54:41, Nils Barth wrote:
> 
> > Best would be:
> > // FIXME: should be: attribute (Dictionary or long) scrollLeft;
> > http://crbug.com/240176
> > [Custom=Setter] attribute long scrollLeft
> > ...as we want to minimize custom code; thanks!
> 
> Done.
> 
> 
> > For now, easiest is probably:
> > // FIXME: http://crbug.com/339000 should be:
> > // void a(x, y);
> > // [RuntimeEnabled=Foo] void a(x, y, z);
> > void a(x, y, optional z);
> > ...and then add the runtime features check in the Blink code.
> > 
> > Does this work?
> 
> Done. There's only one issue with this: when a third argument is provided, its
> type gets checked (by the automatically generated bindings code) regardless of
> whether the runtime flag is set (but we do correctly ignore this argument in
> core/ when the flag isn't set, so the feature isn't actually exposed to the
> web). More concretely, calling window.scrollTo(2, 3, 4) results in "TypeError:
> Failed to execute 'scrollTo' on 'Window': parameter 3 ('scrollOptions') is not
> an object" even when the runtime flag isn't set. Is this behavior a reasonable
> price to pay for avoiding custom bindings?
> 
> As another alternative, we could use type 'any' for the third argument, and
then
> add type-checking inside core/ that only gets executed when the runtime flag
is
> set, but such an approach doesn't really seem that much better custom
bindings.

that much better *than custom bindings

[Nils Barth (inactive), 2014-01-30 00:57:48]

On 2014/01/29 16:52:17, ajuma wrote:
> > For now, easiest is probably:
> > // FIXME: http://crbug.com/339000 should be:
> > // void a(x, y);
> > // [RuntimeEnabled=Foo] void a(x, y, z);
> > void a(x, y, optional z);
> > ...and then add the runtime features check in the Blink code.
> > 
> > Does this work?
> 
> Done. There's only one issue with this: when a third argument is provided, its
> type gets checked (by the automatically generated bindings code) regardless of
> whether the runtime flag is set (but we do correctly ignore this argument in
> core/ when the flag isn't set, so the feature isn't actually exposed to the
> web). More concretely, calling window.scrollTo(2, 3, 4) results in "TypeError:
> Failed to execute 'scrollTo' on 'Window': parameter 3 ('scrollOptions') is not
> an object" even when the runtime flag isn't set. Is this behavior a reasonable
> price to pay for avoiding custom bindings?
> 
> As another alternative, we could use type 'any' for the third argument, and
then
> add type-checking inside core/ that only gets executed when the runtime flag
is
> set, but such an approach doesn't really seem that much better custom
bindings.

IDLs LGTM - thanks!

Your analysis looks spot-on.
I don't know which is preferable: haraken?, jochen?

I think the simplest one is ok (option 1: mark as optional Dictionary):
The incorrect call scrollTo(2, 3, 4) yields a type error, instead of being the
3rd argument being ignored.

We do want to avoid custom bindings not just b/c of excess code, but b/c it gets
out of sync,
so marking 'any' and type-checking in core/ (option 2) is preferable to [Custom]
(both involve excess code, but in core/ it's more robust).

I'm ok with option 1 (optional Dictionary); what do others think?

[haraken, 2014-01-30 01:14:01]

> Your analysis looks spot-on.
> I don't know which is preferable: haraken?, jochen?
> 
> I think the simplest one is ok (option 1: mark as optional Dictionary):
> The incorrect call scrollTo(2, 3, 4) yields a type error, instead of being the
> 3rd argument being ignored.
> 
> We do want to avoid custom bindings not just b/c of excess code, but b/c it
gets
> out of sync,
> so marking 'any' and type-checking in core/ (option 2) is preferable to
[Custom]
> (both involve excess code, but in core/ it's more robust).
> 
> I'm ok with option 1 (optional Dictionary); what do others think?

Option 1 sounds fine.

I'm still not that happy about the custom bindings in the CL. How would it be
hard to support [RuntimeEnabled] on method arguments?

[Nils Barth (inactive), 2014-01-30 01:23:20]

On 2014/01/30 01:14:01, haraken wrote:
> I'm still not that happy about the custom bindings in the CL. How would it be
> hard to support [RuntimeEnabled] on method arguments?

It's pretty involved; I can make it a priority post-rewrite though.

Implementation-wise, this is overloading, rather than on arguments:
[RuntimeEnabled] on overloaded methods
https://code.google.com/p/chromium/issues/detail?id=339000

It's pretty involved b/c we'd need to treat runtime feature checks differently
for overloaded functions and non-overloaded functions, move the runtime feature
checks
around in the code, and for overloaded methods, add separate checks across all
members
of the overload set and for each overloaded method.

This is much easier to do following the overload resolution algorithm rewrite,
as that's going to completely rewrite the generation logic anyway,
so I've marked it as blocked on that bug.

(It would probably be possible to hack around it, but this will be much easier
post-rewrite.)

[haraken, 2014-01-30 01:26:58]

On 2014/01/30 01:23:20, Nils Barth wrote:
> On 2014/01/30 01:14:01, haraken wrote:
> > I'm still not that happy about the custom bindings in the CL. How would it
be
> > hard to support [RuntimeEnabled] on method arguments?
> 
> It's pretty involved; I can make it a priority post-rewrite though.
> 
> Implementation-wise, this is overloading, rather than on arguments:
> [RuntimeEnabled] on overloaded methods
> https://code.google.com/p/chromium/issues/detail?id=339000
> 
> It's pretty involved b/c we'd need to treat runtime feature checks differently
> for overloaded functions and non-overloaded functions, move the runtime
feature
> checks
> around in the code, and for overloaded methods, add separate checks across all
> members
> of the overload set and for each overloaded method.
> 
> This is much easier to do following the overload resolution algorithm rewrite,
> as that's going to completely rewrite the generation logic anyway,
> so I've marked it as blocked on that bug.
> 
> (It would probably be possible to hack around it, but this will be much easier
> post-rewrite.)

OK, then LGTM for bindings/. Shall we add a FIXME why they have to be written in
custom bindings?

[Nils Barth (inactive), 2014-01-30 01:54:24]

On 2014/01/30 01:26:58, haraken wrote:
> OK, then LGTM for bindings/. Shall we add a FIXME why they have to be written
in
> custom bindings?

Yup, he's already done so, linking to the bug.

The custom bindings are actually just [Custom=Setter] on an attribute,
b/c we don't support union types on attributes:
IDL: union types
http://crbug.com/240176

For the methods there are no custom bindings!

[ajuma, 2014-01-30 02:51:08]

jochen, please have a look at core/ and platform/.

nbarth and haraken, thanks for the reviews!

[ajuma, 2014-02-03 14:31:33]

On 2014/01/30 02:51:08, ajuma wrote:
> jochen, please have a look at core/ and platform/.

jochen, friendly ping.

[jochen (gone - plz use gerrit), 2014-02-03 14:42:44]

lgtm

[ajuma, 2014-02-03 15:53:02]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-02-03 15:53:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/146003002/270001

[commit-bot: I haz the power, 2014-02-03 17:15:18]

Change committed as 166322

[commit-bot: I haz the power, 2014-02-03 17:15:19]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 17:15:19]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 17:15:20]

CQ bit was unchecked on CL. Ignoring.

[Nils Barth (inactive), 2014-06-13 04:50:20]

BTW, the FIXME/cruft this CL introduced is fixed in:
Cleanup [RuntimeEnabled] use in Window
https://codereview.chromium.org/329303009/
...which we can do now that [RuntimeEnabled] works on overloaded methods!