Last Comment Bug 690404 - Simplify the script name in the script debugger
: Simplify the script name in the script debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 13
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 732501
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2011-09-29 09:56 PDT by Panos Astithas [:past] (away until 7/21)
Modified: 2012-03-06 11:52 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.75 KB, patch)
2012-02-18 05:49 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (3.90 KB, patch)
2012-02-18 10:42 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (3.41 KB, patch)
2012-02-18 11:57 PST, Victor Porof [:vporof][:vp]
past: review-
Details | Diff | Splinter Review
startLineBug (2.80 MB, video/quicktime)
2012-02-18 11:59 PST, Victor Porof [:vporof][:vp]
no flags Details
chromeBug (377.39 KB, image/png)
2012-02-18 11:59 PST, Victor Porof [:vporof][:vp]
no flags Details
v3 (3.96 KB, patch)
2012-02-22 06:49 PST, Victor Porof [:vporof][:vp]
past: feedback+
Details | Diff | Splinter Review
v4 (14.15 KB, patch)
2012-02-24 02:37 PST, Victor Porof [:vporof][:vp]
past: review-
Details | Diff | Splinter Review
v4.1 (14.42 KB, patch)
2012-02-28 09:43 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4.2 (14.97 KB, patch)
2012-02-29 07:37 PST, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review
v4.4 (14.96 KB, patch)
2012-02-29 09:20 PST, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] (away until 7/21) 2011-09-29 09:56:27 PDT
We currently show the full script URL in the script list, but we could do better. WebKit and Opera only show the filename for JS files and either the last path element for inline scripts (WebKit), or the label "Inline" along with a script.toSource() (Opera).

Another thing to consider is inline scripts: Opera shows them (eval label + toSource()), WebKit does not. Also, SpiderMonkey will be providing us with Debugger.Script instances for functions, besides the scripts themselves. We should probably suppress those.
Comment 1 Victor Porof [:vporof][:vp] 2012-02-18 05:49:03 PST
Created attachment 598523 [details] [diff] [review]
v1
Comment 2 Victor Porof [:vporof][:vp] 2012-02-18 10:42:26 PST
Created attachment 598550 [details] [diff] [review]
v2

Made things a bit easier for bug 723047.
Comment 3 Victor Porof [:vporof][:vp] 2012-02-18 11:57:49 PST
Created attachment 598559 [details] [diff] [review]
v2.1

Simplified and cleaned up things.
I also found a couple of things that may be bugs: after a couple of refreshes, the script.startLine is sometimes 0 (even though it's correct every other time - see attached video).
Moreover, I managed to get a chrome:// script while debugging cnn. Something tells me this shouldn't happen.
Comment 4 Victor Porof [:vporof][:vp] 2012-02-18 11:59:08 PST
Created attachment 598561 [details]
startLineBug
Comment 5 Victor Porof [:vporof][:vp] 2012-02-18 11:59:59 PST
Created attachment 598562 [details]
chromeBug
Comment 6 Panos Astithas [:past] (away until 7/21) 2012-02-20 09:42:28 PST
Comment on attachment 598559 [details] [diff] [review]
v2.1

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

I really like how much better the UI is now, with this patch. r- for now, but we are very close. As mentioned in bug 723047, we will need a test to check that script name simplification works, either here or there.

Some general comments: the signature of DVS_addscript includes an aScriptNameText parameter, which remains unused. We should either get rid of the parameter or move the name simplification code to SS_addScript. Also, there is a typo in the jsdoc for the aSource parameter, can you fix that, too?

(In reply to Victor Porof from comment #3)
> I also found a couple of things that may be bugs: after a couple of
> refreshes, the script.startLine is sometimes 0 (even though it's correct
> every other time - see attached video).

Can you get me the protocol output from the console for this? If it is 0, then it must be the dynamically added event handler, but I'd be interested to know if it was undefined, too.

> Moreover, I managed to get a chrome:// script while debugging cnn. Something
> tells me this shouldn't happen.

Never seen this. If this script does not appear in a protocol notification in the console, it could be related to the use of getMostRecentWindow in this patch.

::: browser/devtools/debugger/debugger-view.js
@@ +1166,4 @@
>        return null;
>      }
>  
> +    let inline = DebuggerView.getStr("inline");

This is used only once, we can inline it.

@@ +1178,5 @@
> +      simplified = aUrl.split("/").pop() ||
> +                   aUrl.replace(domain, "").substring(1) || aUrl;
> +
> +      if (aSource.startLine > 1) {
> +        title = inline + " :" + aSource.startLine + " - " + simplified;

I don't think the line number is useful in the script label and the simplified name is plenty without the additional "Inline" tag. We will not be showing many entries for the same HTML file anyway.

@@ +1184,5 @@
> +        title = simplified;
> +      }
> +    }
> +    if (!title) {
> +      // these are scripts from other domains, just remove the query

Why? I can't think why knowing the exact CDN URL of jquery.min.js would be useful.

@@ +1190,5 @@
> +      title = q > -1 ? aUrl.slice(0, q) : aUrl;
> +      simplified = title;
> +    }
> +    if (!title) {
> +      // everything else failed for some reason, use the full url

This second if (!title) in a row seems fishy. Which cases does this handle? Would moving all of this logic in SS_addScript simplify it?

@@ +1200,2 @@
>      script.setUserData("sourceScript", aSource, null);
> +    script.setUserData("simplifiedScriptLocation", simplified, null);

simplifiedScriptLocation seems rather long, how about simpleName? Your choice.

@@ +1203,3 @@
>      this._scripts.selectedItem = script;
>      return script;
>    },

This method does not distinguish between two files with the same name, but in different URIs. See http://astithas.com/test/bp/index.html for an example test case. Since we need to have unique labels, adding the last path element for the longest path would suffice, I think.

Also, we should display the full URL as a tooltip, so that a confused user can be certain which script is which.

@@ +1226,5 @@
> +  get _contentDomain() {
> +    let url = Cc["@mozilla.org/appshell/window-mediator;1"]
> +      .getService(Ci.nsIWindowMediator)
> +      .getMostRecentWindow("navigator:browser")
> +      .content.location.href;

We generally avoid using getMostRecentWindow. You can get the same thing with window.parent.DebuggerUI.aWindow. This, however, suggests that DebuggerUI might be the best place for defining this function and then patching it into debugger.js.

@@ +1229,5 @@
> +      .getMostRecentWindow("navigator:browser")
> +      .content.location.href;
> +
> +    let uri = Services.io.newURI(url, null, null);
> +    let scheme = Services.io.extractScheme(url);

You can inline this.

@@ +1232,5 @@
> +    let uri = Services.io.newURI(url, null, null);
> +    let scheme = Services.io.extractScheme(url);
> +
> +    if (scheme === "file") {
> +      return url.replace(uri.QueryInterface(Ci.nsIFileURL).file.leafName, "").slice(0, -1);

This seems a bit heavy compared to using lastIndexOf('/'), but OK.

@@ +1233,5 @@
> +    let scheme = Services.io.extractScheme(url);
> +
> +    if (scheme === "file") {
> +      return url.replace(uri.QueryInterface(Ci.nsIFileURL).file.leafName, "").slice(0, -1);
> +    } else {

Redundant else clause.
Comment 7 Victor Porof [:vporof][:vp] 2012-02-22 06:49:33 PST
Created attachment 599591 [details] [diff] [review]
v3
Comment 8 Panos Astithas [:past] (away until 7/21) 2012-02-23 08:04:34 PST
Comment on attachment 599591 [details] [diff] [review]
v3

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

Me likes! One bug still, a few nits, a test and we're done.

::: browser/devtools/debugger/debugger-view.js
@@ +1090,5 @@
> +   *        The script label.
> +   * @return object
> +   *         The requested script or null if nothing was found.
> +   */
> +  scriptWithLabel: function DVS_scriptWithLabel(aLabel) {

containsLabel(aLabel) would be more consistent with contais(aUrl) and shorter to boot. Also, since we only use it in a conditional, returning true/false would be more appropriate.

::: browser/devtools/debugger/debugger.js
@@ +536,4 @@
>    },
>  
>    /**
> +   * Gets a simplified label from a script url.

"Gets a unique, simplified label" to be more explicit.

@@ +554,5 @@
> +   */
> +  _getScriptLabel: function SS_getScriptLabel(aUrl) {
> +    let href = window.parent.content.location.href;
> +
> +    let path = href.substring(0, href.lastIndexOf("/", href.length - 2) + 1);

Why href.length-2? This motivated me to create another test page: http://astithas.com/test/a/

We get one path wrong in that page.

@@ +568,5 @@
> +    // Trim the query part.
> +    let q = label.indexOf('?');
> +    if (q > -1) {
> +      label = label.slice(0, q);
> +    }

These 4 lines appear twice in this file. Let's make them a private helper method.
Comment 9 Victor Porof [:vporof][:vp] 2012-02-24 02:37:11 PST
Created attachment 600341 [details] [diff] [review]
v4
Comment 10 Panos Astithas [:past] (away until 7/21) 2012-02-24 05:04:31 PST
Comment on attachment 600341 [details] [diff] [review]
v4

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

I found a bug with the cache after a location change, but otherwise this is ready to land.

::: browser/devtools/debugger/debugger.js
@@ +551,5 @@
> +   * @return string
> +   *         The simplified label.
> +   */
> +  _getScriptLabel: (function() {
> +    let cache = {};

Using a closure instead of a private property is somewhat inconsistent with our style so far, but I don't care that much. The problem is you never clear the cache from its contents, so that location changes can get a fresh cache. Try visiting this page first with the debugger open:

http://astithas.com/test/bp/index.html

and then visit this one, without closing and reopening the debugger:

http://astithas.com/test/a/

@@ +562,5 @@
> +      }
> +
> +      let href = aHref || window.parent.content.location.href;
> +      let split = url.split("/");
> +      let label = split.pop() || (split.pop() + "/");

This is confusing. Can you rename split to pathElements or similar?

@@ +567,5 @@
> +
> +      if (DebuggerView.Scripts.containsLabel(label)) {
> +        label = url.replace(href.substring(0, href.lastIndexOf("/") + 1), "");
> +      }
> +      if (DebuggerView.Scripts.containsLabel(label)) {

I know see what is unclear to me here: if you move this if clause inside the previous one, it will be obvious that you are checking for the results of a side-effect and not fat-fingered a conditional.

::: browser/devtools/debugger/test/browser_dbg_propertyview-01.js
@@ +96,5 @@
> + /*3*/{ href: "si://interesting.address.moc/random/", leaf: "script.js" },
> + /*4*/{ href: "si://interesting.address.moc/random/", leaf: "x/script.js" },
> + /*5*/{ href: "si://interesting.address.moc/random/", leaf: "x/y/script.js?a=1" },
> + /*6*/{ href: "si://interesting.address.moc/random/x/", leaf: "y/script.js?a=1&b=2" },
> + /*7*/{ href: "si://interesting.address.moc/random/x/y/", leaf: "script.js?a=1&b=2&c=3" }

These comments look weird to me, but OK I guess.
Comment 11 Victor Porof [:vporof][:vp] 2012-02-25 01:08:40 PST
(In reply to Panos Astithas [:past] from comment #10)
> Comment on attachment 600341 [details] [diff] [review]
> 
> Using a closure instead of a private property is somewhat inconsistent with
> our style so far, but I don't care that much. The problem is you never clear
> the cache from its contents, so that location changes can get a fresh cache.
> Try visiting this page first with the debugger open:
> 
> http://astithas.com/test/bp/index.html
> 
> and then visit this one, without closing and reopening the debugger:
> 
> http://astithas.com/test/a/
> 

I'm not sure how the behavior should be different whether the cache is cleared or not, because labels are stored based on their url. Different url => different hits. Could you please make a screencast of what's going on wrong in there?
Comment 12 Panos Astithas [:past] (away until 7/21) 2012-02-28 02:39:11 PST
(In reply to Victor Porof from comment #11)
> (In reply to Panos Astithas [:past] from comment #10)
> > Comment on attachment 600341 [details] [diff] [review]
> > 
> > Using a closure instead of a private property is somewhat inconsistent with
> > our style so far, but I don't care that much. The problem is you never clear
> > the cache from its contents, so that location changes can get a fresh cache.
> > Try visiting this page first with the debugger open:
> > 
> > http://astithas.com/test/bp/index.html
> > 
> > and then visit this one, without closing and reopening the debugger:
> > 
> > http://astithas.com/test/a/
> > 
> 
> I'm not sure how the behavior should be different whether the cache is
> cleared or not, because labels are stored based on their url. Different url
> => different hits. Could you please make a screencast of what's going on
> wrong in there?

http://www.youtube.com/watch?v=ObTdXmtbCOY

See that when the page changes the script list no longer contains all scripts.
Comment 13 Victor Porof [:vporof][:vp] 2012-02-28 04:39:13 PST
(In reply to Panos Astithas [:past] from comment #12)
> http://www.youtube.com/watch?v=ObTdXmtbCOY
> 
> See that when the page changes the script list no longer contains all
> scripts.

The video is private :(
Comment 14 Panos Astithas [:past] (away until 7/21) 2012-02-28 07:22:36 PST
(In reply to Victor Porof from comment #13)
> (In reply to Panos Astithas [:past] from comment #12)
> > http://www.youtube.com/watch?v=ObTdXmtbCOY
> > 
> > See that when the page changes the script list no longer contains all
> > scripts.
> 
> The video is private :(

Oops, sorry! I think I fixed it now.
Comment 15 Victor Porof [:vporof][:vp] 2012-02-28 09:42:15 PST
(In reply to Panos Astithas [:past] from comment #12)
> http://www.youtube.com/watch?v=ObTdXmtbCOY
> 
> See that when the page changes the script list no longer contains all
> scripts.

I really don't see how that bug is related to this patch, since it happens with an empty patchqueue as well. Tested on a latest m-c pull+build, please see http://youtu.be/8D_7lNZ2sNQ
Comment 16 Victor Porof [:vporof][:vp] 2012-02-28 09:43:49 PST
Created attachment 601306 [details] [diff] [review]
v4.1

Addressed feedback from comment #10, apart from the clear cache issue.
Comment 17 Panos Astithas [:past] (away until 7/21) 2012-02-29 03:07:08 PST
(In reply to Victor Porof from comment #15)
> (In reply to Panos Astithas [:past] from comment #12)
> > http://www.youtube.com/watch?v=ObTdXmtbCOY
> > 
> > See that when the page changes the script list no longer contains all
> > scripts.
> 
> I really don't see how that bug is related to this patch, since it happens
> with an empty patchqueue as well. Tested on a latest m-c pull+build, please
> see http://youtu.be/8D_7lNZ2sNQ

OK, so we can fix this in a followup. I filed bug 731537 for this.

That aside, isn't it obvious that having a constantly growing cache is not a good idea, due to wasted memory if nothing else? Here is a troublesome scenario I came up with: user debugs a page with a single script in a /bar.js path, which is simplified to bar.js for our script list. Then he continues development and moves the script to /foo/bar.js and reloads. Won't we show a foo/bar.js label, even though there is no other script in the page, due to still having the previous entry in the cache?

Why don't we simply clear the cache on page navigation and not have to worry about any of this?
Comment 18 Victor Porof [:vporof][:vp] 2012-02-29 07:37:09 PST
Created attachment 601608 [details] [diff] [review]
v4.2
Comment 19 Panos Astithas [:past] (away until 7/21) 2012-02-29 09:14:09 PST
Comment on attachment 601608 [details] [diff] [review]
v4.2

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

Just a small nit, but this is golden!

::: browser/devtools/debugger/debugger.js
@@ +84,4 @@
>          ThreadState.connect(aThreadClient, function() {
>            StackFrames.connect(aThreadClient, function() {
>              SourceScripts.connect(aThreadClient, function() {
> +              SourceScripts.clearLabelsCache();

I think it would be clearer to place this call inside SS_connect, next to this.onScriptsCleared, since they complement each other nicely.
Comment 20 Victor Porof [:vporof][:vp] 2012-02-29 09:20:28 PST
Created attachment 601641 [details] [diff] [review]
v4.4
Comment 21 Panos Astithas [:past] (away until 7/21) 2012-02-29 09:25:52 PST
Comment on attachment 601641 [details] [diff] [review]
v4.4

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

No need to request another review since I already r+ it, unless you just want to hear me say it again: excellent job! :-)
Comment 22 Rob Campbell [:rc] (:robcee) 2012-03-01 05:34:07 PST
https://hg.mozilla.org/integration/fx-team/rev/0d283b67acc8
Comment 23 Tim Taubert [:ttaubert] 2012-03-02 01:51:30 PST
https://hg.mozilla.org/mozilla-central/rev/0d283b67acc8
Comment 24 Matt Brubeck (:mbrubeck) 2012-03-06 10:29:29 PST
This bug (or possibly one of the ones it landed with like bug 723047) seems to have caused a new top test failure (bug 732501).  Can we try backing this out on Try or Inbound?  Or is someone working on fixing the test failure?
Comment 25 Panos Astithas [:past] (away until 7/21) 2012-03-06 11:33:07 PST
I was hoping to get a chance to see if bug 729191 helps or not, before considering a backout. Actually, disabling the test temporarily sounds like a better idea in this case.
Comment 26 Matt Brubeck (:mbrubeck) 2012-03-06 11:52:26 PST
I don't think the failure rate is bad enough that the test has to be disabled, if it's still testing useful stuff.  Just wanted to make sure someone was following up on the timeouts.

Note You need to log in before you can comment on or make changes to this bug.