The default bug view has changed. See this FAQ.

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.