611440 - Smart abbreviation for URLs in the Web Console

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Patrick Walton (:pcwalton)]

Per a discussion in IRC we'd like to see "smart abbreviation" for URLs. The idea is that, for long URLs, we show the first part of the URL and the last component + first part of the query string. Some examples:

http://www.google.com/search?q=kitten+pictures&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:en-US:official&client=firefox-a => www.google.com/search?q=kitten+pictures&...
http://techcrunch.com/2010/11/11/itunes-ping-actually-goes-social-with-full-twitter-integration-facebook-still-mia/ => www.techcrunch.com/.../itunes-ping-actually-goes-.../
http://www.nytimes.com/external/gigaom/2010/11/10/10gigaom-one-third-of-top-grossing-iphone-apps-are-free-86181.html?ref=technology => www.nytimes.com/.../10gigaom-one-third-of-...?ref=technology

Ideally we'd like the user to be able to resize the window and see more and more of the URL appear.

Comment 1

[Patrick Walton (:pcwalton)]

So this is going to need to be based on top of bug 605621, because this depends significantly on the way that network requests are displayed.

Comment 2

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 3

[Nochum Sossonko [:Natch]]

You want to set the cropping (via the 'crop' attribute). It should do what you need...

Comment 4

[Patrick Walton (:pcwalton)]

(In reply to comment #3)
> You want to set the cropping (via the 'crop' attribute). It should do what you
> need...

That only abbreviates in the middle AIUI. It'd be better to be a little smarter with hostnames.

Also, I'm not sure if that works with multi-line descriptions, does it?

Comment 5

[Nochum Sossonko [:Natch]]

(In reply to comment #4)
> That only abbreviates in the middle AIUI. It'd be better to be a little smarter
> with hostnames.

No, it crops wherever you want it to crop (see bug 305206 for a similar attempt with the error console, although it's a long patch...).

> Also, I'm not sure if that works with multi-line descriptions, does it?

I _think_ it does, not sure...

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to comment #5)
> (In reply to comment #4)
> > That only abbreviates in the middle AIUI. It'd be better to be a little smarter
> > with hostnames.
> 
> No, it crops wherever you want it to crop (see bug 305206 for a similar attempt
> with the error console, although it's a long patch...).

Well, we want to crop at the end of the hostname, at the beginning of the path, and at the end of the query string. I don't think there's a way to do that with "crop": it only has a few settings.

Comment 7

[Nochum Sossonko [:Natch]]

I would suggest a regex that cuts the string up at the desired places so that

http://error.example.com/long_path/to/error_location/error.html

becomes

http://error.example.com/ | long_path/to/error_location/ | error.html

then use 3 descriptions/labels and flex/crop as desired. IOW, put the maximum flex on the middle path, minimum on the beginning hostname and medium on the ending file name. Make sure to crop appropriately for each as well.

Comment 8

[Patrick Walton (:pcwalton)]

This is going to break string freeze, because we have two localization strings called "networkUrlWithStatus" and "networkUrlWithStatusAndDuration" that put the URL together with the status and duration - but we now need to split the URL off into a separate XUL/HTML element from the status and/or duration.

Drivers will need to determine whether this feature is worth breaking the string freeze.

Comment 9

[Patrick Walton (:pcwalton)]

Actually, never mind. This patch involves *removing* those two strings, but thankfully the "NetworkPanel.durationMS" saves us from breaking string freeze: we already have the localization for "ms". Yay!

Comment 10

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached.

Comment 11

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 2.

Patch version 2 adds the CSS to winstripe and gnomestripe as well.

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Screenshot.

Screenshot attached.

Comment 13

[Rob Campbell [:rc] (:robcee)]

looking good.

What's going to happen on selection+copy? Are we getting the full text of the URL in the copy buffer or will it be truncated?

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 494858 [details] [diff] [review]
Proposed patch, version 2.

Patch comments:

- What's the difference between .webconsole-msg-url and .webconsole-msg-link?

- Why do you add .webconsole-msg-body-piece? I am asking these questions because we are growing the number of class names up to confusion.

- In browser_webconsole_bug_601177_log_levels.js: you removed the use of the msgs variable, but you did not remove let msgs; and msgs = null.

- In head.js: why not put the labels loop outside of the msgs loop? Just search and loop once through all of the labels in outputNode.

The patch looks fine otherwise, and I like the visual results. Web Console's output is now much more polished. Feedback+ with the above comments addressed.

Comment 15

[Patrick Walton (:pcwalton)]

(In reply to comment #14)
> Comment on attachment 494858 [details] [diff] [review]
> Proposed patch, version 2.
> 
> Patch comments:
> 
> - What's the difference between .webconsole-msg-url and .webconsole-msg-link?

.webconsole-msg-url is the actual URL; .webconsole-msg-link is the URL plus the status (i.e. the part that's clickable).

> - Why do you add .webconsole-msg-body-piece? I am asking these questions
> because we are growing the number of class names up to confusion.

To stop the inheritance of margin and padding.

Comment 16

[Kevin Dangoor]

Patrick: any comment on Rob's note in comment 13 about copying? That's a good point that you'd want to copy the full URL if you're copying it out of the console.

Comment 17

[Rob Campbell [:rc] (:robcee)]

I'd be happy with a follow-up bug if you don't want to worry about it here.

Comment 18

[Patrick Walton (:pcwalton)]

(In reply to comment #13)
> looking good.
> 
> What's going to happen on selection+copy? Are we getting the full text of the
> URL in the copy buffer or will it be truncated?

It should Just Work, but I can verify.

Comment 20

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 3: Code changes.

Patch version 3 rebases to the current state of bug 605621. Review requested.

Comment 21

[Patrick Walton (:pcwalton)]

(In reply to comment #18)
> (In reply to comment #13)
> > looking good.
> > 
> > What's going to happen on selection+copy? Are we getting the full text of the
> > URL in the copy buffer or will it be truncated?
> 
> It should Just Work, but I can verify.

Copying now works properly.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 497192 [details] [diff] [review]
Proposed patch, part 1, version 3: Code changes.

Not a blocker, but generally looks straightforward to me.

Get a review from robc or mihai, and I'll rubberstamp this for landing.

Comment 23

[Mihai Sucan [:msucan]]

This patch depends on the patches from bug 605621.

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 497192 [details] [diff] [review]
Proposed patch, part 1, version 3: Code changes.

The patch seems fine, except the following:

+XPCOMUtils.defineLazyServiceGetter(this, "ioService",
+                                   "@mozilla.org/network/io-service;1",
+                                   "nsIIOService");

This is not needed. The new code does not use ioService.

In test files:

+    findLogEntry("test-network.html");

findLogEntry is undefined. Tests fail.

I believe these are remnants of older patch iterations. Once these are fixed, f+.

Additional nitpick: from the toolbar, I cannot filter output by status. For example, if I write down 404 or 304, I cannot see the requests that have 404 response status. I think this happens because the filter code uses textContent, and the status uses a label with no text content. Not sure if you can do something about this now, but maybe it's worth a follow up bug report?

Comment 25

[Patrick Walton (:pcwalton)]

(In reply to comment #24)
> Additional nitpick: from the toolbar, I cannot filter output by status. For
> example, if I write down 404 or 304, I cannot see the requests that have 404
> response status. I think this happens because the filter code uses textContent,
> and the status uses a label with no text content. Not sure if you can do
> something about this now, but maybe it's worth a follow up bug report?

Yeah, this will be tricky to fix now. Will file a followup once this lands.

Comment 26

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 4: Code changes.

Patch version 4 rebases and addresses feedback comments.

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 505277 [details] [diff] [review]
[checked-in] Proposed patch, version 4: Code changes.

Patch is fine, feedback+ it is.

Comment 28

[Kevin Dangoor]

Comment on attachment 505277 [details] [diff] [review]
[checked-in] Proposed patch, version 4: Code changes.

per comment 22, dolske will approve this after in-group review (which this has now had).

Requesting approval to land this in firefox 4. It makes the console output much more readable and without horizontal scrolling even for long URLs in network requests.

Comment 29

[Rob Campbell [:rc] (:robcee)]

ditto. Running with this now and it's a big usability win. Would like to land it when we get an a+.

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 505277 [details] [diff] [review]
[checked-in] Proposed patch, version 4: Code changes.

rs=me per comment 22, a+.

Comment 31

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 505277 [details] [diff] [review]
[checked-in] Proposed patch, version 4: Code changes.

http://hg.mozilla.org/mozilla-central/rev/812710794ca1

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

Verified fixed on 

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre