Expose scroll customization for touch to JS (behind REF).

Expose scroll customization for touch to JS (behind REF).

This enables scroll customization of touch scrolls for JS.  See
http://goo.gl/xCBm7L for details. This is a followup to
https://codereview.chromium.org/988823003, and completes landing
https://codereview.chromium.org/850443002/.

BUG=410974

[tdresser, 2015-04-07 21:13:15]

tdresser@chromium.org changed reviewers:
+ rbyers@chromium.org

[tdresser, 2015-04-07 21:13:16]

I'll likely want to get another pair of eyes on the V8 binding stuff before
landing (callScrollCustomizationV8Method in particular), but if you can give
this an initial review, that would be great!

[Rick Byers, 2015-04-10 14:51:44]

Looking great!

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html:37:
window.internals.setScrollChain(scrollState, elements);
To what extent do you think you could test this API without relying on
window.internals?  Eg. what's our synthetic JS story here wrt. the scroll chain
again?

If there's some behavior that can only be (or is really best) tested via
window.internals, then let's try to put that in separate files.  The ones that
don't depend on window.internals can be immediately run by other browsers (eg.
as part of a future w3c test suite).  For tests that don't otherwise depend on
window.internals you should use feature detection (eg. 'applyScroll in
Element.prototype') instead of
window.internals.runtimeFlags.scrollCustomizationEnabled.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:5:
<title>ScrollState constructor behaves correctly</title>
update title

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:65:
"window.eventSender exposed, and require scroll customization.");
Do you think we should be making these tests explicitly fail in this case?  It
still makes me nervous seeing the tests pass both inside and outside your
virtual test suite without any output to prove they're actually doing their job.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:93:
Element.prototype.applyScroll = function(scrollState) {
Perhaps you should use the likely more common pattern of setting the custom
applyScroll on each element instance itself and using ES6 'super' to chain to
the built-in implementation?  If you like you could do applyScroll that way and
leave distributeScroll as-is to cover both patterns.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:128:
assert_equals(document.body.scrollTop, bodyScrollTop[i], "For body on step " +
i);
nit: use documentElement instead of body.  body.scrollTop behavior in this case
is legacy, for compat only.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.idl:120: // Experimental ScrollCustomization API
move this below the below methods (this isn't part of the 'non-standard APIs'
referred to by the above comment - the intent is that it will become standard).

Would be nice to add a bug link for more details on the experiment status.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.idl:121: [RuntimeEnabled=ScrollCustomization] void
applyScroll(ScrollState scrollState);
Now that attributes are exposed on the prototype chain, can we do this properly
and make these function-type attributes instead of operations?  You can now
explicitly add the '[ExposeJSAccessors]' attribute to make it clear these
attributes must have getters/setters on the prototype chain (regardless of the
default setting).

This will of course change (hopefully vastly simplify) your bindings logic.  But
my understanding is that there's no longer any reason to depend on the old
broken behavior.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1057603002/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:211: if (curElement ==
frame.document()->documentElement())
why doesn't the documentElement get put on the scroll chain?  You can customize
both the documentElement and body, right?

[tdresser, 2015-05-08 18:23:00]

tdresser@chromium.org changed reviewers:
+ haraken@chromium.org

[tdresser, 2015-05-08 18:23:02]

There are still a few test failures I need to look into, but this is pretty
close.

I suspect the bindings code will need to be rehauled though, it's a little ugly
currently.

haraken@, what's the best way of approaching the bindings in Element.cpp?

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-distribute-to-scroll-chain-descendant.html:37:
window.internals.setScrollChain(scrollState, elements);
On 2015/04/10 14:51:44, Rick Byers wrote:
> To what extent do you think you could test this API without relying on
> window.internals?  Eg. what's our synthetic JS story here wrt. the scroll
chain
> again?
> 
> If there's some behavior that can only be (or is really best) tested via
> window.internals, then let's try to put that in separate files.  The ones that
> don't depend on window.internals can be immediately run by other browsers (eg.
> as part of a future w3c test suite).  For tests that don't otherwise depend on
> window.internals you should use feature detection (eg. 'applyScroll in
> Element.prototype') instead of
> window.internals.runtimeFlags.scrollCustomizationEnabled.

I've filed a bug to enable setting the scroll chain here:
http://crbug.com/483091.
I'll update this test once that API exists.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:5:
<title>ScrollState constructor behaves correctly</title>
On 2015/04/10 14:51:44, Rick Byers wrote:
> update title

Done.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:65:
"window.eventSender exposed, and require scroll customization.");
On 2015/04/10 14:51:44, Rick Byers wrote:
> Do you think we should be making these tests explicitly fail in this case?  It
> still makes me nervous seeing the tests pass both inside and outside your
> virtual test suite without any output to prove they're actually doing their
job.

Done.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:93:
Element.prototype.applyScroll = function(scrollState) {
On 2015/04/10 14:51:44, Rick Byers wrote:
> Perhaps you should use the likely more common pattern of setting the custom
> applyScroll on each element instance itself and using ES6 'super' to chain to
> the built-in implementation?  If you like you could do applyScroll that way
and
> leave distributeScroll as-is to cover both patterns.

Now that we're using attributes, setting the methods directly on their elements
is the only option. Using closures here feels a bit ugly, but I'm not sure what
a cleaner option would be. Does this seem reasonable?

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:128:
assert_equals(document.body.scrollTop, bodyScrollTop[i], "For body on step " +
i);
On 2015/04/10 14:51:44, Rick Byers wrote:
> nit: use documentElement instead of body.  body.scrollTop behavior in this
case
> is legacy, for compat only.

Done, though I had to enable ScrollTopLeftInterop in the virtual test suite.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.idl:120: // Experimental ScrollCustomization API
On 2015/04/10 14:51:44, Rick Byers wrote:
> move this below the below methods (this isn't part of the 'non-standard APIs'
> referred to by the above comment - the intent is that it will become
standard).
> 
> Would be nice to add a bug link for more details on the experiment status.

Moved up, not down, as it leaves the standard APIs contiguous.

Added bug link.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/dom/Element...
Source/core/dom/Element.idl:121: [RuntimeEnabled=ScrollCustomization] void
applyScroll(ScrollState scrollState);
On 2015/04/10 14:51:44, Rick Byers wrote:
> Now that attributes are exposed on the prototype chain, can we do this
properly
> and make these function-type attributes instead of operations?  You can now
> explicitly add the '[ExposeJSAccessors]' attribute to make it clear these
> attributes must have getters/setters on the prototype chain (regardless of the
> default setting).
> 
> This will of course change (hopefully vastly simplify) your bindings logic. 
But
> my understanding is that there's no longer any reason to depend on the old
> broken behavior.

I'm not convinced this is a simplification, but I think it is the right thing to
do.

Done, though there will likely need to be some refactoring done in the bindings
logic before landing.

https://codereview.chromium.org/1057603002/diff/20001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1057603002/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:211: if (curElement ==
frame.document()->documentElement())
I've clarified this in a comment. This only matters in quirks mode.

[tdresser, 2015-05-08 19:38:34]

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:128:
assert_equals(document.body.scrollTop, bodyScrollTop[i], "For body on step " +
i);
On 2015/05/08 18:23:01, tdresser wrote:
> On 2015/04/10 14:51:44, Rick Byers wrote:
> > nit: use documentElement instead of body.  body.scrollTop behavior in this
> case
> > is legacy, for compat only.
> 
> Done, though I had to enable ScrollTopLeftInterop in the virtual test suite.

Correction, I just had to switch to using scrollingElement.

[haraken, 2015-05-09 15:28:53]

> I suspect the bindings code will need to be rehauled though, it's a little
ugly
> currently.
> 
> haraken@, what's the best way of approaching the bindings in Element.cpp?

This kind of customized binding implementation should go to
Source/bindings/core/v8/custom/.

However, it is highly deprecated that you add customized binding implementations
(because in most cases you're doing something not right, or you need to improve
the IDL compiler so that it can auto-generate the bindings).

Let's chat about it at BlinkOn!

[tdresser, 2015-06-08 15:37:36]

I've switched away from using an attribute for applyScroll/distributeScroll.

The plan is to land this to enable folks to play with it a bit, but we'll end up
moving the bindings around once the custom paint/custom layout folks have
finalized how their bindings will work.

haraken@, there's a lot less bindings nastiness in this version, but there's
still some.

Can you take a look at Element::callScrollCustomizationV8Method? Does this look
at all reasonable to you? If so, where should that code live?

This is behind a flag, and when we align with custom paint / layout, we'll
probably change this a lot, but for now I'm leaning towards something like this
approach.

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
File
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html
(right):

https://codereview.chromium.org/1057603002/diff/20001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/scroll-customization/touch-scroll-customization.html:93:
Element.prototype.applyScroll = function(scrollState) {
On 2015/05/08 18:23:02, tdresser wrote:
> On 2015/04/10 14:51:44, Rick Byers wrote:
> > Perhaps you should use the likely more common pattern of setting the custom
> > applyScroll on each element instance itself and using ES6 'super' to chain
to
> > the built-in implementation?  If you like you could do applyScroll that way
> and
> > leave distributeScroll as-is to cover both patterns.
> 
> Now that we're using attributes, setting the methods directly on their
elements
> is the only option. Using closures here feels a bit ugly, but I'm not sure
what
> a cleaner option would be. Does this seem reasonable?

I'm not sure how I would use the super keyword when setting applyScroll on each
instance.
The super keyword only works within the context of an es6 class, doesn't it?

https://codereview.chromium.org/1057603002/diff/140001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/140001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:504: if (function->ScriptId() ==
v8::UnboundScript::kNoScriptId)
I'm not certain that this is a reliable way to check if a function is native.

This would also behave oddly if you set element.applyScroll = otherNativeMethod.

[haraken, 2015-06-09 00:07:13]

This looks much nicer but I want to see more clean-ups :)

Ideally you won't need to write any binding code at all. The applyScroll()
should be speced so that it receives a callback interface (i.e.,
ScrollStateCallback) as an argument. Then core/ can just invokes the callback
with callback->handleEvent(). That's the way normal callback interfaces work and
we should align with that model here as well.

For more details, I think you can look at how
Window.requestAnimationFrame(FrameRequestCallback) is implemented. The
FrameRequestCallback is passed into core/ and then invoked by
FrameRequestCallbackCollection.cpp:

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

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:491: ScriptState* scriptState =
ScriptState::forMainWorld(frame);

Why can we assume the main world?

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:507: ScriptFunctionCall functionCall(thisWrapper,
methodName);

ScriptFunctionCall is an abstraction for devtools. Can we use
V8ScriptRunner::callFunction?

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.idl:23: callback ScrollStateCallback = void (ScrollState
scrollState);

Where is this callback used?

[tdresser, 2015-06-09 14:27:42]

The fundamental difference between this and requestAnimationFrame is that
applyScroll and distributeScroll require default values.

If applyScroll and distributeScroll are methods that take a callback interface,
how do web developers refer to the default implementations of applyScroll and
distributeScroll?

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:491: ScriptState* scriptState =
ScriptState::forMainWorld(frame);
On 2015/06/09 00:07:13, haraken wrote:
> 
> Why can we assume the main world?

We're okay if scroll customization methods can only be assigned from the main
world.

My understanding is that currently, if you assign a scroll customization method
from a world other than the main world, this code will pay no attention to it.
Is that correct?

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:507: ScriptFunctionCall functionCall(thisWrapper,
methodName);
On 2015/06/09 00:07:12, haraken wrote:
> 
> ScriptFunctionCall is an abstraction for devtools. Can we use
> V8ScriptRunner::callFunction?
> 

Done.

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.idl:23: callback ScrollStateCallback = void (ScrollState
scrollState);
On 2015/06/09 00:07:13, haraken wrote:
> 
> Where is this callback used?

Whoops, removed.

[haraken, 2015-06-09 15:16:37]

haraken@chromium.org changed reviewers:
+ blink-reviews-bindings@chromium.org

[haraken, 2015-06-09 15:16:38]

+binding-reviews

> The fundamental difference between this and requestAnimationFrame is that
> applyScroll and distributeScroll require default values.
>
> If applyScroll and distributeScroll are methods that take a callback
interface,
> how do web developers refer to the default implementations of applyScroll and
> distributeScroll?

Your approach will "work", but this is not a standard way to implement this kind
of behavior. Callback interface is the standard way. If you have to go with a
different approach than other web APIs, you'll need a strong justification for
that.

I'm curious why something like the following doesn't work:

  distributeScroll();  // Default behavior.
  distributeScroll(function f() { });  // Customized behavior.

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:491: ScriptState* scriptState =
ScriptState::forMainWorld(frame);
On 2015/06/09 14:27:42, tdresser wrote:
> On 2015/06/09 00:07:13, haraken wrote:
> > 
> > Why can we assume the main world?
> 
> We're okay if scroll customization methods can only be assigned from the main
> world.

That is not OK. Unless you have a strong reason to do so, we should support the
Web API in isolated worlds as well.

> My understanding is that currently, if you assign a scroll customization
method
> from a world other than the main world, this code will pay no attention to it.
> Is that correct?

If an isolated world assigns a scroll customization method, it will install the
method on the main world -- which is not correct.

Assuming that callScrollCustomizationV8Method is called from only JavaScript, I
think you can use:

  ScriptState* scriptState = ScriptState::current(toIsolate(frame));

which will give you the ScriptState of the JS that called the
callScrollCustomizationV8Method.

(However, I'd strongly recommend using a callback interface because that's a
standard way to handle this kind of situation in a safe manner.)

https://codereview.chromium.org/1057603002/diff/180001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/180001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:507: V8ScriptRunner scriptRunner;

We should have DISALLOW_ALLOCATION on V8ScriptRunner... it shouldn't be
instantiated.

https://codereview.chromium.org/1057603002/diff/180001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:510: scriptRunner.callFunction(function,
scriptState->executionContext(), thisObject, 1, info, scriptState->isolate());

Use V8ScriptRunner::.callFunction().

[tdresser, 2015-06-09 15:43:11]

> I'm curious why something like the following doesn't work:
> 
>   distributeScroll();  // Default behavior.
>   distributeScroll(function f() { });  // Customized behavior.

I don't think this API is very easy to understand. It would be clearer as:
distributeScroll(scrollState); // Default behavior
setDistributeScroll(function f() {}); // Customized behavior


I also need to be able to access the most recent assigned behavior.
Consider the following case:

var el = document.getElementById('el');

function customScrollA(scrollState) {
  console.log("Custom scroll A");
  el.distributeScroll(scrollState);
};

el.distributeScroll(customScrollA);

function customScrollB(scrollState) {
  console.log("Custom scroll B");
  el.distributeScroll(scrollState);
  console.log(el.distributeScroll = customScrollA); // [*]
}

[*] el.distributeScroll here needs to be customScrollA, not the default scroll
behavior.

We could get most of the properties we want by just ignoring direct assignment
on distributeScroll and applyScroll, and only allowing assignment through an
explicit setter. Having the setter take a callback interface seems reasonable to
me.

Does that approach sound okay to you?

The implementation of the distributeScroll method would be a little awkward.
Element::distributeScroll(ScrollState& scrollState) {
  if (not overridden)
    performNativeScroll();
  else
    performCustomScroll();
}

If this sounds okay to you, I'll go ahead and switch to using this approach.

[haraken, 2015-06-09 16:05:23]

On 2015/06/09 15:43:11, tdresser wrote:
> > I'm curious why something like the following doesn't work:
> > 
> >   distributeScroll();  // Default behavior.
> >   distributeScroll(function f() { });  // Customized behavior.
> 
> I don't think this API is very easy to understand. It would be clearer as:
> distributeScroll(scrollState); // Default behavior
> setDistributeScroll(function f() {}); // Customized behavior

In this case, how can web developers get the default behavior after calling
setDistributionScroll?

(I don't have a strong opinion about how the API looks like -- you should be an
expert :) I just want you to design the API following the way other APIs are
speced.)

[Rick Byers, 2015-06-09 16:21:45]

On 2015/06/09 16:05:23, haraken wrote:
> On 2015/06/09 15:43:11, tdresser wrote:
> > > I'm curious why something like the following doesn't work:
> > > 
> > >   distributeScroll();  // Default behavior.
> > >   distributeScroll(function f() { });  // Customized behavior.
> > 
> > I don't think this API is very easy to understand. It would be clearer as:
> > distributeScroll(scrollState); // Default behavior
> > setDistributeScroll(function f() {}); // Customized behavior
> 
> In this case, how can web developers get the default behavior after calling
> setDistributionScroll?
> 
> (I don't have a strong opinion about how the API looks like -- you should be
an
> expert :) I just want you to design the API following the way other APIs are
> speced.)

Note that the fundamental problem here is that there's a rich composition design
pattern prevalent in every other major UI framework that (amazingly) has never
been attempted on the web before.  Real extensible UI frameworks rely on OO
paradigms for extensibility, but we've never had object oriented CSSOM (because
we've never really had extensibility in CSS).  This is a deficiency of the web
platform that we'll ultimately have to correct.  So there's no getting around
the fact that scroll customization will feel not entirely webby.  In fact this
is exactly one of the main problems we're working to fix - "feeling webby" and
"extensible" are currently somewhat at odds.  Custom layout and custom paint are
hitting the same thing.

That said, this is ultimately a debate for the Houdini group at CSS WG, and we
should have it in the larger context of all of these different extensibility
APIs.  So for now I'm happy using a hacky API that is easy to implement and
exposes all the power we need exposed, even if it's ugly / unnatural.  We'll fix
the actual API shape later (probably involving a bunch of W3C debate and then
bindings work!).

Sound reasonable?

[tdresser, 2015-06-09 16:47:36]

On 2015/06/09 16:21:45, Rick Byers wrote:
> On 2015/06/09 16:05:23, haraken wrote:
> > On 2015/06/09 15:43:11, tdresser wrote:
> > > > I'm curious why something like the following doesn't work:
> > > > 
> > > >   distributeScroll();  // Default behavior.
> > > >   distributeScroll(function f() { });  // Customized behavior.
> > > 
> > > I don't think this API is very easy to understand. It would be clearer as:
> > > distributeScroll(scrollState); // Default behavior
> > > setDistributeScroll(function f() {}); // Customized behavior
> > 
> > In this case, how can web developers get the default behavior after calling
> > setDistributionScroll?
> > 
> > (I don't have a strong opinion about how the API looks like -- you should be
> an
> > expert :) I just want you to design the API following the way other APIs are
> > speced.)
> 
> Note that the fundamental problem here is that there's a rich composition
design
> pattern prevalent in every other major UI framework that (amazingly) has never
> been attempted on the web before.  Real extensible UI frameworks rely on OO
> paradigms for extensibility, but we've never had object oriented CSSOM
(because
> we've never really had extensibility in CSS).  This is a deficiency of the web
> platform that we'll ultimately have to correct.  So there's no getting around
> the fact that scroll customization will feel not entirely webby.  In fact this
> is exactly one of the main problems we're working to fix - "feeling webby" and
> "extensible" are currently somewhat at odds.  Custom layout and custom paint
are
> hitting the same thing.
> 
> That said, this is ultimately a debate for the Houdini group at CSS WG, and we
> should have it in the larger context of all of these different extensibility
> APIs.  So for now I'm happy using a hacky API that is easy to implement and
> exposes all the power we need exposed, even if it's ugly / unnatural.  We'll
fix
> the actual API shape later (probably involving a bunch of W3C debate and then
> bindings work!).
> 
> Sound reasonable?

Sorry, I wasn't very clear when I said developers need to be able to access the
default values.
Developers need to be able to reference the default values, but it's fine if
they can only access them before a new handler has been assigned. In theory, a
framework can always save off the default implementation and use it later,
though this requires that no-one modifies the scroll customization methods
beforehand.

var originalApplyScroll = element.applyScroll;

function customScroll(scrollState) {
  originalApplyScroll.call(element, scrollState);
}

[haraken, 2015-06-10 00:44:50]

On 2015/06/09 16:47:36, tdresser wrote:
> On 2015/06/09 16:21:45, Rick Byers wrote:
> > On 2015/06/09 16:05:23, haraken wrote:
> > > On 2015/06/09 15:43:11, tdresser wrote:
> > > > > I'm curious why something like the following doesn't work:
> > > > > 
> > > > >   distributeScroll();  // Default behavior.
> > > > >   distributeScroll(function f() { });  // Customized behavior.
> > > > 
> > > > I don't think this API is very easy to understand. It would be clearer
as:
> > > > distributeScroll(scrollState); // Default behavior
> > > > setDistributeScroll(function f() {}); // Customized behavior
> > > 
> > > In this case, how can web developers get the default behavior after
calling
> > > setDistributionScroll?
> > > 
> > > (I don't have a strong opinion about how the API looks like -- you should
be
> > an
> > > expert :) I just want you to design the API following the way other APIs
are
> > > speced.)
> > 
> > Note that the fundamental problem here is that there's a rich composition
> design
> > pattern prevalent in every other major UI framework that (amazingly) has
never
> > been attempted on the web before.  Real extensible UI frameworks rely on OO
> > paradigms for extensibility, but we've never had object oriented CSSOM
> (because
> > we've never really had extensibility in CSS).  This is a deficiency of the
web
> > platform that we'll ultimately have to correct.  So there's no getting
around
> > the fact that scroll customization will feel not entirely webby.  In fact
this
> > is exactly one of the main problems we're working to fix - "feeling webby"
and
> > "extensible" are currently somewhat at odds.  Custom layout and custom paint
> are
> > hitting the same thing.
> > 
> > That said, this is ultimately a debate for the Houdini group at CSS WG, and
we
> > should have it in the larger context of all of these different extensibility
> > APIs.  So for now I'm happy using a hacky API that is easy to implement and
> > exposes all the power we need exposed, even if it's ugly / unnatural.  We'll
> fix
> > the actual API shape later (probably involving a bunch of W3C debate and
then
> > bindings work!).
> > 
> > Sound reasonable?

Thanks for the context -- sounds reasonable. You're going to implement a
somewhat new model that the current web APIs don't provide, so you may need a
new implementation pattern.

Then I'm OK with implementing something unstable but want to avoid implementing
something dangerous (from the perspective of binding security). For example, the
current implementation has a risk of leaking JS objects from an isolated world
to the main world. There would be a way to work around the issues, but the
easiest way would be just to follow a standard pattern that is already known to
be safe (until we're pretty sure that we need to introduce a new model and solve
a bunch of problems associated with the model).

From that perspective, the idea of setDistributeScroll sounds fine (proposed by
tdresser in comment #13). Shall we go with setDistributeScroll in short term?

[Rick Byers, 2015-06-11 03:05:26]

On 2015/06/10 00:44:50, haraken wrote:
> On 2015/06/09 16:47:36, tdresser wrote:
> > On 2015/06/09 16:21:45, Rick Byers wrote:
> > > On 2015/06/09 16:05:23, haraken wrote:
> > > > On 2015/06/09 15:43:11, tdresser wrote:
> > > > > > I'm curious why something like the following doesn't work:
> > > > > > 
> > > > > >   distributeScroll();  // Default behavior.
> > > > > >   distributeScroll(function f() { });  // Customized behavior.
> > > > > 
> > > > > I don't think this API is very easy to understand. It would be clearer
> as:
> > > > > distributeScroll(scrollState); // Default behavior
> > > > > setDistributeScroll(function f() {}); // Customized behavior
> > > > 
> > > > In this case, how can web developers get the default behavior after
> calling
> > > > setDistributionScroll?
> > > > 
> > > > (I don't have a strong opinion about how the API looks like -- you
should
> be
> > > an
> > > > expert :) I just want you to design the API following the way other APIs
> are
> > > > speced.)
> > > 
> > > Note that the fundamental problem here is that there's a rich composition
> > design
> > > pattern prevalent in every other major UI framework that (amazingly) has
> never
> > > been attempted on the web before.  Real extensible UI frameworks rely on
OO
> > > paradigms for extensibility, but we've never had object oriented CSSOM
> > (because
> > > we've never really had extensibility in CSS).  This is a deficiency of the
> web
> > > platform that we'll ultimately have to correct.  So there's no getting
> around
> > > the fact that scroll customization will feel not entirely webby.  In fact
> this
> > > is exactly one of the main problems we're working to fix - "feeling webby"
> and
> > > "extensible" are currently somewhat at odds.  Custom layout and custom
paint
> > are
> > > hitting the same thing.
> > > 
> > > That said, this is ultimately a debate for the Houdini group at CSS WG,
and
> we
> > > should have it in the larger context of all of these different
extensibility
> > > APIs.  So for now I'm happy using a hacky API that is easy to implement
and
> > > exposes all the power we need exposed, even if it's ugly / unnatural. 
We'll
> > fix
> > > the actual API shape later (probably involving a bunch of W3C debate and
> then
> > > bindings work!).
> > > 
> > > Sound reasonable?
> 
> Thanks for the context -- sounds reasonable. You're going to implement a
> somewhat new model that the current web APIs don't provide, so you may need a
> new implementation pattern.
> 
> Then I'm OK with implementing something unstable but want to avoid
implementing
> something dangerous (from the perspective of binding security). For example,
the
> current implementation has a risk of leaking JS objects from an isolated world
> to the main world. There would be a way to work around the issues, but the
> easiest way would be just to follow a standard pattern that is already known
to
> be safe (until we're pretty sure that we need to introduce a new model and
solve
> a bunch of problems associated with the model).
> 
> From that perspective, the idea of setDistributeScroll sounds fine (proposed
by
> tdresser in comment #13). Shall we go with setDistributeScroll in short term?

That's fine with me as an incremental step forward...  Thanks!

[tdresser, 2015-06-12 15:53:08]

On 2015/06/11 03:05:26, Rick Byers wrote:
> On 2015/06/10 00:44:50, haraken wrote:
> > On 2015/06/09 16:47:36, tdresser wrote:
> > > On 2015/06/09 16:21:45, Rick Byers wrote:
> > > > On 2015/06/09 16:05:23, haraken wrote:
> > > > > On 2015/06/09 15:43:11, tdresser wrote:
> > > > > > > I'm curious why something like the following doesn't work:
> > > > > > > 
> > > > > > >   distributeScroll();  // Default behavior.
> > > > > > >   distributeScroll(function f() { });  // Customized behavior.
> > > > > > 
> > > > > > I don't think this API is very easy to understand. It would be
clearer
> > as:
> > > > > > distributeScroll(scrollState); // Default behavior
> > > > > > setDistributeScroll(function f() {}); // Customized behavior
> > > > > 
> > > > > In this case, how can web developers get the default behavior after
> > calling
> > > > > setDistributionScroll?
> > > > > 
> > > > > (I don't have a strong opinion about how the API looks like -- you
> should
> > be
> > > > an
> > > > > expert :) I just want you to design the API following the way other
APIs
> > are
> > > > > speced.)
> > > > 
> > > > Note that the fundamental problem here is that there's a rich
composition
> > > design
> > > > pattern prevalent in every other major UI framework that (amazingly) has
> > never
> > > > been attempted on the web before.  Real extensible UI frameworks rely on
> OO
> > > > paradigms for extensibility, but we've never had object oriented CSSOM
> > > (because
> > > > we've never really had extensibility in CSS).  This is a deficiency of
the
> > web
> > > > platform that we'll ultimately have to correct.  So there's no getting
> > around
> > > > the fact that scroll customization will feel not entirely webby.  In
fact
> > this
> > > > is exactly one of the main problems we're working to fix - "feeling
webby"
> > and
> > > > "extensible" are currently somewhat at odds.  Custom layout and custom
> paint
> > > are
> > > > hitting the same thing.
> > > > 
> > > > That said, this is ultimately a debate for the Houdini group at CSS WG,
> and
> > we
> > > > should have it in the larger context of all of these different
> extensibility
> > > > APIs.  So for now I'm happy using a hacky API that is easy to implement
> and
> > > > exposes all the power we need exposed, even if it's ugly / unnatural. 
> We'll
> > > fix
> > > > the actual API shape later (probably involving a bunch of W3C debate and
> > then
> > > > bindings work!).
> > > > 
> > > > Sound reasonable?
> > 
> > Thanks for the context -- sounds reasonable. You're going to implement a
> > somewhat new model that the current web APIs don't provide, so you may need
a
> > new implementation pattern.
> > 
> > Then I'm OK with implementing something unstable but want to avoid
> implementing
> > something dangerous (from the perspective of binding security). For example,
> the
> > current implementation has a risk of leaking JS objects from an isolated
world
> > to the main world. There would be a way to work around the issues, but the
> > easiest way would be just to follow a standard pattern that is already known
> to
> > be safe (until we're pretty sure that we need to introduce a new model and
> solve
> > a bunch of problems associated with the model).
> > 
> > From that perspective, the idea of setDistributeScroll sounds fine (proposed
> by
> > tdresser in comment #13). Shall we go with setDistributeScroll in short
term?
> 
> That's fine with me as an incremental step forward...  Thanks!

My proposed solution doesn't work (though the bindings do become really
simple!).

We can't use my proposed:
Element::distributeScroll(ScrollState& scrollState) {
  if (not overridden)
    performNativeScroll();
  else
    performCustomScroll();
}

Because javascript which gets a handle to the native scroll behavior, and calls
setDistributeScroll on an element changes the behavior of it's handle to the
native scroll behavior.

I think haraken hinted at that earlier, but I missed it.


I don't think this is possible without some amount of custom bindings (unless
there's some existing bindings logic I'm not aware of, which is quite possible).

When exposing the current applyScroll / distributeScroll behavior to JS, we have
three options.
1. Expose these methods through a getter.
2. Expose the methods to JS directly (not using an attribute).
3. Call the custom methods defined in JavaScript from C++.

We can't mix options one and two, because triggering the native behavior must
have the same API as triggering custom behavior, as a web developer won't always
know which they're triggering.

With the first approach, we need to be able to return JS methods and native
methods through the same interface. Passing a native method out to JS requires
custom bindings to wrap it up in the appropriate v8 function type.

With the second approach, these methods must have non-constant behavior.
However, JS needs to be able to refer to the behavior they had at any point in
the program. This is impossible if the behavior is non-constant.

The third approach obviously requires custom bindings.


Is that argument sound, or am I missing something? Using a callback interface
solves the problem of how to set and execute custom behavior for a scroll
customization method, but it doesn't solve the problem of how to return it.
Ideally there would be some magic to convert from a callback interface to the
appropriate function type when you return it from a method, but as far as I can
tell there's currently only magic to convert a function to a callback interface,
not the other way around.

Thanks!

[haraken, 2015-06-13 04:03:32]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2015-06-13 04:03:33]

Just to clarify: I'm fine with adding some custom bindings if it needs some
customized behavior (we sometimes do this). What I want to avoid is to use a
non-standard way that has a risk.


> Is that argument sound, or am I missing something? Using a callback interface
> solves the problem of how to set and execute custom behavior for a scroll
> customization method, but it doesn't solve the problem of how to return it.
> Ideally there would be some magic to convert from a callback interface to the
> appropriate function type when you return it from a method, but as far as I
can
> tell there's currently only magic to convert a function to a callback
interface,
> not the other way around.

I think there should be a way to return a callback interface back to JS. I'm not
sure if there is any actual DOM attribute/method that returns a callback
interface, but as far as I read the spec, it should be supported.

https://heycam.github.io/webidl/#es-callback-function

If we don't have the ability, for example, we cannot implement
Element.getEventListener() etc. (Note: This is just an example. We shouldn't
implement Element.getEventListener() for security reasons)

shiino-san: Does the IDL compiler support a DOM attribute/method that returns a
callback interface?

[Yuki, 2015-06-15 05:51:08]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org

[Yuki, 2015-06-15 05:51:09]

+bashi, whether the code generator supports attributes/methods which return
callback interface.

[Yuki, 2015-06-15 09:27:10]

On 2015/06/15 05:51:09, Yuki wrote:
> +bashi, whether the code generator supports attributes/methods which return
> callback interface.

Can we use a function as callback function, or a dict as an instance of callback
interface?

[bashi, 2015-06-16 03:12:04]

On 2015/06/15 09:27:10, Yuki wrote:
> On 2015/06/15 05:51:09, Yuki wrote:
> > +bashi, whether the code generator supports attributes/methods which return
> > callback interface.
> 
> Can we use a function as callback function, or a dict as an instance of
callback
> interface?

I didn't follow the whole discussion, but it seems that we support
attributes/methods which return callback interfaces.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[tdresser, 2015-06-16 12:26:14]

On 2015/06/16 03:12:04, bashi1 wrote:
> On 2015/06/15 09:27:10, Yuki wrote:
> > On 2015/06/15 05:51:09, Yuki wrote:
> > > +bashi, whether the code generator supports attributes/methods which
return
> > > callback interface.
> > 
> > Can we use a function as callback function, or a dict as an instance of
> callback
> > interface?
> 
> I didn't follow the whole discussion, but it seems that we support
> attributes/methods which return callback interfaces.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

bashi, that's an example of returning a callback function, not a callback
interface, isn't it?

With a callback interface, I can easily return an instance of the interface
constructed in C++.
Is that possible with a callback function? I thought I tried to do this with a
callback function, but it looked like it would require some custom bindings. I
could be remembering incorrectly though.

[bashi, 2015-06-17 02:18:18]

On 2015/06/16 12:26:14, tdresser wrote:
> On 2015/06/16 03:12:04, bashi1 wrote:
> > On 2015/06/15 09:27:10, Yuki wrote:
> > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > +bashi, whether the code generator supports attributes/methods which
> return
> > > > callback interface.
> > > 
> > > Can we use a function as callback function, or a dict as an instance of
> > callback
> > > interface?
> > 
> > I didn't follow the whole discussion, but it seems that we support
> > attributes/methods which return callback interfaces.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> bashi, that's an example of returning a callback function, not a callback
> interface, isn't it?
> 
> With a callback interface, I can easily return an instance of the interface
> constructed in C++.
> Is that possible with a callback function? I thought I tried to do this with a
> callback function, but it looked like it would require some custom bindings. I
> could be remembering incorrectly though.

Ah, right. It's not a callback interface.

I read the current implementation and found that we only have basic support for
callback interface. It won't work as attributes/return values.

I don't fully understand the situation, but I wonder why your proposal
(providing setter for custom behavior) doesn't work. Do you have a concrete
example why it isn't work?

[tdresser, 2015-06-17 13:20:21]

On 2015/06/17 02:18:18, bashi1 wrote:
> On 2015/06/16 12:26:14, tdresser wrote:
> > On 2015/06/16 03:12:04, bashi1 wrote:
> > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > +bashi, whether the code generator supports attributes/methods which
> > return
> > > > > callback interface.
> > > > 
> > > > Can we use a function as callback function, or a dict as an instance of
> > > callback
> > > > interface?
> > > 
> > > I didn't follow the whole discussion, but it seems that we support
> > > attributes/methods which return callback interfaces.
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > bashi, that's an example of returning a callback function, not a callback
> > interface, isn't it?
> > 
> > With a callback interface, I can easily return an instance of the interface
> > constructed in C++.
> > Is that possible with a callback function? I thought I tried to do this with
a
> > callback function, but it looked like it would require some custom bindings.
I
> > could be remembering incorrectly though.
> 
> Ah, right. It's not a callback interface.
> 
> I read the current implementation and found that we only have basic support
for
> callback interface. It won't work as attributes/return values.
> 
> I don't fully understand the situation, but I wonder why your proposal
> (providing setter for custom behavior) doesn't work. Do you have a concrete
> example why it isn't work?

We need to be able to:
1. set custom behavior
2. return custom behavior
3. return default behavior (defined in C++)

We need to be able to return custom or default behavior from the same method.

Callback functions provide properties 1 and 2.
Callback interfaces give us 1 and 3.

It appears there currently isn't a solution which gives us all 3 required
properties.

[haraken, 2015-06-17 15:49:02]

On 2015/06/17 13:20:21, tdresser wrote:
> On 2015/06/17 02:18:18, bashi1 wrote:
> > On 2015/06/16 12:26:14, tdresser wrote:
> > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > +bashi, whether the code generator supports attributes/methods which
> > > return
> > > > > > callback interface.
> > > > > 
> > > > > Can we use a function as callback function, or a dict as an instance
of
> > > > callback
> > > > > interface?
> > > > 
> > > > I didn't follow the whole discussion, but it seems that we support
> > > > attributes/methods which return callback interfaces.
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > bashi, that's an example of returning a callback function, not a callback
> > > interface, isn't it?
> > > 
> > > With a callback interface, I can easily return an instance of the
interface
> > > constructed in C++.
> > > Is that possible with a callback function? I thought I tried to do this
with
> a
> > > callback function, but it looked like it would require some custom
bindings.
> I
> > > could be remembering incorrectly though.
> > 
> > Ah, right. It's not a callback interface.
> > 
> > I read the current implementation and found that we only have basic support
> for
> > callback interface. It won't work as attributes/return values.
> > 
> > I don't fully understand the situation, but I wonder why your proposal
> > (providing setter for custom behavior) doesn't work. Do you have a concrete
> > example why it isn't work?
> 
> We need to be able to:
> 1. set custom behavior
> 2. return custom behavior
> 3. return default behavior (defined in C++)
> 
> We need to be able to return custom or default behavior from the same method.
> 
> Callback functions provide properties 1 and 2.
> Callback interfaces give us 1 and 3.
> 
> It appears there currently isn't a solution which gives us all 3 required
> properties.

Would it be an option that we use a callback function for 1 and 2 and use a
separate DOM attribute for 3?

I guess we'll end up with a very confusing API if we try to achieve all of 1, 2
and 3 using one DOM attribute/method (even though it's technically doable using
custom bindings).

[tdresser, 2015-06-17 15:55:48]

On 2015/06/17 15:49:02, haraken wrote:
> On 2015/06/17 13:20:21, tdresser wrote:
> > On 2015/06/17 02:18:18, bashi1 wrote:
> > > On 2015/06/16 12:26:14, tdresser wrote:
> > > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > > +bashi, whether the code generator supports attributes/methods
which
> > > > return
> > > > > > > callback interface.
> > > > > > 
> > > > > > Can we use a function as callback function, or a dict as an instance
> of
> > > > > callback
> > > > > > interface?
> > > > > 
> > > > > I didn't follow the whole discussion, but it seems that we support
> > > > > attributes/methods which return callback interfaces.
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > bashi, that's an example of returning a callback function, not a
callback
> > > > interface, isn't it?
> > > > 
> > > > With a callback interface, I can easily return an instance of the
> interface
> > > > constructed in C++.
> > > > Is that possible with a callback function? I thought I tried to do this
> with
> > a
> > > > callback function, but it looked like it would require some custom
> bindings.
> > I
> > > > could be remembering incorrectly though.
> > > 
> > > Ah, right. It's not a callback interface.
> > > 
> > > I read the current implementation and found that we only have basic
support
> > for
> > > callback interface. It won't work as attributes/return values.
> > > 
> > > I don't fully understand the situation, but I wonder why your proposal
> > > (providing setter for custom behavior) doesn't work. Do you have a
concrete
> > > example why it isn't work?
> > 
> > We need to be able to:
> > 1. set custom behavior
> > 2. return custom behavior
> > 3. return default behavior (defined in C++)
> > 
> > We need to be able to return custom or default behavior from the same
method.
> > 
> > Callback functions provide properties 1 and 2.
> > Callback interfaces give us 1 and 3.
> > 
> > It appears there currently isn't a solution which gives us all 3 required
> > properties.
> 
> Would it be an option that we use a callback function for 1 and 2 and use a
> separate DOM attribute for 3?
> 
> I guess we'll end up with a very confusing API if we try to achieve all of 1,
2
> and 3 using one DOM attribute/method (even though it's technically doable
using
> custom bindings).

To enable composition of components written using this api, we can't use a
separate DOM attribute for 3. Components need to be able to refer to the current
behavior of a scroller without knowing if it's current behavior is native or
custom.

I think the most sane API, and the one that is the most similar to the majority
of UI frameworks (which use inheritance for this), would involve using a single
DOM attribute (or an explicit getter / setter).

[haraken, 2015-06-17 16:09:04]

On 2015/06/17 15:55:48, tdresser wrote:
> On 2015/06/17 15:49:02, haraken wrote:
> > On 2015/06/17 13:20:21, tdresser wrote:
> > > On 2015/06/17 02:18:18, bashi1 wrote:
> > > > On 2015/06/16 12:26:14, tdresser wrote:
> > > > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > > > +bashi, whether the code generator supports attributes/methods
> which
> > > > > return
> > > > > > > > callback interface.
> > > > > > > 
> > > > > > > Can we use a function as callback function, or a dict as an
instance
> > of
> > > > > > callback
> > > > > > > interface?
> > > > > > 
> > > > > > I didn't follow the whole discussion, but it seems that we support
> > > > > > attributes/methods which return callback interfaces.
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > 
> > > > > bashi, that's an example of returning a callback function, not a
> callback
> > > > > interface, isn't it?
> > > > > 
> > > > > With a callback interface, I can easily return an instance of the
> > interface
> > > > > constructed in C++.
> > > > > Is that possible with a callback function? I thought I tried to do
this
> > with
> > > a
> > > > > callback function, but it looked like it would require some custom
> > bindings.
> > > I
> > > > > could be remembering incorrectly though.
> > > > 
> > > > Ah, right. It's not a callback interface.
> > > > 
> > > > I read the current implementation and found that we only have basic
> support
> > > for
> > > > callback interface. It won't work as attributes/return values.
> > > > 
> > > > I don't fully understand the situation, but I wonder why your proposal
> > > > (providing setter for custom behavior) doesn't work. Do you have a
> concrete
> > > > example why it isn't work?
> > > 
> > > We need to be able to:
> > > 1. set custom behavior
> > > 2. return custom behavior
> > > 3. return default behavior (defined in C++)
> > > 
> > > We need to be able to return custom or default behavior from the same
> method.
> > > 
> > > Callback functions provide properties 1 and 2.
> > > Callback interfaces give us 1 and 3.
> > > 
> > > It appears there currently isn't a solution which gives us all 3 required
> > > properties.
> > 
> > Would it be an option that we use a callback function for 1 and 2 and use a
> > separate DOM attribute for 3?
> > 
> > I guess we'll end up with a very confusing API if we try to achieve all of
1,
> 2
> > and 3 using one DOM attribute/method (even though it's technically doable
> using
> > custom bindings).
> 
> To enable composition of components written using this api, we can't use a
> separate DOM attribute for 3. Components need to be able to refer to the
current
> behavior of a scroller without knowing if it's current behavior is native or
> custom.
> 
> I think the most sane API, and the one that is the most similar to the
majority
> of UI frameworks (which use inheritance for this), would involve using a
single
> DOM attribute (or an explicit getter / setter).

Then what you need is probably to return a copy of the callback function?

var f = Element.distributScroll;
Element.setDistributionScroll(function() { ... });
var f2 = Element.distributScroll;
// At this point, f should be the default behavior and f2 should be the
customized behavior.

You can realize the behaivor by making Element.distributeScroll return a copy of
the callback function.

[tdresser, 2015-06-17 16:51:53]

On 2015/06/17 16:09:04, haraken wrote:
> On 2015/06/17 15:55:48, tdresser wrote:
> > On 2015/06/17 15:49:02, haraken wrote:
> > > On 2015/06/17 13:20:21, tdresser wrote:
> > > > On 2015/06/17 02:18:18, bashi1 wrote:
> > > > > On 2015/06/16 12:26:14, tdresser wrote:
> > > > > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > > > > +bashi, whether the code generator supports attributes/methods
> > which
> > > > > > return
> > > > > > > > > callback interface.
> > > > > > > > 
> > > > > > > > Can we use a function as callback function, or a dict as an
> instance
> > > of
> > > > > > > callback
> > > > > > > > interface?
> > > > > > > 
> > > > > > > I didn't follow the whole discussion, but it seems that we support
> > > > > > > attributes/methods which return callback interfaces.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > > 
> > > > > > bashi, that's an example of returning a callback function, not a
> > callback
> > > > > > interface, isn't it?
> > > > > > 
> > > > > > With a callback interface, I can easily return an instance of the
> > > interface
> > > > > > constructed in C++.
> > > > > > Is that possible with a callback function? I thought I tried to do
> this
> > > with
> > > > a
> > > > > > callback function, but it looked like it would require some custom
> > > bindings.
> > > > I
> > > > > > could be remembering incorrectly though.
> > > > > 
> > > > > Ah, right. It's not a callback interface.
> > > > > 
> > > > > I read the current implementation and found that we only have basic
> > support
> > > > for
> > > > > callback interface. It won't work as attributes/return values.
> > > > > 
> > > > > I don't fully understand the situation, but I wonder why your proposal
> > > > > (providing setter for custom behavior) doesn't work. Do you have a
> > concrete
> > > > > example why it isn't work?
> > > > 
> > > > We need to be able to:
> > > > 1. set custom behavior
> > > > 2. return custom behavior
> > > > 3. return default behavior (defined in C++)
> > > > 
> > > > We need to be able to return custom or default behavior from the same
> > method.
> > > > 
> > > > Callback functions provide properties 1 and 2.
> > > > Callback interfaces give us 1 and 3.
> > > > 
> > > > It appears there currently isn't a solution which gives us all 3
required
> > > > properties.
> > > 
> > > Would it be an option that we use a callback function for 1 and 2 and use
a
> > > separate DOM attribute for 3?
> > > 
> > > I guess we'll end up with a very confusing API if we try to achieve all of
> 1,
> > 2
> > > and 3 using one DOM attribute/method (even though it's technically doable
> > using
> > > custom bindings).
> > 
> > To enable composition of components written using this api, we can't use a
> > separate DOM attribute for 3. Components need to be able to refer to the
> current
> > behavior of a scroller without knowing if it's current behavior is native or
> > custom.
> > 
> > I think the most sane API, and the one that is the most similar to the
> majority
> > of UI frameworks (which use inheritance for this), would involve using a
> single
> > DOM attribute (or an explicit getter / setter).
> 
> Then what you need is probably to return a copy of the callback function?
> 
> var f = Element.distributScroll;
> Element.setDistributionScroll(function() { ... });
> var f2 = Element.distributScroll;
> // At this point, f should be the default behavior and f2 should be the
> customized behavior.
> 
> You can realize the behaivor by making Element.distributeScroll return a copy
of
> the callback function.

You need custom bindings to convert a native method into a callback function,
don't you?

[haraken, 2015-06-17 17:53:53]

On 2015/06/17 16:51:53, tdresser wrote:
> On 2015/06/17 16:09:04, haraken wrote:
> > On 2015/06/17 15:55:48, tdresser wrote:
> > > On 2015/06/17 15:49:02, haraken wrote:
> > > > On 2015/06/17 13:20:21, tdresser wrote:
> > > > > On 2015/06/17 02:18:18, bashi1 wrote:
> > > > > > On 2015/06/16 12:26:14, tdresser wrote:
> > > > > > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > > > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > > > > > +bashi, whether the code generator supports
attributes/methods
> > > which
> > > > > > > return
> > > > > > > > > > callback interface.
> > > > > > > > > 
> > > > > > > > > Can we use a function as callback function, or a dict as an
> > instance
> > > > of
> > > > > > > > callback
> > > > > > > > > interface?
> > > > > > > > 
> > > > > > > > I didn't follow the whole discussion, but it seems that we
support
> > > > > > > > attributes/methods which return callback interfaces.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > > > 
> > > > > > > bashi, that's an example of returning a callback function, not a
> > > callback
> > > > > > > interface, isn't it?
> > > > > > > 
> > > > > > > With a callback interface, I can easily return an instance of the
> > > > interface
> > > > > > > constructed in C++.
> > > > > > > Is that possible with a callback function? I thought I tried to do
> > this
> > > > with
> > > > > a
> > > > > > > callback function, but it looked like it would require some custom
> > > > bindings.
> > > > > I
> > > > > > > could be remembering incorrectly though.
> > > > > > 
> > > > > > Ah, right. It's not a callback interface.
> > > > > > 
> > > > > > I read the current implementation and found that we only have basic
> > > support
> > > > > for
> > > > > > callback interface. It won't work as attributes/return values.
> > > > > > 
> > > > > > I don't fully understand the situation, but I wonder why your
proposal
> > > > > > (providing setter for custom behavior) doesn't work. Do you have a
> > > concrete
> > > > > > example why it isn't work?
> > > > > 
> > > > > We need to be able to:
> > > > > 1. set custom behavior
> > > > > 2. return custom behavior
> > > > > 3. return default behavior (defined in C++)
> > > > > 
> > > > > We need to be able to return custom or default behavior from the same
> > > method.
> > > > > 
> > > > > Callback functions provide properties 1 and 2.
> > > > > Callback interfaces give us 1 and 3.
> > > > > 
> > > > > It appears there currently isn't a solution which gives us all 3
> required
> > > > > properties.
> > > > 
> > > > Would it be an option that we use a callback function for 1 and 2 and
use
> a
> > > > separate DOM attribute for 3?
> > > > 
> > > > I guess we'll end up with a very confusing API if we try to achieve all
of
> > 1,
> > > 2
> > > > and 3 using one DOM attribute/method (even though it's technically
doable
> > > using
> > > > custom bindings).
> > > 
> > > To enable composition of components written using this api, we can't use a
> > > separate DOM attribute for 3. Components need to be able to refer to the
> > current
> > > behavior of a scroller without knowing if it's current behavior is native
or
> > > custom.
> > > 
> > > I think the most sane API, and the one that is the most similar to the
> > majority
> > > of UI frameworks (which use inheritance for this), would involve using a
> > single
> > > DOM attribute (or an explicit getter / setter).
> > 
> > Then what you need is probably to return a copy of the callback function?
> > 
> > var f = Element.distributScroll;
> > Element.setDistributionScroll(function() { ... });
> > var f2 = Element.distributScroll;
> > // At this point, f should be the default behavior and f2 should be the
> > customized behavior.
> > 
> > You can realize the behaivor by making Element.distributeScroll return a
copy
> of
> > the callback function.
> 
> You need custom bindings to convert a native method into a callback function,
> don't you?

Then what happens if you expose:

  Element.defaultDistributionScroll;
  Element.distributionScroll;
  Element.setDistributionScroll();

? The Element.distributionScroll is set to the defaultDistributionScroll by
default. When setDistributionScroll is called, it overrides the
Element.distributionScroll. If a developer wants to keep using the default
behavoir, he can use Element.defaultDistributionScroll.

[tdresser, 2015-06-18 13:34:41]

On 2015/06/17 17:53:53, haraken wrote:
> On 2015/06/17 16:51:53, tdresser wrote:
> > On 2015/06/17 16:09:04, haraken wrote:
> > > On 2015/06/17 15:55:48, tdresser wrote:
> > > > On 2015/06/17 15:49:02, haraken wrote:
> > > > > On 2015/06/17 13:20:21, tdresser wrote:
> > > > > > On 2015/06/17 02:18:18, bashi1 wrote:
> > > > > > > On 2015/06/16 12:26:14, tdresser wrote:
> > > > > > > > On 2015/06/16 03:12:04, bashi1 wrote:
> > > > > > > > > On 2015/06/15 09:27:10, Yuki wrote:
> > > > > > > > > > On 2015/06/15 05:51:09, Yuki wrote:
> > > > > > > > > > > +bashi, whether the code generator supports
> attributes/methods
> > > > which
> > > > > > > > return
> > > > > > > > > > > callback interface.
> > > > > > > > > > 
> > > > > > > > > > Can we use a function as callback function, or a dict as an
> > > instance
> > > > > of
> > > > > > > > > callback
> > > > > > > > > > interface?
> > > > > > > > > 
> > > > > > > > > I didn't follow the whole discussion, but it seems that we
> support
> > > > > > > > > attributes/methods which return callback interfaces.
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > > > > 
> > > > > > > > bashi, that's an example of returning a callback function, not a
> > > > callback
> > > > > > > > interface, isn't it?
> > > > > > > > 
> > > > > > > > With a callback interface, I can easily return an instance of
the
> > > > > interface
> > > > > > > > constructed in C++.
> > > > > > > > Is that possible with a callback function? I thought I tried to
do
> > > this
> > > > > with
> > > > > > a
> > > > > > > > callback function, but it looked like it would require some
custom
> > > > > bindings.
> > > > > > I
> > > > > > > > could be remembering incorrectly though.
> > > > > > > 
> > > > > > > Ah, right. It's not a callback interface.
> > > > > > > 
> > > > > > > I read the current implementation and found that we only have
basic
> > > > support
> > > > > > for
> > > > > > > callback interface. It won't work as attributes/return values.
> > > > > > > 
> > > > > > > I don't fully understand the situation, but I wonder why your
> proposal
> > > > > > > (providing setter for custom behavior) doesn't work. Do you have a
> > > > concrete
> > > > > > > example why it isn't work?
> > > > > > 
> > > > > > We need to be able to:
> > > > > > 1. set custom behavior
> > > > > > 2. return custom behavior
> > > > > > 3. return default behavior (defined in C++)
> > > > > > 
> > > > > > We need to be able to return custom or default behavior from the
same
> > > > method.
> > > > > > 
> > > > > > Callback functions provide properties 1 and 2.
> > > > > > Callback interfaces give us 1 and 3.
> > > > > > 
> > > > > > It appears there currently isn't a solution which gives us all 3
> > required
> > > > > > properties.
> > > > > 
> > > > > Would it be an option that we use a callback function for 1 and 2 and
> use
> > a
> > > > > separate DOM attribute for 3?
> > > > > 
> > > > > I guess we'll end up with a very confusing API if we try to achieve
all
> of
> > > 1,
> > > > 2
> > > > > and 3 using one DOM attribute/method (even though it's technically
> doable
> > > > using
> > > > > custom bindings).
> > > > 
> > > > To enable composition of components written using this api, we can't use
a
> > > > separate DOM attribute for 3. Components need to be able to refer to the
> > > current
> > > > behavior of a scroller without knowing if it's current behavior is
native
> or
> > > > custom.
> > > > 
> > > > I think the most sane API, and the one that is the most similar to the
> > > majority
> > > > of UI frameworks (which use inheritance for this), would involve using a
> > > single
> > > > DOM attribute (or an explicit getter / setter).
> > > 
> > > Then what you need is probably to return a copy of the callback function?
> > > 
> > > var f = Element.distributScroll;
> > > Element.setDistributionScroll(function() { ... });
> > > var f2 = Element.distributScroll;
> > > // At this point, f should be the default behavior and f2 should be the
> > > customized behavior.
> > > 
> > > You can realize the behaivor by making Element.distributeScroll return a
> copy
> > of
> > > the callback function.
> > 
> > You need custom bindings to convert a native method into a callback
function,
> > don't you?
> 
> Then what happens if you expose:
> 
>   Element.defaultDistributionScroll;
>   Element.distributionScroll;
>   Element.setDistributionScroll();
> 
> ? The Element.distributionScroll is set to the defaultDistributionScroll by
> default. When setDistributionScroll is called, it overrides the
> Element.distributionScroll. If a developer wants to keep using the default
> behavoir, he can use Element.defaultDistributionScroll.

That would be reasonable, if it's possible.
How do I set the value of "distributeScroll" to "defaultDistributeScroll" in
C++?
I could write some custom bindings logic to look up the value of
"defaultDistributeScroll", and then set the value of distributeScroll to it, but
that's a similar amount of custom bindings to my previous approaches.

[haraken, 2015-06-18 18:51:06]

> > Then what happens if you expose:
> > 
> >   Element.defaultDistributionScroll;
> >   Element.distributionScroll;
> >   Element.setDistributionScroll();
> > 
> > ? The Element.distributionScroll is set to the defaultDistributionScroll by
> > default. When setDistributionScroll is called, it overrides the
> > Element.distributionScroll. If a developer wants to keep using the default
> > behavoir, he can use Element.defaultDistributionScroll.
> 
> That would be reasonable, if it's possible.
> How do I set the value of "distributeScroll" to "defaultDistributeScroll" in
> C++?

Why do you need to set the value of distributeScroll to
defaultDistributionScroll? I'm guessing that:

- Element.defaultDistributionScroll is mapped to a callback interface that does
the default behavior forever. It never changes.
- Element.distributionScroll returns Element.defaultDistributionScroll if there
is no customized callback interface installed. Otherwise, it returns the
customized callback interface.
- Element.setDistributionScroll installs the customized callback interface.

> I could write some custom bindings logic to look up the value of
> "defaultDistributeScroll", and then set the value of distributeScroll to it,
but
> that's a similar amount of custom bindings to my previous approaches.

I'm not saying that you must remove all custom bindings. What we should avoid is
to use a non-standard way in the custom bindings that has a risk of cross-world
wrapper leaks etc.

[tdresser, 2015-06-18 18:57:28]

> I'm not saying that you must remove all custom bindings. What we should avoid
is
> to use a non-standard way in the custom bindings that has a risk of
cross-world
> wrapper leaks etc.

For sure, I'm just trying to minimize the use of custom bindings. If we can get
away without using any, that would be ideal.

> - Element.distributionScroll returns Element.defaultDistributionScroll if
there
> is no customized callback interface installed. Otherwise, it returns the
> customized callback interface.

This is the tricky bit. How do I define Element.distributeScroll such that it
returns Element.defaultDistributeScroll if there is no customized callback
interface installed?

[haraken, 2015-06-18 19:08:27]

On 2015/06/18 18:57:28, tdresser wrote:
> > I'm not saying that you must remove all custom bindings. What we should
avoid
> is
> > to use a non-standard way in the custom bindings that has a risk of
> cross-world
> > wrapper leaks etc.
> 
> For sure, I'm just trying to minimize the use of custom bindings. If we can
get
> away without using any, that would be ideal.
> 
> > - Element.distributionScroll returns Element.defaultDistributionScroll if
> there
> > is no customized callback interface installed. Otherwise, it returns the
> > customized callback interface.
> 
> This is the tricky bit. How do I define Element.distributeScroll such that it
> returns Element.defaultDistributeScroll if there is no customized callback
> interface installed?

This would need some custom bindings.

Assuming that you don't want to share the customized callback interface among
worlds (i.e., if the main world customized the scroll callback, it doesn't
affect other isolated worlds), one way to do this would be to use a hidden value
of V8 objects.

setDistributionScroll can set the customized callback interface on the hidden
value of the element wrapper. distributionScroll returns the hidden value if the
hidden value exists. Otherwise, distributionScroll returns the default callback
interface.

[tdresser, 2015-06-18 19:26:25]

On 2015/06/18 19:08:27, haraken wrote:
> On 2015/06/18 18:57:28, tdresser wrote:
> > > I'm not saying that you must remove all custom bindings. What we should
> avoid
> > is
> > > to use a non-standard way in the custom bindings that has a risk of
> > cross-world
> > > wrapper leaks etc.
> > 
> > For sure, I'm just trying to minimize the use of custom bindings. If we can
> get
> > away without using any, that would be ideal.
> > 
> > > - Element.distributionScroll returns Element.defaultDistributionScroll if
> > there
> > > is no customized callback interface installed. Otherwise, it returns the
> > > customized callback interface.
> > 
> > This is the tricky bit. How do I define Element.distributeScroll such that
it
> > returns Element.defaultDistributeScroll if there is no customized callback
> > interface installed?
> 
> This would need some custom bindings.
> 
> Assuming that you don't want to share the customized callback interface among
> worlds (i.e., if the main world customized the scroll callback, it doesn't
> affect other isolated worlds), one way to do this would be to use a hidden
value
> of V8 objects.
> 
> setDistributionScroll can set the customized callback interface on the hidden
> value of the element wrapper. distributionScroll returns the hidden value if
the
> hidden value exists. Otherwise, distributionScroll returns the default
callback
> interface.

Perfect, thanks.
I'll give this a shot.

[tdresser, 2015-06-25 20:33:40]

Patchset #11 (id:200001) has been deleted

[tdresser, 2015-06-25 20:33:50]

Patchset #11 (id:220001) has been deleted

[tdresser, 2015-06-26 14:53:15]

I think this approach minimizes the amount of custom bindings logic. I think it
negatively impacts the API shape, but this API shape isn't final anyways, so
that's fine for now.

All the custom bindings logic is isolated in ScrollStateCallback.

I'm having a hard time getting the memory management correct in the non-Oilpan
world though. The ScrollStateCallback objects can be pointed to by any subset of
JavaScript and C++. There's no clear ownership, and I'm not sure if I can get JS
to interact correctly with the existing refcount.

Any thoughts? If there's a better way to pass the target Element to the
handleEvent method, let me know!

Thanks.

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:491: ScriptState* scriptState =
ScriptState::forMainWorld(frame);
On 2015/06/09 15:16:38, haraken wrote:
> On 2015/06/09 14:27:42, tdresser wrote:
> > On 2015/06/09 00:07:13, haraken wrote:
> > > 
> > > Why can we assume the main world?
> > 
> > We're okay if scroll customization methods can only be assigned from the
main
> > world.
> 
> That is not OK. Unless you have a strong reason to do so, we should support
the
> Web API in isolated worlds as well.
> 
> > My understanding is that currently, if you assign a scroll customization
> method
> > from a world other than the main world, this code will pay no attention to
it.
> > Is that correct?
> 
> If an isolated world assigns a scroll customization method, it will install
the
> method on the main world -- which is not correct.
> 
> Assuming that callScrollCustomizationV8Method is called from only JavaScript,
I
> think you can use:
> 
>   ScriptState* scriptState = ScriptState::current(toIsolate(frame));
> 
> which will give you the ScriptState of the JS that called the
> callScrollCustomizationV8Method.
> 
> (However, I'd strongly recommend using a callback interface because that's a
> standard way to handle this kind of situation in a safe manner.)

callScrollCustomizationV8Method is never called from JavaScript.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.cpp (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:87:
data->Set(v8::String::NewFromUtf8(isolate, kHandlerFunction),
v8::External::New(isolate, this));
I'm fairly sure this is wrong. The v8::External doesn't ref the
ScrollStateCallback, which allows it to be deleted while JS still has a
reference to it. I'm not sure what the correct fix is here though.

[haraken, 2015-06-27 08:09:33]

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.cpp:42: void* pointers[15];

It's unfortunate that we increase sizeof(ElementRareData) for runtime enabled
features that are currently disabled.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.h:171: RefPtrWillBeMember<ScrollStateCallback>
m_distributeScroll;

You can make Oilpan the default for ScrollStateCallback and use
PersistentWillBeMember.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.cpp (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:6: #include
"core/page/scrolling/ScrollStateCallback.h"

This file uses V8 APIs. So this should go to
Source/bindings/core/v8/ScrollStateCallback.h

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:15: static const char
applyScrollRequiresScrollStateObject[]

Nit: Don't need the identation.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:31:
data->Get(v8::String::NewFromUtf8(info.GetIsolate(), kHandlerFunction));

Nit: You don't need to wrap lines at 80 characters. 120 characters or so are OK
in Blink. The same comment for other parts in this CL.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:33:
maybeScrollStateCallback.ToLocalChecked();

  if (!data->Get(...).ToLocal(&localScrollStateCallback))
    return;

I don't like the complexity around Maybe V8 APIs, but we cannot avoid this...

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:87:
data->Set(v8::String::NewFromUtf8(isolate, kHandlerFunction),
v8::External::New(isolate, this));
On 2015/06/26 14:53:15, tdresser wrote:
> I'm fairly sure this is wrong. The v8::External doesn't ref the
> ScrollStateCallback, which allows it to be deleted while JS still has a
> reference to it. I'm not sure what the correct fix is here though.

I don't understand why you need the complexity about creating a function
template etc.

For example, in case of Document.requestAnimationFrame, we just need to
implement FrameRequestCallback, which is fairly straightforward and simple. Why
do we need all the complexity for ScrollStateCallback?

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.h (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.h:15: class ScrollStateCallback :
public RefCountedWillBeGarbageCollectedFinalized<ScrollStateCallback> {

I think we can just make Oilpan the default. WillBe is not needed. I think it
will simplify this CL.

  class ScrollStateCallback : public GarbageCollectedFinalized<class
ScrollStateCallback : public RefCountedWillBeGarbageCollectedFinalized> { ... };

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.h:37: Element* m_targetElement;

This is a strong reference, so shall we use Member<Element>?

[tdresser, 2015-06-29 20:59:33]

PTAL, thanks!

I'm still working out some issues with flaky tests when oilpan is disabled.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.cpp:42: void* pointers[15];
On 2015/06/27 08:09:32, haraken wrote:
> 
> It's unfortunate that we increase sizeof(ElementRareData) for runtime enabled
> features that are currently disabled.
> 

Can you suggest an alternative? You previously mentioned the option of using a
v8 hidden value, but that would require a bunch of logic for interacting with
the v8 types.

We'd need to:
- When receiving the ScrollStateCallback, convert it into a v8::Value.
- When calling the ScrollStateCallback, convert it from a v8::Value.
- Fetch the v8::Object for the current element whenever we get or set a scroll
customization method.

I can give this a shot, but it feels a little bit heavier than would be ideal.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.h:171: RefPtrWillBeMember<ScrollStateCallback>
m_distributeScroll;
On 2015/06/27 08:09:33, haraken wrote:
> 
> You can make Oilpan the default for ScrollStateCallback and use
> PersistentWillBeMember.

Thanks, I'd tried that earlier, but had been missing the behavior of Persistent.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.cpp (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:6: #include
"core/page/scrolling/ScrollStateCallback.h"
On 2015/06/27 08:09:33, haraken wrote:
> 
> This file uses V8 APIs. So this should go to
> Source/bindings/core/v8/ScrollStateCallback.h
> 

It looks like this is a bit tricky, as the idl file can't be moved to that
location. I've factored out the V8 API bits into
ScrollStateCallbackToScriptValue.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:15: static const char
applyScrollRequiresScrollStateObject[]
On 2015/06/27 08:09:33, haraken wrote:
> 
> Nit: Don't need the identation.

Done.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:31:
data->Get(v8::String::NewFromUtf8(info.GetIsolate(), kHandlerFunction));
On 2015/06/27 08:09:33, haraken wrote:
> 
> Nit: You don't need to wrap lines at 80 characters. 120 characters or so are
OK
> in Blink. The same comment for other parts in this CL.
> 
> 

Ran git cl format.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:33:
maybeScrollStateCallback.ToLocalChecked();
On 2015/06/27 08:09:33, haraken wrote:
> 
>   if (!data->Get(...).ToLocal(&localScrollStateCallback))
>     return;
> 
> I don't like the complexity around Maybe V8 APIs, but we cannot avoid this...

Done. (I'm still using a temporary, as it looks like I'd need an explicit cast
to MaybeLocal to avoid getting the v8::Local version otherwise.)

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:87:
data->Set(v8::String::NewFromUtf8(isolate, kHandlerFunction),
v8::External::New(isolate, this));
On 2015/06/27 08:09:33, haraken wrote:
> On 2015/06/26 14:53:15, tdresser wrote:
> > I'm fairly sure this is wrong. The v8::External doesn't ref the
> > ScrollStateCallback, which allows it to be deleted while JS still has a
> > reference to it. I'm not sure what the correct fix is here though.
> 
> I don't understand why you need the complexity about creating a function
> template etc.
> 
> For example, in case of Document.requestAnimationFrame, we just need to
> implement FrameRequestCallback, which is fairly straightforward and simple.
Why
> do we need all the complexity for ScrollStateCallback?

I've written up a quick doc summarizing the requirements we're meeting with this
patch here:
https://docs.google.com/document/d/1Uu7ZudjaGk1zUVMGBtkZs1pslSU3iUHcyPlPv-cX6...

I've addressed this in the Requirements section:
https://docs.google.com/document/d/1Uu7ZudjaGk1zUVMGBtkZs1pslSU3iUHcyPlPv-cX6...

In short, the reason we need all this complexity is that we need to be able to
pass a ScrollStateCallback back out to JavaScript, which isn't a requirement for
requestAnimationFrame.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.h (right):

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.h:15: class ScrollStateCallback :
public RefCountedWillBeGarbageCollectedFinalized<ScrollStateCallback> {
On 2015/06/27 08:09:33, haraken wrote:
> 
> I think we can just make Oilpan the default. WillBe is not needed. I think it
> will simplify this CL.
> 
>   class ScrollStateCallback : public GarbageCollectedFinalized<class
> ScrollStateCallback : public RefCountedWillBeGarbageCollectedFinalized> { ...
};

Thanks, that definitely simplifies things.

https://codereview.chromium.org/1057603002/diff/240001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.h:37: Element* m_targetElement;
On 2015/06/27 08:09:33, haraken wrote:
> 
> This is a strong reference, so shall we use Member<Element>?

It can't be a Member<Element> in the non-oilpan case, so I'm using
RefPtrWillBeMember here.

Personally I think this is clearer as a raw ptr - this is only non-null within a
very limited scope.

[haraken, 2015-06-30 07:08:31]

I'm getting a bit tired of reviewing this CL... Would it be really worth working
on the complexity for such an at-the-moment API (which is just for developers
that want to try the API at the moment)? I think we should first work on
polishing up the API before digging into the binding complexity.

The complexity in ScrollStateCallbackToScriptValue.cpp is not acceptable. I
guess what we need would be:

1) Make V8ScriptStateCallback hold a v8::Function. See V8RequestAnimationFrame.
The V8ScriptStateCallback should be auto-generated by ScriptStateCallback.idl.

  class V8ScriptStateCallback {
    ScopedPersistent<v8::Function> m_callback;
  };

2) Support toV8() functions in callback interfaces. Specifically,
toV8(ScriptStateCallback*) just needs to return m_callback. Then the binding
layer can just call toV8(distributeScroll()) to get a wrapper of the callback
interface. (The wrapper is a v8::Function.)

https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
File Source/bindings/core/v8/ScrollStateCallbackToScriptValue.cpp (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
Source/bindings/core/v8/ScrollStateCallbackToScriptValue.cpp:32: if
(!maybeLocalScrollStateCallback.ToLocal(&localScrollStateCallback))

  if (data->Get(...).ToLocal())

Nit: We want to avoid writing MaybeLocal in the binding layer as much as
possible.

https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
File Source/bindings/core/v8/ScrollStateCallbackToScriptValue.h (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
Source/bindings/core/v8/ScrollStateCallbackToScriptValue.h:16: ScriptValue
scrollStateCallbackToScriptValue(ScrollStateCallback&, ScriptState*);

This should go to ToV8.h. (See my comment in Element.h.)

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:477: class DistributeScrollCallback : public
ScrollStateCallback {

We should define this in ScrollStateCallback.h (or a dedicated file).

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:488: class ApplyScrollCallback : public
ScrollStateCallback {

Ditto.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:510: setDistributeScroll(new
DistributeScrollCallback());

In a case where we don't have a customized calback, we want to avoid storing an
empty callback to ElementRareData because it will create an ElementRareData.

Can we just return a native callback without storing anything in
ElementRareData?

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:523: setApplyScroll(new ApplyScrollCallback());

Ditto.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.h:373: ScriptValue distributeScroll(ScriptState*);

Would it be possible to:

- make distributeScroll return ScriptStateCallback*
- let the binding layer call toV8(distributeScroll()), which converts the
ScriptStateCallback* to a V8 wrapper

This is a common way to convert a DOM object to a V8 wrapper. You can define the
toV8 method in ToV8.h.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/Element.h:376: ScriptValue applyScroll(ScriptState*);

Ditto.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.cpp (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.cpp:43: PersistentWillBeMember<Element>
persistentWillBeMember[2];

This looks like an unacceptable size increase. Let's revisit this issue once we
resolve other concerns in this CL.

FWIW, I'm tring to reduce sizeof(Persistent) here:
https://codereview.chromium.org/1213133002/

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.h:127: ScrollStateCallback*
getDistributeScroll() { return m_distributeScroll.get(); }

Nit: Blink normally doesn't use a "get" prefix.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
Source/core/dom/ElementRareData.h:131: ScrollStateCallback* getApplyScroll() {
return m_applyScroll.get(); }

Ditto.

https://codereview.chromium.org/1057603002/diff/260001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollStateCallback.cpp (right):

https://codereview.chromium.org/1057603002/diff/260001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollStateCallback.cpp:20: m_targetElement =
&targetElement;

Instead of storing m_targetElement to ScrollStateCallback, how about just
passing the target element to handleEvent()?

[tdresser, 2015-07-06 20:42:32]

On 2015/06/30 07:08:31, haraken wrote:
> I'm getting a bit tired of reviewing this CL... Would it be really worth
working
> on the complexity for such an at-the-moment API (which is just for developers
> that want to try the API at the moment)? I think we should first work on
> polishing up the API before digging into the binding complexity.
> 
> The complexity in ScrollStateCallbackToScriptValue.cpp is not acceptable. I
> guess what we need would be:
> 
> 1) Make V8ScriptStateCallback hold a v8::Function. See
V8RequestAnimationFrame.
> The V8ScriptStateCallback should be auto-generated by ScriptStateCallback.idl.
> 
>   class V8ScriptStateCallback {
>     ScopedPersistent<v8::Function> m_callback;
>   };
> 
> 2) Support toV8() functions in callback interfaces. Specifically,
> toV8(ScriptStateCallback*) just needs to return m_callback. Then the binding
> layer can just call toV8(distributeScroll()) to get a wrapper of the callback
> interface. (The wrapper is a v8::Function.)
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
> File Source/bindings/core/v8/ScrollStateCallbackToScriptValue.cpp (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
> Source/bindings/core/v8/ScrollStateCallbackToScriptValue.cpp:32: if
> (!maybeLocalScrollStateCallback.ToLocal(&localScrollStateCallback))
> 
>   if (data->Get(...).ToLocal())
> 
> Nit: We want to avoid writing MaybeLocal in the binding layer as much as
> possible.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
> File Source/bindings/core/v8/ScrollStateCallbackToScriptValue.h (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/bindings/core/v...
> Source/bindings/core/v8/ScrollStateCallbackToScriptValue.h:16: ScriptValue
> scrollStateCallbackToScriptValue(ScrollStateCallback&, ScriptState*);
> 
> This should go to ToV8.h. (See my comment in Element.h.)
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> File Source/core/dom/Element.cpp (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.cpp:477: class DistributeScrollCallback : public
> ScrollStateCallback {
> 
> We should define this in ScrollStateCallback.h (or a dedicated file).
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.cpp:488: class ApplyScrollCallback : public
> ScrollStateCallback {
> 
> Ditto.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.cpp:510: setDistributeScroll(new
> DistributeScrollCallback());
> 
> In a case where we don't have a customized calback, we want to avoid storing
an
> empty callback to ElementRareData because it will create an ElementRareData.
> 
> Can we just return a native callback without storing anything in
> ElementRareData?
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.cpp:523: setApplyScroll(new ApplyScrollCallback());
> 
> Ditto.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Element.h
> File Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.h:373: ScriptValue distributeScroll(ScriptState*);
> 
> Would it be possible to:
> 
> - make distributeScroll return ScriptStateCallback*
> - let the binding layer call toV8(distributeScroll()), which converts the
> ScriptStateCallback* to a V8 wrapper
> 
> This is a common way to convert a DOM object to a V8 wrapper. You can define
the
> toV8 method in ToV8.h.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/Element.h:376: ScriptValue applyScroll(ScriptState*);
> 
> Ditto.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> File Source/core/dom/ElementRareData.cpp (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/ElementRareData.cpp:43: PersistentWillBeMember<Element>
> persistentWillBeMember[2];
> 
> This looks like an unacceptable size increase. Let's revisit this issue once
we
> resolve other concerns in this CL.
> 
> FWIW, I'm tring to reduce sizeof(Persistent) here:
> https://codereview.chromium.org/1213133002/
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> File Source/core/dom/ElementRareData.h (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/ElementRareData.h:127: ScrollStateCallback*
> getDistributeScroll() { return m_distributeScroll.get(); }
> 
> Nit: Blink normally doesn't use a "get" prefix.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/dom/Elemen...
> Source/core/dom/ElementRareData.h:131: ScrollStateCallback* getApplyScroll() {
> return m_applyScroll.get(); }
> 
> Ditto.
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/page/scrol...
> File Source/core/page/scrolling/ScrollStateCallback.cpp (right):
> 
>
https://codereview.chromium.org/1057603002/diff/260001/Source/core/page/scrol...
> Source/core/page/scrolling/ScrollStateCallback.cpp:20: m_targetElement =
> &targetElement;
> 
> Instead of storing m_targetElement to ScrollStateCallback, how about just
> passing the target element to handleEvent()?

Yeah, you're right, we should be taking a less invasive approach.
I've got a new approach up here: https://codereview.chromium.org/1209183004/,
which doesn't require any custom bindings, but is significantly less flexible.

It should be adequate for now.
We still need to find a better place than ElementRareData to stash the
ScrollStateCallback objects, but that shouldn't be too bad.

Thanks,
Tim