897050 - Prefer displayName instead of name

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looking at the documentation[0], it seems to be better in every way. Worst case, it is the same as name; other cases, it gives more detailed information.

https://wiki.mozilla.org/Debugger#Accessor_Properties_of_the_Debugger.Object_prototype

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Actually, one benefit of the current code is that the client can tell if the function name was set by the user or inferred by the compiler without parsing the source. Granted, our current front-end doesn't make use of that distinction, but I can imagine others may well want to.

Comment 2

[Jonathan [:elefont]]

Is this still open? It seems like it would be a good first issue for me. Although I'm not very good as Javascript.

Comment 3

[Anup Kumar [:reznord]]

Hi, 

I am willing to work on this bug. So please I request you to assign it to me.

Thanks in Advance,

Regards,
A. Anup

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jonathan, sorry I missed your comment. Yes, the bug is still open! To start, get yourself a local build of fx-team going, and start a new patch in mercurial queue. To learn how to do this, check out https://wiki.mozilla.org/DevTools/Hacking

Once you have figured that out and got your build going, your going to want to make changes to ObjectActor.prototype.grip[0]. A grip[1] is the JSON representation of an object being debugged over the remote debugging protocol.

You need to change the code so both `name` and `displayName` are attached to the grip at all times instead of one or the other. This will allow clients to always have all the information on the function, and let them choose how they want to display the function to its users.

Then, you will need to change our debugger frontend to prefer `displayName` over `name` when availble: http://mxr.mozilla.org/mozilla-central/search?string=displayName&find=browser%2Fdevtools%2Fdebugger&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

You will also need to update this xpcshell test: /toolkit/devtools/server/tests/unit/test_functiongrips-01.js 

In the devtools hacking guide, there are instructions on how to run the tests, when you have the changes completed, and tests passing locally, upload your patch to this bug and flag me for review!

If you have any other questions, feel free to ping me on irc, or comment in this bug again.

Cheers!

[0]: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2324

[1]: https://wiki.mozilla.org/Remote_Debugging_Protocol#Grips

Comment 5

[Jonathan [:elefont]]

Nick, thanks for replying! I'm currently building the fx-team project, and I've been reading up on the remote debugging protocol so that I have a better idea of what I'm going to fix. I'll definitely keep you updated as I make progress, run into issues, etc. 

--Jonathan

Comment 6

[Jonathan [:elefont]]

Attachment: debuggerDisplayName.patch

Nick, I think I fixed the bug. I made changes to the following files, which you'll find in the attached patch:

1) /browser/devtools/debugger/debugger-toolbar.js
2) /toolkit/devtools/server/actors/script.js
3) /toolkit/devtools/server/tests/unit/test_functiongrips-01.js

Please review & let me know if all is well. If not, could we please discuss it so that I can see & understand where I went wrong? I would appreciate any feedback. Thanks.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 796455 [details] [diff] [review]
debuggerDisplayName.patch

Review of attachment 796455 [details] [diff] [review]:
-----------------------------------------------------------------

This look great! Thanks for the path, Jonathan :)

I have just a couple small requests (detailed inline below). Once you fix those and upload the new patch, I will push the patch to try[0] where we will aggregate test results from all of the platforms firefox runs on. Assuming the tests come back green, I'll push the patch to fx-team integration repository, where it will incubate for a day or so, then will be merged into mozilla-central and show up in the next Nightly!

::: toolkit/devtools/server/actors/script.js
@@ +2336,5 @@
>      // Add additional properties for functions.
>      if (this.obj.class === "Function") {
>        if (this.obj.name) {
>          g.name = this.obj.name;
> +      } if (this.obj.displayName) {

Style nit: please put this if on its own line, instead of the same line as the previous if's closing "{".

::: toolkit/devtools/server/tests/unit/test_functiongrips-01.js
@@ +28,5 @@
>    gThreadClient.addOneTimeListener("paused", function(aEvent, aPacket) {
>      let args = aPacket.frame.arguments;
>  
>      do_check_eq(args[0].class, "Function");
> +    do_check_eq(args[0].displayName, "stopMe");

Since we are always attaching name and displayName if available, we should be testing that both properties are attached here. Please update the patch so that we test both "name" and "displayName" here.

Comment 8

[Jonathan [:elefont]]

Attachment: debuggerDisplayName-revised.patch

Nick, I read your comments, and I've attached a revised patch.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Jonathan, thanks for the update. Please attach the whole patch as a single patch file, and mark any older versions of the patch as obsolete, rather than attaching another patch of your incremental changes. This makes it a whole lot easier for me to review.

Also, instead of flagging "review+", flag "review?" and enter my email in the box ([e-mail]) so that I will get an email notifying me you have asked me for review.

Thanks!

Comment 10

[Jonathan [:elefont]]

Attachment: bug_897050.patch

Nick,

I included all the changes in this single patch instead of "piecemeal" patches I was attaching earlier. Sorry about my n00b mistakes! I'm still a n00b with a long way to go :)

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 797671 [details] [diff] [review]
bug_897050.patch

Review of attachment 797671 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

Here is the try push: https://tbpl.mozilla.org/?tree=Try&rev=43db5445de82

Assuming it comes back good, I will push this to fx-team :)

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jonathan, looks like browser/devtools/debugger/tests/browser_dbg_displayName.js is failing. To catch this stuff early, make sure you are running tests[0] before uploading patches, specifically for the debugger:

  $ ./mach xpcshell-test toolkit/devtools/server
  $ ./mach mochitest-browser browser/devtools/debugger

The second test command would have caught this test.

To solve it, I think the correct solution is actually to prefer userDisplayName to displayName to name.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

[0]: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests

Comment 14

[Jonathan [:elefont]]

Nick,

I tried your suggestion, and all the xpcshell tests & Chrome Mochitests seem to pass. The specific test you mentioned (browser/devtools/debugger/tests/browser_dbg_displayName.js) seems to pass as well. However, there are two issues:

1) The Browser Mochitest test suite is failing. This is what I see in the terminal while running that particular test suite. I'm not sure what's going on:  0:35.59 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_addon.js | Found a tab after previous test timed out: about:blank

2) I'm not sure how changing the order of the properties passes the test that originally failed. I think this really stems from my lacking of familiarity/understanding with function properties in grips. I'll try to present my current understanding here. I would appreciate it if you could look over it & tell me if I'm right or wrong. We have the following boolean expression:

(c.displayName || c.userDisplayName || c.name || "(anonymous)")

Yet the test, browser_dbg_displayName.js, fails when we put displayName before userDisplayName:
browser/devtools/debugger/test/browser_dbg_displayName.js | Frame name should be anonFunc - Got a/<, expected anonFunc

According to the wiki, displayName is "a name the system has inferred for the function", and userDisplayName is the value of displayName if that value is a string. So it seems like anytime displayName has a string value, userDisplayName will have the exact same value.

In browser_dbg_displayName.html, there is the following line of code:

anon.displayName = "anonFunc";

Ok, so displayName has a string value of "anonFunc", which means that userDisplayName must also have that value. So as far as I can tell, we have two equal, "truthy" values in that boolean expression. I don't understand how switching the two would cause a test to suddenly pass. 

I must have misunderstood something about the remote debugging protocol. What did I miss?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi Jonathan!

(In reply to Jonathan from comment #14)
> Nick,
> 
> I tried your suggestion, and all the xpcshell tests & Chrome Mochitests seem
> to pass. The specific test you mentioned
> (browser/devtools/debugger/tests/browser_dbg_displayName.js) seems to pass
> as well. However, there are two issues:
> 
> 1) The Browser Mochitest test suite is failing. This is what I see in the
> terminal while running that particular test suite. I'm not sure what's going
> on:  0:35.59 TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/commandline/test/
> browser_cmd_addon.js | Found a tab after previous test timed out: about:blank

I am 98% sure that this is an unrelated, known bug, and if you were to run the tests on a fresh check out of fx-team without your changes, you would probably see the failure.

Generally as a rule of thumn, when you are changing code for the debugger, you only need to run

  $ ./mach xpcshell-test toolkit/devtools/server

and

  $ ./mach mochitest-browser browser/devtools/debugger

If those two suites are passing, feel free to upload a patch and I can push it to try server where we can do more comprehensive, cross-platform testing, and figure out if seemingly unrelated failures are known failures or caused by your changes at that time.

> 
> 2) I'm not sure how changing the order of the properties passes the test
> that originally failed. I think this really stems from my lacking of
> familiarity/understanding with function properties in grips. I'll try to
> present my current understanding here. I would appreciate it if you could
> look over it & tell me if I'm right or wrong. We have the following boolean
> expression:
> 
> (c.displayName || c.userDisplayName || c.name || "(anonymous)")

What I was trying to get at in comment 12 was that this code should be

  (c.userDisplayName || c.displayName || c.name || "(anonymous)")

and the same for the other place where we have this logic. Bonus points for consolidating them both to

  function getFunctionName(f) {
    return f.userDisplayName
      || f.displayName
      || f.name
      || "(anonymous)";
  }

but we don't have to do that in this bug, we can make a follow up.

> 
> Yet the test, browser_dbg_displayName.js, fails when we put displayName
> before userDisplayName:
> browser/devtools/debugger/test/browser_dbg_displayName.js | Frame name
> should be anonFunc - Got a/<, expected anonFunc
> 
> According to the wiki, displayName is "a name the system has inferred for
> the function", and userDisplayName is the value of displayName if that value
> is a string. So it seems like anytime displayName has a string value,
> userDisplayName will have the exact same value.
> 
> In browser_dbg_displayName.html, there is the following line of code:
> 
> anon.displayName = "anonFunc";
> 
> Ok, so displayName has a string value of "anonFunc", which means that
> userDisplayName must also have that value. So as far as I can tell, we have
> two equal, "truthy" values in that boolean expression. I don't understand
> how switching the two would cause a test to suddenly pass. 
> 
> I must have misunderstood something about the remote debugging protocol.
> What did I miss?

Very close. It is true that because anon.displayName is set to "anonFunc" the Debugger.Object we get for the function has userDisplayName equal to "anonFunc". However, if you go back to our logic for choosing a function name to display, you'll see we prefer displayName to userDisplayName; therefore the test fails. Change the order of preference like I comment above and it should work.

Cheers!

Comment 16

[Jonathan [:elefont]]

Attachment: bug_897050.patch

Nick,

I ran the two test suites you mentioned, and the following test suite ran fine:
./mach xpcshell-test toolkit/devtools/server/

However, this test suite failed:
./mach mochitest-browser browser/devtools/debugger/

and here's the error message. I think it's the same one you mentioned is a known bug:
 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_cmd.js | Found an unexpected tab at the end of test run: http://example.com/browser/browser/devtools/debugger/test/browser_dbg_cmd.html

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 802026 [details] [diff] [review]
bug_897050.patch

Review of attachment 802026 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! I will push it to try and determine if the failure you are seeing is a known bug, or introduced here.

::: toolkit/devtools/server/actors/script.js
@@ +2336,5 @@
>      // Add additional properties for functions.
>      if (this.obj.class === "Function") {
>        if (this.obj.name) {
>          g.name = this.obj.name;
> +      } 

Nit: trailing white space, please remove it.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=6e0a848298d4

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push since none of the tests ran yesterday (infrastructure issues):

https://tbpl.mozilla.org/?tree=Try&rev=7e9e1cded806

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Fixed in fx-team!

https://hg.mozilla.org/integration/fx-team/rev/4f25eb972870

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f25eb972870