Last Comment Bug 760868 - Gmail includes scripts names that make the debugger script list too long
: Gmail includes scripts names that make the debugger script list too long
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 13 Branch
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Victor Porof [:vporof][:vp]
:
: James Long (:jlongster)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-02 14:35 PDT by Dave Camp (:dcamp)
Modified: 2012-08-03 07:15 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
v1 (3.89 KB, patch)
2012-06-04 05:38 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (14.80 KB, patch)
2012-06-04 14:45 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2.1 (14.81 KB, patch)
2012-06-04 14:49 PDT, Victor Porof [:vporof][:vp]
past: review+
dao+bmo: feedback-
Details | Diff | Splinter Review
v2.2 (15.35 KB, patch)
2012-06-05 06:52 PDT, Victor Porof [:vporof][:vp]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2012-06-02 14:35:31 PDT
Specifically, a long url starting with recentposts?...
Comment 1 Victor Porof [:vporof][:vp] 2012-06-04 05:38:00 PDT
Created attachment 629747 [details] [diff] [review]
v1

Yup...
Comment 2 Victor Porof [:vporof][:vp] 2012-06-04 14:45:23 PDT
Created attachment 629937 [details] [diff] [review]
v2

Well, this was fun.
Comment 3 Victor Porof [:vporof][:vp] 2012-06-04 14:49:02 PDT
Created attachment 629939 [details] [diff] [review]
v2.1

Minor grammar and typo fixes.
Comment 4 Dão Gottwald [:dao] 2012-06-04 14:50:34 PDT
Comment on attachment 629939 [details] [diff] [review]
v2.1

>+const HELLIP = String.fromCharCode(8230);

use intl.ellipsis
Comment 5 Panos Astithas [:past] 2012-06-05 06:35:25 PDT
Comment on attachment 629939 [details] [diff] [review]
v2.1

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

The only important issue is limiting the filename case, the rest are just nits. I'm a little concerned about the increased complexity from trimURL and to its recursive nature, but I learned to stop worrying and love the bomb^H^H^H^Hpatch.

::: browser/devtools/debugger/debugger-controller.js
@@ +949,4 @@
>    },
>  
>    /**
> +   * Trims as much as possible from an URL, while keeping the result unique

Nit: "a URL"

@@ +956,5 @@
> +   *        The script URL.
> +   * @param string aLabel
> +   *        The resulting label at each step.
> +   * @param number aSeq
> +   *        The current iteration step.

The last two arguments should be marked optional.

@@ +963,2 @@
>     */
> +  _trimURL: function SS__trimURL(aUrl, aLabel, aSeq) {

So, trimUrlQuery there, but trimURL here, huh?
FINE.

@@ +966,5 @@
> +      try {
> +        // Use an nsIURL to parse all the url path parts.
> +        aUrl = Services.io.newURI(aUrl, null, null).QueryInterface(Ci.nsIURL);
> +      } catch (e) {
> +        // This doesn't look like an url, or nsIURL can't handle it.

Nit: "a URL"

@@ +978,5 @@
> +        // base name and extension if available).
> +
> +        // If this url contains an invalid query, unfortunately nsIURL thinks
> +        // it's part of the file extension. It must be removed.
> +        aLabel = aUrl.fileName.replace(/\&.*/, "");

I believe you should check against MAX_REF here as well. There is a pathological case in https://tbpl.mozilla.org/?tree=Mozilla-Inbound

@@ +988,5 @@
> +      aSeq = 1;
> +    }
> +
> +    // If we have a label and it doesn't start with a query...
> +    if (aLabel && !aLabel.match(/^\?/)) {

aLabel.indexOf('?') != 0 should be faster.
Comment 6 Victor Porof [:vporof][:vp] 2012-06-05 06:46:38 PDT
(In reply to Dão Gottwald [:dao] from comment #4)
> 
> I believe you should check against MAX_REF here as well. There is a
> pathological case in https://tbpl.mozilla.org/?tree=Mozilla-Inbound

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 629939 [details] [diff] [review]
> v2.1
> 
> >+const HELLIP = String.fromCharCode(8230);
> 
> use intl.ellipsis

No need for those anymore, we'll use a max-width on the menulist.

> 
> @@ +988,5 @@
> > +      aSeq = 1;
> > +    }
> > +
> > +    // If we have a label and it doesn't start with a query...
> > +    if (aLabel && !aLabel.match(/^\?/)) {
> 
> aLabel.indexOf('?') != 0 should be faster.

But.. me gusta regex..
OK :P
Comment 7 Victor Porof [:vporof][:vp] 2012-06-05 06:52:09 PDT
Created attachment 630153 [details] [diff] [review]
v2.2

Addressed comments.
Comment 8 Panos Astithas [:past] 2012-06-05 07:46:17 PDT
(In reply to Victor Porof from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > >+const HELLIP = String.fromCharCode(8230);
> > 
> > use intl.ellipsis
> 
> No need for those anymore, we'll use a max-width on the menulist.

Yep, we chatted about it in IRC. I forgot to mention that in my previous comment.
Comment 9 Panos Astithas [:past] 2012-06-06 02:03:19 PDT
https://hg.mozilla.org/integration/fx-team/rev/e767617a0003
Comment 10 Tim Taubert [:ttaubert] 2012-06-06 08:42:34 PDT
https://hg.mozilla.org/mozilla-central/rev/e767617a0003
Comment 11 Panos Astithas [:past] 2012-06-20 07:07:51 PDT
Comment on attachment 630153 [details] [diff] [review]
v2.2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: some sites will cause the debugger toolbar to appear chopped, making the feature look unpolished
Testing completed (on m-c, etc.): On m-c and fx-team
Risk to taking this patch (and alternatives if risky): minimal risk, it's a tool only developers will use
String or UUID changes made by this patch: none
Comment 12 Alex Keybl [:akeybl] 2012-06-24 13:23:03 PDT
Comment on attachment 630153 [details] [diff] [review]
v2.2

[Triage Comment]
Feature polish with no risk to the everyday Firefox user.
Comment 14 Ioana (away) 2012-08-03 07:15:24 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0

Ellipses are now used for the long script names.

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