Bug 40080 - Web Inspector: better Function.prototype.bind for the internal code
Summary: Web Inspector: better Function.prototype.bind for the internal code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-02 13:08 PDT by Nikita Vasilyev
Modified: 2010-06-04 08:37 PDT (History)
9 users (show)

See Also:


Attachments
function.bind() causing useless listenerBody message (14.11 KB, image/png)
2010-06-02 13:08 PDT, Nikita Vasilyev
no flags Details
Show handler instead of Function.prototype.bind (14.19 KB, image/png)
2010-06-02 13:16 PDT, Nikita Vasilyev
no flags Details
Show handler instead of Function.prototype.bind (1.50 KB, patch)
2010-06-02 13:19 PDT, Nikita Vasilyev
timothy: review+
timothy: commit-queue+
Details | Formatted Diff | Diff
Show a handler function instead of Function.prototype.bind (1.50 KB, patch)
2010-06-02 13:36 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Show a handler function instead of Function.prototype.bind (1.50 KB, patch)
2010-06-02 14:05 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Show a handler function instead of Function.prototype.bind (1.50 KB, patch)
2010-06-04 04:58 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2010-06-02 13:08:06 PDT
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))); }
Comment 1 Nikita Vasilyev 2010-06-02 13:16:17 PDT
Created attachment 57688 [details]
Show handler instead of Function.prototype.bind
Comment 2 Nikita Vasilyev 2010-06-02 13:19:01 PDT
Created attachment 57689 [details]
Show handler instead of Function.prototype.bind
Comment 3 Nikita Vasilyev 2010-06-02 13:36:14 PDT
Created attachment 57694 [details]
Show a handler function instead of Function.prototype.bind

binded --> bound
Comment 4 Joseph Pecoraro 2010-06-02 13:45:35 PDT
Nikita, great idea!

We should keep this in mind for if/when we end up using a native Function.prototype.bind. (@see bug 26382)
Comment 5 Nikita Vasilyev 2010-06-02 14:05:51 PDT
Created attachment 57700 [details]
Show a handler function instead of Function.prototype.bind

s/binded/bound/ in the ChangeLog.
Comment 6 Yury Semikhatsky 2010-06-03 02:46:26 PDT
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() {
Comment 7 Pavel Feldman 2010-06-03 03:11:54 PDT
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.
Comment 8 Patrick Mueller 2010-06-03 05:20:56 PDT
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();
};
Comment 9 Pavel Feldman 2010-06-03 05:29:59 PDT
> 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?
Comment 10 Patrick Mueller 2010-06-03 06:07:23 PDT
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: ".
Comment 11 Timothy Hatcher 2010-06-03 06:16:44 PDT
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.
Comment 12 Pavel Feldman 2010-06-03 06:18:27 PDT
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.
Comment 13 Timothy Hatcher 2010-06-03 06:21:09 PDT
That sounds fine. I agree JSC needs to catch up. Then again I'd rather have one JS engine that everyone shared…
Comment 14 Nikita Vasilyev 2010-06-04 02:58:57 PDT
(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
Comment 15 Pavel Feldman 2010-06-04 04:42:41 PDT
> 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.
Comment 16 Nikita Vasilyev 2010-06-04 04:58:30 PDT
Created attachment 57864 [details]
Show a handler function instead of Function.prototype.bind
Comment 17 WebKit Commit Bot 2010-06-04 08:37:49 PDT
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>
Comment 18 WebKit Commit Bot 2010-06-04 08:37:56 PDT
All reviewed patches have been landed.  Closing bug.