Bug 40080 – Web Inspector: better Function.prototype.bind for the internal code

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Nikita Vasilyev]

Created attachment 57687 [details]
function.bind() causing useless listenerBody message

While digging Web Inspector I noticed how useless sometimes "Event Listeners" pane is. The problem is in the "binded" functions. All binded functions looks like: function () { return func.apply(thisObject, args.concat(Array.prototype.slice.call(arguments, 0))); }

[Nikita Vasilyev]

Created attachment 57688 [details]
Show handler instead of Function.prototype.bind

[Nikita Vasilyev]

Created attachment 57689 [details]
Show handler instead of Function.prototype.bind

[Nikita Vasilyev]

Created attachment 57694 [details]
Show a handler function instead of Function.prototype.bind

binded --> bound

[Joseph Pecoraro]

Nikita, great idea!

We should keep this in mind for if/when we end up using a native Function.prototype.bind. (@see bug 26382)

[Nikita Vasilyev]

Created attachment 57700 [details]
Show a handler function instead of Function.prototype.bind

s/binded/bound/ in the ChangeLog.

[Yury Semikhatsky]

Comment on attachment 57700 [details]
Show a handler function instead of Function.prototype.bind


WebCore/inspector/front-end/utilities.js:61
 +      bound.toString = function toString() {
Psease, use anonymouse function on the right here: bound.toString = function() {

[Pavel Feldman]

Comment on attachment 57700 [details]
Show a handler function instead of Function.prototype.bind

Clearing r+ since Nikita won't be able to land it with modifications anyways (he is not yet a committer).

WebCore/inspector/front-end/utilities.js:58
 +      function bound() {
Please move { to the next line.


Also, I am not a fan of this change - it improves things for Web Inspector, but leaves rest of the web developers suffering. None of the major frameworks do this trick for the bind.
In fact, there already is something that could become a standard: see https://bugs.webkit.org/show_bug.cgi?id=25171. I don't like it much, but it is already there. I'd rather support displayName in event listeners list and set in bind to be consistent.

Ideal solution to my taste is when we infer the name from the closure. It is hard though.

[Patrick Mueller]

Using displayName will allow you to see a function name on the call stack in the debugger, in the profiler, and other places.  Not clear if it helps with the toString() output that Nikita originally posted the bug about.  Quick check in WI says it doesn't:

    > var x = function() { }
    undefined

    > x
    function () { }

    > x.displayName = "a_function_called_x"
    "a_function_called_x"

    > x
    function () { }

    > x.toString()
    "function () { }"

But it wouldn't show the "original" function body anyway.

My main objection to the proposed fix is that it's misleading.  The debugger is showing you something which isn't true.  I'd be quite happy if the toString() was implemented like this though:

binded.toString = function toString() {
    return "bound: " + func.toString();
};

[Pavel Feldman]

> But it wouldn't show the "original" function body anyway.
> 
> My main objection to the proposed fix is that it's misleading.  The debugger is showing you something which isn't true.  I'd be quite happy if the toString() was implemented like this though:
> 
> binded.toString = function toString() {
>     return "bound: " + func.toString();
> };

I would be totally fine with the change if we did not have this displayName already. In fact, we don't support it in v8 (and do not need to support it) since we have name inferring that works much better. I wonder if JSC guys are interested in getting rid of displayName and doing inferring as well. In that case, toString would not conflict with the displayName approach. Timothy, what do you say?

[Patrick Mueller]

I forgot to mention that if we do the toString() thuing, we should also do the displayName thing.  So we get the benefits for toString() usage, and on the stack, in the profiler, etc.  Suggest the displayName be set to the derived function name (if there is one!), prefixed with "bound: ".

[Timothy Hatcher]

I don't see what the big deal is with bound.toString = function toString(). It helps the debugger and profiler in JSC. Sure we would rather have inferred names, but we don't yet. Andwho knows when we will.

I don't see any huge negatives from keeping it. The fact that we even talked about this is pretty pathetic. Just remove it and move on. (IIRC, Nikita has done this in other patches.)

Patrick, setting displayName is overkill and will make bind slower for no end user added benifit.

[Pavel Feldman]

Patrick opened my eyes to the fact that we'd like the function body here, not the name only. The reason I keep forgetting about it is that it is lame. In Chromium, we have a link to the source code with the handler function for each event listener instead. I think JSC should catch up soonish. But neither toString nor displayName would fix the problem of navigating to the proper function in case of bind.

I think what we should do is to instrument all the handler registrations and store JavaScript source location at the place of the registration. That clearly adds some overhead and needs to be optional, but it gives us source location of the handler registration. And that is exactly what user needs. He might see call to bind there, but he will see bind's arguments as well. We were thinking of doing this for timeline long ago. It might be good time to start.

[Timothy Hatcher]

That sounds fine. I agree JSC needs to catch up. Then again I'd rather have one JS engine that everyone shared…

[Nikita Vasilyev]

(In reply to comment #8)
> My main objection to the proposed fix is that it's misleading.  The debugger is showing you something which isn't true.  I'd be quite happy if the toString() was implemented like this though:
> 
> binded.toString = function toString() {
>     return "bound: " + func.toString();
> };

Sounds good enough for me. 

So, everyone, what's the final decision?


(In reply to comment #7)
> ... None of the major frameworks do this trick for the bind.

http://dev.jquery.com/ticket/6632
https://prototype.lighthouseapp.com/projects/8886/tickets/1069-make-bound-functions-look-nicely-in-web-inspector-event-listeners-pane


(In reply to comment #12)
> I think what we should do is to instrument all the handler registrations and store JavaScript source location at the place of the registration. That clearly adds some overhead and needs to be optional, but it gives us source location of the handler registration. And that is exactly what user needs. He might see call to bind there, but he will see bind's arguments as well. We were thinking of doing this for timeline long ago. It might be good time to start.

Chrome already did it, isn't it?
http://elv1s.ru/i/chrome-inspector-events-line_numbers.png

[Pavel Feldman]

> So, everyone, what's the final decision?
> 

The final decision is to ask you to prepend 'bound:', fix style nites and let us land it!

> 
> Chrome already did it, isn't it?
> http://elv1s.ru/i/chrome-inspector-events-line_numbers.png

It did, but the line number is going to point to bind no matter you land this fix or now. And it'll become misleading.

[Nikita Vasilyev]

Created attachment 57864 [details]
Show a handler function instead of Function.prototype.bind

[WebKit Commit Bot]

Comment on attachment 57864 [details]
Show a handler function instead of Function.prototype.bind

Clearing flags on attachment: 57864

Committed r60682: <http://trac.webkit.org/changeset/60682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.