Performance.onwebkitresourcetimingbufferfull should be a Function

Performance.onwebkitresourcetimingbufferfull should be a Function

Make Performance.onwebkitresourcetimingbufferfull a Function instead of
an EventListener to match the specification:
http://www.w3c-test.org/webperf/specs/ResourceTiming/#extensions-performance-interface

This means that there is no longer any event passed as parameter to the
callback and that we can drop the [EventTarget] IDL extended attribute
for the Performance interface.

This patch also adds support for Function attributes to the bindings
generator as it was missing.

BUG=258452
BUG=257583

[arv (Not doing code reviews), 2013-07-09 18:19:58]

https://codereview.chromium.org/18916003/diff/1/LayoutTests/http/tests/misc/w...
File LayoutTests/http/tests/misc/webtiming-buffer-full-no-event.html (right):

https://codereview.chromium.org/18916003/diff/1/LayoutTests/http/tests/misc/w...
LayoutTests/http/tests/misc/webtiming-buffer-full-no-event.html:14:
bufferFullCount++;
Can you also assert that this === window.performance?

var self;

function onBufferFull(event) {
  ...
  self = this;
  shouldBe("self", "performance");
  bufferFullCount++;
}

https://codereview.chromium.org/18916003/diff/1/Source/core/page/Performance.cpp
File Source/core/page/Performance.cpp (right):

https://codereview.chromium.org/18916003/diff/1/Source/core/page/Performance....
Source/core/page/Performance.cpp:174: if (!callback.isFunction())
This is a bit inconsistent.

WebIDL says throw a TypeError if it is not a Function (which of course will not
work because we need to support null at least).

EventListeners sets it to null if not a function so maybe we should do that if
we don't want to throw.

[adamk, 2013-07-09 18:26:44]

This approach will allow leaking objects between Isolated Worlds (that is,
between page script and extensions). That separation is something the bindings
layer now relies upon for correctness. To avoid this, you'll at least need to
store a callback per-DOMWrapperWorld (see the approach I took for attribute
event listeners in
https://src.chromium.org/viewvc/blink?revision=152377&view=revision).

There's another leak here, though, which is a memory leak. Since Performance
holds a strong reference (via its ScriptValue) to the assigned callback
function, it's possible to end up with a reference cycle that'll keep DOM stuff
alive for too long. To avoid that, I'd recommend separating the callback logic
into its own class, like other callbacks do. MutationCallback/V8MutationCallback
is the right model here, as it correctly avoids memory leaks by using weak
references (and perhaps we can generalize the approach taken there and generate
the code, somehow).

[arv (Not doing code reviews), 2013-07-09 18:36:25]

The memory leak is not an issue in this case [*] but I do agree that it
would be better to solve this the right way so that the next person who
comes along will not just copy paste this and get a significant leak.

[*] The reason why it is not an issue is that window.performance is never
collected (it is cached on the DOMWindow).


On Tue, Jul 9, 2013 at 2:26 PM, <adamk@chromium.org> wrote:

> This approach will allow leaking objects between Isolated Worlds (that is,
> between page script and extensions). That separation is something the
> bindings
> layer now relies upon for correctness. To avoid this, you'll at least need
> to
> store a callback per-DOMWrapperWorld (see the approach I took for attribute
> event listeners in
>
https://src.chromium.org/**viewvc/blink?revision=152377&**view=revision<https...
> ).
>
> There's another leak here, though, which is a memory leak. Since
> Performance
> holds a strong reference (via its ScriptValue) to the assigned callback
> function, it's possible to end up with a reference cycle that'll keep DOM
> stuff
> alive for too long. To avoid that, I'd recommend separating the callback
> logic
> into its own class, like other callbacks do. MutationCallback/**
> V8MutationCallback
> is the right model here, as it correctly avoids memory leaks by using weak
> references (and perhaps we can generalize the approach taken there and
> generate
> the code, somehow).
>
>
https://codereview.chromium.**org/18916003/<https://codereview.chromium.org/1...
>



-- 
erik

[abarth-chromium, 2013-07-09 18:37:19]

Why does the spec use a Function here?  That seems wrong.  We should change the
spec to work like every other DOM object.

[abarth-chromium, 2013-07-09 18:38:44]

On 2013/07/09 18:36:25, arv wrote:
> The memory leak is not an issue in this case [*] but I do agree that it
> would be better to solve this the right way so that the next person who
> comes along will not just copy paste this and get a significant leak.
> 
> [*] The reason why it is not an issue is that window.performance is never
> collected (it is cached on the DOMWindow).

That's not accurate.  The DOMWindow will never be collected, which means we'll
leak the entire Document, even after navigating to another Document.

[do-not-use, 2013-07-10 09:47:28]

On 2013/07/09 18:37:19, abarth wrote:
> Why does the spec use a Function here?  That seems wrong.  We should change
the
> spec to work like every other DOM object.

I have asked the question on the W3C mailing list:
http://lists.w3.org/Archives/Public/public-web-perf/2013Jul/0004.html

Let's wait for a response and decide what to do accordingly.

[do-not-use, 2013-07-26 06:57:54]

On 2013/07/10 09:47:28, Christophe Dumez wrote:
> On 2013/07/09 18:37:19, abarth wrote:
> > Why does the spec use a Function here?  That seems wrong.  We should change
> the
> > spec to work like every other DOM object.
> 
> I have asked the question on the W3C mailing list:
> http://lists.w3.org/Archives/Public/public-web-perf/2013Jul/0004.html
> 
> Let's wait for a response and decide what to do accordingly.

Looks like the specification is going to be updated to use an EventHandler
instead of a Function:
http://lists.w3.org/Archives/Public/public-web-perf/2013Jul/0027.html

I am therefore abandoning this CL. We may need another CL later on to have
Performance inherit EventTarget if the specification does so.

[adamk, 2013-07-26 16:24:14]

On 2013/07/26 06:57:54, Christophe Dumez wrote:
> On 2013/07/10 09:47:28, Christophe Dumez wrote:
> > On 2013/07/09 18:37:19, abarth wrote:
> > > Why does the spec use a Function here?  That seems wrong.  We should
change
> > the
> > > spec to work like every other DOM object.
> > 
> > I have asked the question on the W3C mailing list:
> > http://lists.w3.org/Archives/Public/public-web-perf/2013Jul/0004.html
> > 
> > Let's wait for a response and decide what to do accordingly.
> 
> Looks like the specification is going to be updated to use an EventHandler
> instead of a Function:
> http://lists.w3.org/Archives/Public/public-web-perf/2013Jul/0027.html
> 
> I am therefore abandoning this CL. We may need another CL later on to have
> Performance inherit EventTarget if the specification does so.

Thanks for doing the legwork on this one, an EventHandler seems better to me
(especially because it avoids having to re-implement that again...).