Closed Bug 690404 Opened 13 years ago Closed 12 years ago

Simplify the script name in the script debugger

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: past, Assigned: vporof)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
Component: Developer Tools → Developer Tools: Debugger
QA Contact: developer.tools → developer.tools.debugger
Priority: -- → P2
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #598523 - Flags: review?(past)
Attached patch v2 (obsolete) — Splinter Review
Made things a bit easier for bug 723047.
Attachment #598523 - Attachment is obsolete: true
Attachment #598523 - Flags: review?(past)
Attachment #598550 - Flags: review?(past)
Attached patch v2.1 (obsolete) — Splinter Review
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.
Attachment #598550 - Attachment is obsolete: true
Attachment #598550 - Flags: review?(past)
Attachment #598559 - Flags: review?(past)
Attached video startLineBug (obsolete) —
Attached image chromeBug (obsolete) —
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.
Attachment #598559 - Flags: review?(past) → review-
Attached patch v3 (obsolete) — Splinter Review
Attachment #598559 - Attachment is obsolete: true
Attachment #598561 - Attachment is obsolete: true
Attachment #598562 - Attachment is obsolete: true
Attachment #599591 - Flags: feedback?(past)
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.
Attachment #599591 - Flags: feedback?(past) → feedback+
Attached patch v4 (obsolete) — Splinter Review
Attachment #600341 - Flags: review?(past)
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.
Attachment #600341 - Flags: review?(past) → review-
Attachment #599591 - Attachment is obsolete: true
(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?
(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.
(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 :(
(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.
(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
Attached patch v4.1 (obsolete) — Splinter Review
Addressed feedback from comment #10, apart from the clear cache issue.
Attachment #600341 - Attachment is obsolete: true
Attachment #601306 - Flags: review?(past)
(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?
Attached patch v4.2 (obsolete) — Splinter Review
Attachment #601306 - Attachment is obsolete: true
Attachment #601306 - Flags: review?(past)
Attachment #601608 - Flags: review?(past)
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.
Attachment #601608 - Flags: review?(past) → review+
Attached patch v4.4Splinter Review
Attachment #601641 - Flags: review?(past)
Attachment #601608 - Attachment is obsolete: true
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! :-)
Attachment #601641 - Flags: review?(past) → review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/0d283b67acc8
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0d283b67acc8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Depends on: 732501
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?
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.
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.