760868 - Gmail includes scripts names that make the debugger script list too long

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Dave Camp (:dcamp)]

Specifically, a long url starting with recentposts?...

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Yup...

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v2

Well, this was fun.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

Minor grammar and typo fixes.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 629939 [details] [diff] [review]
v2.1

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

use intl.ellipsis

Comment 5

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

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]]

(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]]

Attachment: v2.2

Addressed comments.

Comment 8

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

(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 (he/him) [:past] (please ni?)]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e767617a0003

Comment 11

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

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]]

Comment on attachment 630153 [details] [diff] [review]
v2.2

[Triage Comment]
Feature polish with no risk to the everyday Firefox user.

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8ea76ca18e1

Comment 14

[Ioana (away)]

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.