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

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dcamp, Assigned: vporof)

Tracking

13 Branch
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox15 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Specifically, a long url starting with recentposts?...
Priority: -- → P2
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 629747 [details] [diff] [review]
v1

Yup...
Attachment #629747 - Flags: review?(past)
(Assignee)

Comment 2

5 years ago
Created attachment 629937 [details] [diff] [review]
v2

Well, this was fun.
Attachment #629747 - Attachment is obsolete: true
Attachment #629747 - Flags: review?(past)
Attachment #629937 - Flags: review?(past)
(Assignee)

Comment 3

5 years ago
Created attachment 629939 [details] [diff] [review]
v2.1

Minor grammar and typo fixes.
Attachment #629937 - Attachment is obsolete: true
Attachment #629937 - Flags: review?(past)
Attachment #629939 - Flags: review?(past)
Comment on attachment 629939 [details] [diff] [review]
v2.1

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

use intl.ellipsis
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.
Attachment #629939 - Flags: review?(past) → review+

Updated

5 years ago
Attachment #629939 - Flags: feedback-
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
Created attachment 630153 [details] [diff] [review]
v2.2

Addressed comments.
(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.
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/e767617a0003
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e767617a0003
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
Attachment #629939 - Attachment is obsolete: true
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
Attachment #630153 - Flags: approval-mozilla-aurora?
Comment on attachment 630153 [details] [diff] [review]
v2.2

[Triage Comment]
Feature polish with no risk to the everyday Firefox user.
Attachment #630153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b8ea76ca18e1
status-firefox15: --- → fixed

Comment 14

5 years ago
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.
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.