605621 - Web Console output box should be reskinned

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

Per Alex Faaborg's mockups in bug 599498, we need a "timeline" display for the Web Console output.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: WIP screenshot.

Here's a screenshot of the current work-in-progress.

Comment 2

[Patrick Walton (:pcwalton)]

We'll need a few icons to be designed for this:

 * a blue (rgb(0, 182, 240)) 8x8 "X"
 * an orange (rgb(251, 149, 0)) 8x8 "X"
 * an orange (rgb(251, 149, 0)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 X
 * a light gray (rgb(203, 203, 203)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 (i) info icon

We also may want a gray (#808080) 8x8 "left arrow" icon to represent user input, and a gray (#808080) 8x8 "right arrow" icon to represent the corresponding output.

See toolkit/themes/foostripe/global/icons/webconsole.png for placeholder icons.

Comment 3

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

Screenshot looks great, Patrick!

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1: Code changes.

Proposed patch, part 1 reroutes all console messages through a common output function (shades of bug 578658 here) and reskins the layout of the output. In particular, now we use xul:descriptions, ending the abuse of xul:label that existed previously.

Comment 8

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 2 removes a dump() that crept in there by mistake.

This patch may seem large, but on the plus side, it deletes significantly more code than it adds :)

Comment 9

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2: CSS changes.

Patch part 2 implements the mockup via CSS changes. Tested on all three platforms. (The CSS is essentially identical among platforms, except for the monospace font.)

Comment 10

[Patrick Walton (:pcwalton)]

Nominating for blocking status, as this blocks a betaN+ blocker.

Comment 11

[Mihai Sucan [:msucan]]

Patrick: these patches fail to apply cleanly. It was sufficiently easy to make the first patch to apply, but the second one requires more work.

What shall I do?

(tried applying the patches on top of a clean m-c default branch)

Comment 12

[Patrick Walton (:pcwalton)]

Aha, I thought the "Depends on" field was properly set, but it wasn't :)

These patches depend on the patches from the following bugs, in this order:
* bug 607163 - webConsole.css -> browser.xul
* bug 601667 - toolbar styling

(As an aside, this is why I've been itching to land things... we're diverging too far from m-c for comfort.)

Comment 13

[Mihai Sucan [:msucan]]

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

Thanks Patrick! I imported the patches, and now they applied cleanly.

In the new adjustGroupVisibility() method: please use "}\nelse{" instead of "} else {", as the rest of the code does.

The warning of logWarningAboutReplacedAPI() doesn't show. I have a page which overwrites it, and the warning is not displayed. Still, the browser_warn_user_about_replaced_api.js test passes, which is weird. Please test and check what is going wrong.

In logWarningAboutReplacedAPI():
+    let hudBox = this.getHeadsUpDisplay(aHUDId);

We want to get rid of the getHeadsUpDisplay() method. Maybe you can use a hudWeakRef?

In logActivity(), again you use getHeadsUpDisplay().

Other thoughts: Why not merge the create/filter/outputMessageNode() methods into one call? I mean, when you have a message to log, you always want it created, filtered or shown.

Lastly, why call filterMessageNode() only in some places?

The change in browser_webconsole_bug_595934_message_categories.js:

-    matchString: "malformed-xml",
+    matchString: "malformedxml.xhtml",

This basically means it will also catch the network message about malformedxml.xhtml being loaded. I want to be sure the category name is found, or something from the error/warning message. (see how performTest() works)


Giving this patch feedback-, until the above comments are addressed. The patch looks fine otherwise. I am glad to see such cleanup in the HUDService.

Comment 14

[Mihai Sucan [:msucan]]

Comment on attachment 489382 [details] [diff] [review]
Proposed patch, part 2: CSS changes.

This looks fine to me.

One comment: I am not sure if it's nice from the UI perspective to have both a vertical rectangle with a nice color *and* a colored shape. These waste too much horizontal space. Why not just the shape? Or just the rectangle?

Comment 15

[Patrick Walton (:pcwalton)]

(In reply to comment #14)
> Comment on attachment 489382 [details] [diff] [review]
> Proposed patch, part 2: CSS changes.
> 
> This looks fine to me.
> 
> One comment: I am not sure if it's nice from the UI perspective to have both a
> vertical rectangle with a nice color *and* a colored shape. These waste too
> much horizontal space. Why not just the shape? Or just the rectangle?

Best to ask Faaborg.

Comment 16

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 3 addresses feedback. After making the fixes, I was unable to reproduce the problem with the warning disappearing.

Comment 17

[Mihai Sucan [:msucan]]

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

Yes, the warning now shows up. In logWarningAboutReplacedAPI():

+    let hudBox = this.hudWeakReferences[aHUDId].get().HUDBox;
+    let outputNode = hudBox.querySelector(".hud-output-node");

You can directly use .outputNode instead of .HUDBox.

The patch looks fine, but for some reason the code is now hit by bug 580618. Meaning, messages stop showing up after a minute of user inactivity - hud weakrefs are GCed. Ugh...

At this point you either go back to the semi-deprecated getHeadsUpDisplay() method, or we fix hud weakrefs. I'd prefer the latter option, because it's technically more sound, but we've been postponing the fix for quite some time already. I leave the choice to you. We shall talk about this in today's meeting.

Comment 18

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 4 addresses feedback comments. Review requested.

Comment 19

[Patrick Walton (:pcwalton)]

Comment on attachment 489382 [details] [diff] [review]
Proposed patch, part 2: CSS changes.

Obsoleting these patches for now. I'm going to go ahead and pull bug 588379 (source file links/styling) into this one, since it seems to me that we should consider the way links look as part of the overall appearance.

Comment 20

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 5 adds view-source links to the error messages.

Comment 21

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2: CSS changes.

Proposed patch, part 2, version 2 contains the CSS changes, with support for the view-source links as well as RTL support.

Comment 22

[Mihai Sucan [:msucan]]

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

The patch looks fine, but:

- it's still broken. The use of hudWeakRefs makes the Web Console unusable. Please rebase on top of bug 580618. This patch cannot be used like this.

- in adjustGroupVisibility():

+      let sel = "*:not(.hud-filtered-by-string):not(.hud-filtered-by-type)";
+      if (group.querySelector(sel)) {

According to the coding style guidelines, we shouldn't use such short variable names. "let sel" alone is kind of cryptic. Please use "let selector".

- in adjustVisibilityForMessageType():

+        groups.push(node.parentNode);

Doesn't this push the same group multiple times?

if (groups.indexOf(node.parentNode) == -1) groups.push(node.parentNode);

- in adjustVisibilityOnSearchStringChange() same as above.

- reportPageError() ... hmm, makes me want this patch rebased on top of bug 597136. Once that lands, reportPageError() is no longer needed.

+    let lineNumber = aScriptError.lineNumber;
+    if (lineNumber === 0) {
+      lineNumber = null;
+    }

Just pass the lineNumber as-is to createMessageNode(). It's awkward to change the number to null, if it's 0. In createMessageNode() do if (lineNumber) ... show the line number, otherwise not. I believe line numbers do not start from 0. Please let me know if I am wrong.

- You use strict equality comparison. No clear benefit, and the rest of the code doesn't use strict equality comparison.

- In filterMessageNode():

+    let prefKey = MESSAGE_PREFERENCE_KEYS[aNode.category][aNode.severity];
+    if (prefKey !== null && !HUDService.getFilterState(aHUDId, prefKey)) {

Why check against null? if (prefKey && ...) { ... }.

- In browser_webconsole_bug_605621_view_source.js: you perform the mouse click, then use executeSoon(). This can fail. You should wait for the click event to be fired. executeSoon() is entirely async from the click event, and the current solution can fail.

- Also you do:

+    let wnd = aSubject.QueryInterface(Ci.nsIDOMWindow);
+
+    ok(true, "the view source window was opened in response to clicking " +
+       "the location node");

Why not do ok(wnd, ...) ?


That is all! Giving it feedback+, with the above comments addressed.

Comment 23

[Mihai Sucan [:msucan]]

Comment on attachment 490276 [details] [diff] [review]
Proposed patch, part 2, version 2: CSS changes.

Feedback+ for this patch as well.

I am not sure about the performance of some CSS rules you used: ".webconsole-msg-console.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon". This, for example, looks rather unoptimized. Maybe you can simplify these selectors.

Comment 25

[Neil Deakin]

Just as a note:

+    let wnd = aSubject.QueryInterface(Ci.nsIDOMWindow);

This line is equivalent to let wnd = aSubject;

as QI isn't needed for dom objects.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm not sure that we need to block on all dependencies, but we need a better skinning for ship. It would be nice if this bug could be updated with a clear list of what MUST happen as opposed to what would SHOULD happen ;)

Comment 27

[Kevin Dangoor]

I'm removing bug 611795 from the depends on list because that bug is not one that we must have to ship. I think bug 601667 is in the depends list because this patches here likely depend on those patches (but that's fine anyhow, because the toolbar visual improvement is just as important as this one).

It sounds like bug 611506 is not required either, but Patrick would know better about that one.

Comment 28

[Patrick Walton (:pcwalton)]

(In reply to comment #26)
> I'm not sure that we need to block on all dependencies, but we need a better
> skinning for ship. It would be nice if this bug could be updated with a clear
> list of what MUST happen as opposed to what would SHOULD happen ;)

The executive summary is that the UX team wants it to look like this: https://bug599498.bugzilla.mozilla.org/attachment.cgi?id=481368

So if we assume that this look is needed, then this is my take on what MUST happen:

 * All messages must be routed through a common styling and display function. Currently, we have a variety of paths that manually construct messages from DOM nodes and wedge them into the output in different ways. This produces visual inconsistencies that make the console output below the level of polish we need to ship Firefox 4.
 * That common styling function must produce timestamps formatted as shown in the mockup and produce the vertical "timeline" display, including the use of color and shape to convey importance.
 * Errors must contain links to the source file and line number in which they occurred.
 * Gaps in the timeline must be shown when significant time has passed since the last message. (We've had code to do this on trunk for some time now, it was just flaky before - so this part is more of a bug fix than a new feature.)

All of the above are fixed by the patches attached to this bug.

What SHOULD happen:

 * Long URLs should be abbreviated, to prevent horizontal scroll bars from appearing (bug 611440).
 * Repeated messages in the Web Console should be collapsed into one (bug 611795). WebKit's developer console does this.
 * The splitter should be restyled (bug 611851). This is particularly nice to have on the Mac.
 * The Web Console code should be cleaned up in the interest of future maintainability as much as is reasonable from a risk vs. reward standpoint before Firefox 4. This includes bug 611506 as well as the other dependencies of bug 592463.

Comment 29

[Patrick Walton (:pcwalton)]

(In reply to comment #22)
> - in adjustVisibilityForMessageType():
> 
> +        groups.push(node.parentNode);
> 
> Doesn't this push the same group multiple times?
> 
> if (groups.indexOf(node.parentNode) == -1) groups.push(node.parentNode);
> 
> - in adjustVisibilityOnSearchStringChange() same as above.

Yeah, it can, but it doesn't seem to hurt, and searching for duplicates might end up being slower than just processing the same groups.

Comment 30

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 6 rebases to trunk and addresses feedback. I'm now building up single class names for the icons so that the CSS is simpler.

Comment 31

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3: CSS changes.

Proposed patch, part 2, version 3 addresses feedback. Review requested.

Comment 32

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 7 rebases to trunk.

Comment 34

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 35

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 36

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 37

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 8 brings the code up to date with the changes made in bug 601667 (i.e. it adds support for both CSS errors and CSS warnings).

Comment 38

[Patrick Walton (:pcwalton)]

Note that we now need a CSS warning icon. This brings the list of needed icons to the following:

 * a blue (rgb(0, 182, 240)) 8x8 "X"
 * a blue (rgb(0, 182, 240)) 8x8 "/!\" warning triangle
 * an orange (rgb(251, 149, 0)) 8x8 "X"
 * an orange (rgb(251, 149, 0)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 X
 * a light gray (rgb(203, 203, 203)) 8x8 /!\ warning triangle
 * a light gray (rgb(203, 203, 203)) 8x8 (i) info icon

Comment 39

[Patrick Walton (:pcwalton)]

These patches, and the CSS, have bitrotted.

Comment 40

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 9 rebases to trunk and fixes the styling on errors produced by user input at the command line.

Comment 41

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 4: CSS changes.

Proposed patch, part 2, version 4 rebases to trunk and adds styling for CSS warnings.

Comment 42

[Mihai Sucan [:msucan]]

Patrick: with this patch applied...

1. Network errors and info are not different. When I disable Network > Log I see all network messages disappear. When I disable Network > Error, the same happens.

I would expect network > error to toggle only the messages that show network errors. Similarly, with the network > log checkbox.

2. CSS > errors does nothing. CSS messages are all considered to be warnings. Why show the two options, then? afaik, it was agreed to consider all css messages as warnings.

3. Font size is too small for me. Could you please increase it a bit? I am not asking this only for me, I am quite certain there are (many) other web devs / designers who would feel the need to increase the font size, to improve readability.

Thanks!

Comment 43

[Patrick Walton (:pcwalton)]

(In reply to comment #42)
> 2. CSS > errors does nothing. CSS messages are all considered to be warnings.
> Why show the two options, then? afaik, it was agreed to consider all css
> messages as warnings.

The CSS loader can emit errors. See: http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#948

Comment 44

[David Dahl :ddahl]

(In reply to comment #43)
> (In reply to comment #42)
> > 2. CSS > errors does nothing. CSS messages are all considered to be warnings.
> > Why show the two options, then? afaik, it was agreed to consider all css
> > messages as warnings.
> 
> The CSS loader can emit errors. See:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#948

It looks like the category "CSS Loader" is not handled by our code, see:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3133

Do we have a bug for that?

Comment 45

[Mihai Sucan [:msucan]]

(In reply to comment #43)
> (In reply to comment #42)
> > 2. CSS > errors does nothing. CSS messages are all considered to be warnings.
> > Why show the two options, then? afaik, it was agreed to consider all css
> > messages as warnings.
> 
> The CSS loader can emit errors. See:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#948

Ah, yeah, good point.

(In reply to comment #44)
> It looks like the category "CSS Loader" is not handled by our code, see:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3133
> 
> Do we have a bug for that?

We have a patch for that in bug 606498.

Comment 46

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 10 addresses feedback.

Comment 47

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 5: CSS changes.

Proposed patch, part 2, version 5 addresses feedback comments.

Comment 48

[Mihai Sucan [:msucan]]

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

Thanks for your updates!

The Network > Errors checkbox now works - it hides network errors as expected. However, Network > Log disables network errors *as well*. Given it's a checkbox, I would expect errors to still be visible. Use case: maybe there are lots of requests that are successful and I want to see only the failed ones.

Giving the patch f+ with the above fixed.

Comment 49

[Mihai Sucan [:msucan]]

Comment on attachment 495581 [details] [diff] [review]
Proposed patch, part 2, version 5: CSS changes.

Thanks for the font size increase. It really makes a difference in readability.

Shouldn't there be a visual difference between network errors and normal network logging? Shouldn't errors have a black X icon? Also, perhaps a red line? Not sure, but at least the icon, or some other indication that the failed request is different from the others.

Comment 50

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 10 attached. It replaces the output node with a richlistbox. This makes selection and copying work again (and it's much simpler!)

Comment 51

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 10 to version 11.

Here's an interdiff between version 10 and version 11, for Mihai.

Comment 52

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 6: CSS changes.

Patch part 2, version 6 cleans up the CSS and updates it to apply to the richtextbox.

Comment 53

[Mihai Sucan [:msucan]]

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

Interesting changes. For one, I am really glad to see the maintainScrollPosition() and liftNode() methods gone, but the switch to the richlist is weird a bit. It's a weird change in behavior, especially when you try to select the lines.

Comments:

- I believe you forgot to address the feedback in comment #48: the network > log checkbox still hides all network requests from output, including errors. Network errors should stay visible.

- The text that is copied from the output does not include the network status. In the network logging code please update the clipboardText, to include the status and timing info as well.

- Errors do not have the source URL and line number when copied to clipboard. Please include these.


Code comments:

- In regroupOutput(), please follow the source code style:

+      } else {
+        nodes[i].classList.remove("webconsole-new-group");
+      }

That should be "}\nelse {".

- In logNetActivity():

+    let msgNode = chromeDocument.createElementNS(HTML_NS, "html:span");

Why use an html:span over a xul:label? I don't have anything against that but I believe Enn might, and we are 100% XUL, except these two spans you add in logNetActivity(). If we are going to go mixed, then no problem, but we should make up our mind. You may ignore this point, if you want.


- The createMessageNode() method takes aClipboardText as the last argument, but it's never used. The aBody argument takes a string or an object which can hold the node string and clipboard text.

I would suggest making it less confusing. Keep aBody a message or a node, and use the new aClipboardText argument. That's all.

- In browser_webconsole_bug_601177_log_levels.js:

-  ok(findEntry("hud-info", "test-image.png"),
+  ok(findEntry("hud-networkinfo", "test-image.png"),
     "found test-image.png");

-  ok(findEntry("hud-error", "foobar-known-to-fail.png"),
+  ok(findEntry("hud-network", "foobar-known-to-fail.png"),
     "found foobar-known-to-fail.png");

That is a bit confusing. Why does the network error item has the .hud-network class, while the other successful request has the .hud-networkinfo class name? The check I did was precisely to see if there's an error-specific class name being used for the network error request. I wanted to see if the severity is correctly assigned.


Giving the patch f- this time, because I'd like to see the problems above addressed. Otherwise, the patch is really great!

(Thanks for the interdiff - it was a real time-saver!)

Comment 54

[Mihai Sucan [:msucan]]

Comment on attachment 496392 [details] [diff] [review]
Proposed patch, part 2, version 6: CSS changes.

I believe you forgot to address the change I asked in comment #49: please make a visual difference between successful and failed network requests.


+.webconsole-msg-body {
+  margin-top: 0;
+  margin-bottom: 0;
+  -moz-margin-start: 3px;
+  -moz-margin-end: 6px;
+  padding: 2px 0 0px;
 }

I think padding can be: 2px 0 0. (without the px)

+.webconsole-msg-cssparser > .webconsole-marker {
+  -moz-border-start: solid rgb(0, 182, 240) 6px;
+}

There's no consistency: sometimes colors are given as keywords (eg. GrayText), or hex hashes (eg. #000), or rgb() values. Why not use a consistent approach of giving color values?


The font size on gnomestripe is 12px, while on pinstripe and winstripe it's 11px, is this difference fine? Shouldn't it be the same (12px) for all themes?

On Windows you chose the font "Lucida Console" as the first preferred font. Why not add Consolas? I think it would be good to add it as the first font of choice on Windows machines.

Giving the patch f+, with the above comments addressed.

Comment 55

[Patrick Walton (:pcwalton)]

(In reply to comment #53)
> Why use an html:span over a xul:label? I don't have anything against that but I
> believe Enn might, and we are 100% XUL, except these two spans you add in
> logNetActivity(). If we are going to go mixed, then no problem, but we should
> make up our mind. You may ignore this point, if you want.

Actually, I tried that first, and IIRC it didn't wrap properly. IIUC a <description> is designed to hold HTML.

Comment 56

[Patrick Walton (:pcwalton)]

(In reply to comment #53)
> Comment on attachment 496355 [details] [diff] [review]
> Proposed patch, part 1, version 11: Code changes.
> 
> Interesting changes. For one, I am really glad to see the
> maintainScrollPosition() and liftNode() methods gone, but the switch to the
> richlist is weird a bit. It's a weird change in behavior, especially when you
> try to select the lines.

Filed bug 617915 on this. I think that overall it's a usability win, but we should see about getting the problem of multiple selection in richlistboxes fixed at the platform level.

Comment 57

[Philip Chee]

> Actually, I tried that first, and IIRC it didn't wrap properly. IIUC a
> <description> is designed to hold HTML.

Hmm. Doesn't label.textContent = "foobar" wrap properly?

As seen in mozilla.dev.tech.xul:

var butn = document.createElement("button");
butn.setAttribute("style", "white-space: pre-wrap; text-align: center;");
butn.textContent="It\r\nWorks!\r\n\r\nThanks for the point\r\nin the right direction.";

Comment 58

[Mihai Sucan [:msucan]]

(In reply to comment #57)
> > Actually, I tried that first, and IIRC it didn't wrap properly. IIUC a
> > <description> is designed to hold HTML.
> 
> Hmm. Doesn't label.textContent = "foobar" wrap properly?
> 
> As seen in mozilla.dev.tech.xul:
> 
> var butn = document.createElement("button");
> butn.setAttribute("style", "white-space: pre-wrap; text-align: center;");
> butn.textContent="It\r\nWorks!\r\n\r\nThanks for the point\r\nin the right
> direction.";

The main point was for that code to be XUL, not specifically a xul:label. I just think we should be consistent in our UI choices. It can be a label, a description, or something else.

Comment 59

[Neil Deakin]

What exactly are you trying to do here?

<label> should be used for elements that label another control.
<description> should be used for other text.

The <label value=""> form should be used for single-line non-wrapping text with only one style.
The <label></label> form should be used for wrapping or styled text.
<description> works identically in this regard.

<xul:description> is display: block and is laid out identical to <html:div>.

Comment 60

[Patrick Walton (:pcwalton)]

(In reply to comment #59)
> What exactly are you trying to do here?

Place a link inside the body of a <description>, in this case.

Comment 61

[Patrick Walton (:pcwalton)]

(In reply to comment #53)
> Comment on attachment 496355 [details] [diff] [review]
> Proposed patch, part 1, version 11: Code changes.
> 
> Interesting changes. For one, I am really glad to see the
> maintainScrollPosition() and liftNode() methods gone, but the switch to the
> richlist is weird a bit. It's a weird change in behavior, especially when you
> try to select the lines.
> 
> Comments:
> 
> - I believe you forgot to address the feedback in comment #48: the network >
> log checkbox still hides all network requests from output, including errors.
> Network errors should stay visible.
> 
> - The text that is copied from the output does not include the network status.
> In the network logging code please update the clipboardText, to include the
> status and timing info as well.
> 
> - Errors do not have the source URL and line number when copied to clipboard.
> Please include these.
> 
> 
> Code comments:
> 
> - In regroupOutput(), please follow the source code style:
> 
> +      } else {
> +        nodes[i].classList.remove("webconsole-new-group");
> +      }
> 
> That should be "}\nelse {".
> 
> - In logNetActivity():
> 
> +    let msgNode = chromeDocument.createElementNS(HTML_NS, "html:span");
> 
> Why use an html:span over a xul:label? I don't have anything against that but I
> believe Enn might, and we are 100% XUL, except these two spans you add in
> logNetActivity(). If we are going to go mixed, then no problem, but we should
> make up our mind. You may ignore this point, if you want.
> 
> 
> - The createMessageNode() method takes aClipboardText as the last argument, but
> it's never used. The aBody argument takes a string or an object which can hold
> the node string and clipboard text.
> 
> I would suggest making it less confusing. Keep aBody a message or a node, and
> use the new aClipboardText argument. That's all.
> 
> - In browser_webconsole_bug_601177_log_levels.js:
> 
> -  ok(findEntry("hud-info", "test-image.png"),
> +  ok(findEntry("hud-networkinfo", "test-image.png"),
>      "found test-image.png");
> 
> -  ok(findEntry("hud-error", "foobar-known-to-fail.png"),
> +  ok(findEntry("hud-network", "foobar-known-to-fail.png"),
>      "found foobar-known-to-fail.png");
> 
> That is a bit confusing. Why does the network error item has the .hud-network
> class, while the other successful request has the .hud-networkinfo class name?
> The check I did was precisely to see if there's an error-specific class name
> being used for the network error request. I wanted to see if the severity is
> correctly assigned.

"hud-network" is the error class, while "hud-networkinfo" is the non-error class. See the MESSAGE_PREFERENCE_KEYS table.

It's very confusing, the result of my trying to keep the size of this patch down (by minimizing test breakage) and to avoid changing the preference keys, so the users' preferences persist across versions. If we're willing to break preferences, then we could just eliminate the whole table. Probably worth filing a followup bug on?

As an aside, CATEGORY_CLASSES and SEVERITY_CLASSES only exist to try to keep the size of the patch down by avoiding changing the ".jsterm-" classes in the CSS. They could also be eliminated in the future in favor of the *_FRAGMENTS constants.

Comment 62

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 12 addresses feedback comments.

Comment 63

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 7: CSS changes.

Patch part 2, version 7 fixes the CSS to correspond to the changes in version 12 of the code changes, and cleans it up a bit. Feedback re-requested due to the large changes.

Comment 64

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 13 adds more comments.

Comment 65

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 12 to version 13.

Interdiff between versions 12 and 13 attached.

Comment 66

[Shawn Wilsher :sdwilsh]

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

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+// The various categories of messages.
>+const CATEGORY_NETWORK = 0;
>+const CATEGORY_CSS = 1;
>+const CATEGORY_JS = 2;
>+const CATEGORY_WEBDEV = 3;
>+const CATEGORY_MISC = 4;    // always on
>+const CATEGORY_INPUT = 5;   // always on
>+const CATEGORY_OUTPUT = 6;  // always on
>+
>+// The possible message severities.
>+const SEVERITY_ERROR = 0;
>+const SEVERITY_WARNING = 1;
>+const SEVERITY_INFO = 2;
>+const SEVERITY_LOG = 3;
I would prefer you to start the numbering of constants at one, not zero, for the reasons described in Code Complete (2nd Edition), Section 12.6 (even if this file didn't always do that before)

>+// The fragment of a CSS class name that identifies each category.
>+const CATEGORY_CLASS_FRAGMENTS = [
>+  "network",
>+  "cssparser",
>+  "exception",
>+  "console",
>+  "misc",
>+  "input",
>+  "output"
>+];
trailing comma on the last entry please

>+// The fragment of a CSS class name that identifies each severity.
>+const SEVERITY_CLASS_FRAGMENTS = [
>+  "error",
>+  "warn",
>+  "info",
>+  "log"
>+];
Ditto

>+// The preference keys to use for each category/severity combination, indexed
>+// first by category (rows) and then by severity (columns).
>+//
>+// Most of these rather idiosyncratic names are historical and predate the
>+// division of message type into "category" and "severity".
>+const MESSAGE_PREFERENCE_KEYS = [
>+//  Error         Warning   Info    Log
>+  [ "network",    null,         null,   "networkinfo" ],  // Network
>+  [ "csserror",   "cssparser",  null,   null          ],  // CSS
>+  [ "exception",  "jswarn",     null,   null          ],  // JS
>+  [ "error",      "warn",       "info", "log"         ],  // Web Developer
>+  [ null,         null,         null,   null          ],  // Misc.
>+  [ null,         null,         null,   null          ],  // Input
>+  [ null,         null,         null,   null          ]   // Output
>+];
Ditto

>- * @returns void
>+ * @returns number
>+ *        The current user-selected log limit.
Oof.  This is supposed to be @return (not plural)

>+    for (let i = 0; i < nodes.length; i++) {
>+      let thisTimestamp = nodes[i].timestamp;
I'd actual prefer it if you used nodes.forEach whenever you iterate an array but don't care about the index you are at.  You don't have to fix this, but try to keep this in mind with future work.

>@@ -1879,61 +1861,44 @@ HUD_SERVICE.prototype =
>    * @param nsIDOMNode aMessageNode
>    *        The message DOM Node that will be appended to aConsoleNode
>    * @returns void
>    */
>   logHUDMessage: function HS_logHUDMessage(aMessage,
>                                            aConsoleNode,
>                                            aMessageNode)
>   {
>+    ConsoleUtils.outputMessageNode(aMessageNode, aMessage.hudId);
aConsoleNode is no longer used.  Please remove it.

>   logConsoleAPIMessage: function HS_logConsoleAPIMessage(aHudId,
>                                                          aLevel,
>                                                          aArguments)
Since you are touching this method, please update it's java-doc comments as they don't really indicate what the arguments are for.

>   {
>+    const LEVELS = {
>+      error: SEVERITY_ERROR,
>+      warn: SEVERITY_WARNING,
>+      info: SEVERITY_INFO,
>+      log: SEVERITY_LOG
>     };
any reason these were not pulled out into constants in the first place and then just pass a constant into this method?

>   logWarningAboutReplacedAPI:
>   function HS_logWarningAboutReplacedAPI(aHUDId)
Same with the comments on this method

>+  /**
>+   * Reports an error in the page source, either JavaScript or CSS.
>+   *
>+   * @param number aCategory
>+   *        The category of the message; either CATEGORY_CSS or CATEGORY_JS.
Given this, I think you should throw an error when aCategory is not one of these types.

>+   * @param nsIScriptError aScriptError
>+   *        The error message to report.
>+   */
>+  reportPageError: function HS_reportPageError(aCategory, aScriptError)
>+  {
>+    let severity = (aScriptError.flags & aScriptError.warningFlag) ||
>+                   (aScriptError.flags & aScriptError.strictFlag)
>+                   ? SEVERITY_WARNING
>+                   : SEVERITY_ERROR;
I realize that you are running into line length issues here, but this is difficult to read.  Might I suggest:
>+    let severity = SEVERITY_ERROR;
>+    if (aScriptError.flags & aScriptError.warningFlag ||
>+        aScriptError.flags & aScriptError.strictFlag) {
>+      severity = SEVERITY_WARNING;
>+    }

>+                if (status >= 400 && status < 600) {
>+                  ConsoleUtils.setMessageType(msgObject.messageNode,
>+                                              CATEGORY_NETWORK,
>+                                              SEVERITY_ERROR);
I think I'd prefer the 400 and 600 to be defined as constants up top

>+  copySelectedItems: function HS_copySelectedItems(aOutputNode)
>+  {
>+    let strings = [];
>+    for (let i = 0; i < aOutputNode.selectedCount; i++) {
>+      let item = aOutputNode.selectedItems[i];
>+      if (i > 0 && item.classList.contains("webconsole-new-group")) {
>+        strings.push("\n");
>+      }
comment here why this is done please

>  * JSTermFirefoxMixin
>  *
>  * JavaScript Terminal Firefox Mixin
>  *
>  */
> function
> JSTermFirefoxMixin(aContext,
>                    aParentNode,
>-                   aExistingConsole,
>-                   aCSSClassOverride)
>+                   aExistingConsole)
Add better comments to this method please (java-doc ones)

>+   * Given a category and message body, creates a DOM node to represent an
>+   * incoming message. The timestamp is automatically added.
>+   *
>+   * @param nsIDOMDocument aDocument
>+   *        The document in which to create the node.
>+   * @param number aCategory
>+   *        The category of the message: one of the CATEGORY_* constants.
>+   * @param number aSeverity
>+   *        The severity of the message: one of the SEVERITY_* constants;
>+   * @param string|nsIDOMNode aBody
>+   *        The body of the message, either a simple string or a DOM node.
>+   * @param string? aSourceURL
>+   *        The URL of the source file that emitted the error. Optional.
>+   * @param number? aSourceLine
>+   *        The line number on which the error occurred. If zero or omitted,
>+   *        there is no line number associated with this message.
>+   * @param string? aClipboardText
>+   *        The text that should be copied to the clipboard when this node is
>+   *        copied. If omitted, defaults to the body text. If `aBody` is not
>+   *        a string, then the clipboard text must be supplied.
>+   * @returns nsIDOMNode
>+   */
Elsewhere we've used [optional] and not a question mark after the type.  I wasn't sure what the question mark was for at first, to be honest.

You should also throw an error in the case where aClipboardText is undefined and aBody is not a string in this method.

Lastly, elaborate more in the @return statement

>+    if (!(aBody instanceof Ci.nsIDOMNode)) {
>+      if (!aClipboardText) {
>+        aClipboardText = aBody;
>+        if (aSourceURL) {
>+          aClipboardText += " @ " + aSourceURL;
>+          if (aSourceLine) {
>+            aClipboardText += ":" + aSourceLine;
>+          }
>+        }
>+      }
>+
>+      aBody = aDocument.createTextNode(aBody);
>+    }
Let's try to make this a little less indented and more compact.  How about this:
      aClipboardText = aClipboardText ||
                      (aBody + (aSourceUrl ? " @ " + aSourceURL : "") +
                       (aSourceLine ? ":" + aSourceLine : ""));
      aBody = aBody instanceof Ci.nsIDOMNode ?
        aBody : aDocument.createTextNode(aBody);

>+  /**
>+   * Adjusts the category and severity of the given message, clearing the old
>+   * category and severity if present.
>+   *
>+   * @param nsIDOMNode aMessageNode
>+   *        The message node to alter.
>+   * @param number aNewCategory
>+   *        The new category for the message; one of the CATEGORY_ constants.
>+   * @param number aNewSeverity
>+   *        The new severity for the message; one of the SEVERITY_ constants.
>+   */
missing @return

>+  setMessageType:
>+  function ConsoleUtils_setMessageType(aMessageNode, aNewCategory,
>+                                       aNewSeverity) {
>+    if ('category' in aMessageNode) {
nit: use " instead of '

>+  /**
>+   * Creates the XUL label that displays the textual location of an incoming
>+   * message.
>+   *
>+   * @param nsIDOMDocument aDocument
>+   *        The document in which to create the node.
>+   * @param string aSourceURL
>+   *        The URL of the source file responsible for the error.
>+   * @param number? aSourceLine
>+   *        The line number on which the error occurred. If zero or omitted,
>+   *        there is no line number associated with this message.
>+   */
missing @return

>+  /**
>+   * Abbreviates the given source URL so that it can be displayed flush-right
>+   * without being too distracting.
>+   *
>+   * @param string aSourceURL
>+   *        The source URL to shorten.
>+   * @returns string
>+   */
elaborate on the @return please

>   isCommandEnabled: function CommandController_isCommandEnabled(aCommand)
>   {
>+    let outputNode = this._getFocusedOutputNode();
>+    if (!outputNode) {
>+      return false;
>+    }
>+
>+    switch (aCommand) {
>+      case "cmd_copy":
>+        return outputNode.selectedCount > 0;
>+      case "cmd_selectAll":
>+        return true;
>+    }
Shouldn't you also add a test to make sure that cmd_copy works as expected?

>+      case "CSS Parser":
>+      case "CSS Loader":
>+        HUDService.reportPageError(CATEGORY_CSS, aSubject);
>+        return;
Are there tests for these?

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_580400_groups.js
>@@ -55,29 +55,32 @@ function testGroups() {
>   let hudId = HUDService.displaysIndex()[0];
> 
>   let HUD = HUDService.hudReferences[hudId];
>   let jsterm = HUD.jsterm;
>   let outputNode = jsterm.outputNode;
> 
>   let timestamp0 = Date.now();
>   jsterm.execute("0");
>-  is(outputNode.querySelectorAll(".hud-group").length, 1,
>+  is(outputNode.querySelectorAll(".webconsole-new-group").length, 0,
>     "one group exists after the first console message");
> 
>   jsterm.execute("1");
>   let timestamp1 = Date.now();
>   if (timestamp1 - timestamp0 < 5000) {
>-    is(outputNode.querySelectorAll(".hud-group").length, 1,
>+    is(outputNode.querySelectorAll(".webconsole-new-group").length, 0,
>       "only one group still exists after the second console message");
Theses two messages do not line up with what is actually being tested anymore.

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_586142_insert_newlines.js
>+// Tests that copying multiple messages inserts newlines in between.
Oh, I see; copying tests are here (and in other files).  Can you hg rename the file (I'm fine with losing the bug number in it) to something more accurate then please?

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_601177_log_levels.js
>     browser.addEventListener("load", function(aEvent) {
>       browser.removeEventListener(aEvent.type, arguments.callee, true);
>-      executeSoon(onContentLoaded);
>+      //executeSoon(onContentLoaded);
>+      setTimeout(onContentLoaded, 1000);
er?

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_605621_view_source.js
note: you do not need to include bug numbers in the file names

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_js_input_and_output_styling.js
>+  ok(jsInputNode.classList.contains("webconsole-msg-input"),
>+    "JS input node is of the CSS class 'webconsole-msg-input'");
nit: " should line up after ( on the next line [applies globally]

>+++ b/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties
>@@ -1,19 +1,16 @@
> typeError=Error: 
> typeWarning=Warning: 
> typeNetwork=Network: 
> typeException=Exception:  
> typeCssParser=CSS Parser: 
> typeStrict=Strict Warning: 
> msgCategory=Category: 
>-errFile=Source File: %S
> errLine=Line: %S
>-errLineCol=Line: %S, Column: %S
>-errCode=Source Code:
fiddlesticks.  This should have had [strings] in the whiteboard.  Ping Axel (Pike on irc).  You may need to leave these in there for now.

I want to look at the interdiff with comments addressed.  Otherwise, this looks great.

Comment 67

[Axel Hecht [:Pike]]

String removals are a OK, at least as long as we're not doing any branches for a beta. The sooner the better.

Comment 68

[Patrick Walton (:pcwalton)]

(In reply to comment #66)
> I would prefer you to start the numbering of constants at one, not zero, for
> the reasons described in Code Complete (2nd Edition), Section 12.6 (even if
> this file didn't always do that before)

Well, I started them at zero so they could index into the MESSAGE_PREFERENCE_KEYS matrix. Should I make them start at 1 and subtract 1 each time or leave them starting at 0?

> any reason these were not pulled out into constants in the first place and then
> just pass a constant into this method?

The levels come from the lazy console service, which lives elsewhere. I wanted to minimize the amount of code touched by this file.

> Let's try to make this a little less indented and more compact.  How about
> this:
>       aClipboardText = aClipboardText ||
>                       (aBody + (aSourceUrl ? " @ " + aSourceURL : "") +
>                        (aSourceLine ? ":" + aSourceLine : ""));
>       aBody = aBody instanceof Ci.nsIDOMNode ?
>         aBody : aDocument.createTextNode(aBody);

I changed it to this, in order to add the parameter checking; is this fine?

    // If a string was supplied for the body, turn it into a DOM node and an
    // associated clipboard string now.
    if (!(aBody instanceof Ci.nsIDOMNode)) {
      aClipboardText = aClipboardText ||
                       (aBody + (aSourceURL ? " @ " + aSourceURL : "") +
                                (aSourceLine ? ":" + aSourceLine : ""));
      aBody = aDocument.createTextNode(aBody);
    } else if (aClipboardText == null) {
      throw new Error("HUDService.createMessageNode(): DOM node supplied " +
                      "without any clipboard text");
    }

> >+      case "CSS Parser":
> >+      case "CSS Loader":
> >+        HUDService.reportPageError(CATEGORY_CSS, aSubject);
> >+        return;
> Are there tests for these?

browser_webconsole_bug_595934_message_categories.js tests for the CSS Loader errors, and browser_webconsole_bug_589162_css_filter.js tests for the CSS Parser errors.

Comment 69

[Patrick Walton (:pcwalton)]

(In reply to comment #66)
> >@@ -1879,61 +1861,44 @@ HUD_SERVICE.prototype =
> >    * @param nsIDOMNode aMessageNode
> >    *        The message DOM Node that will be appended to aConsoleNode
> >    * @returns void
> >    */
> >   logHUDMessage: function HS_logHUDMessage(aMessage,
> >                                            aConsoleNode,
> >                                            aMessageNode)
> >   {
> >+    ConsoleUtils.outputMessageNode(aMessageNode, aMessage.hudId);
> aConsoleNode is no longer used.  Please remove it.

I actually removed the whole method, since it's only used in one place and that place might as well call ConsoleUtils.outputMessageNode() directly.

Comment 70

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 13 to version 14.

Interdiff between patch part 1, version 13 and version 14 attached.

Comment 71

[Patrick Walton (:pcwalton)]

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

Proposed patch, part 1, version 14 addresses review feedback.

Comment 72

[Shawn Wilsher :sdwilsh]

(In reply to comment #68)
> Well, I started them at zero so they could index into the
> MESSAGE_PREFERENCE_KEYS matrix. Should I make them start at 1 and subtract 1
> each time or leave them starting at 0?
Oof.  For those I guess it makes sense to start at zero.  Add a comment to them explaining why you start at that please.

> I changed it to this, in order to add the parameter checking; is this fine?
> 
>     // If a string was supplied for the body, turn it into a DOM node and an
>     // associated clipboard string now.
>     if (!(aBody instanceof Ci.nsIDOMNode)) {
>       aClipboardText = aClipboardText ||
>                        (aBody + (aSourceURL ? " @ " + aSourceURL : "") +
>                                 (aSourceLine ? ":" + aSourceLine : ""));
>       aBody = aDocument.createTextNode(aBody);
>     } else if (aClipboardText == null) {
>       throw new Error("HUDService.createMessageNode(): DOM node supplied " +
>                       "without any clipboard text");
>     }
I think the throw should be right up at the top of the method.  No reason to do some work, only to realize you got bogus input.

Comment 73

[Patrick Walton (:pcwalton)]

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

Patch part 1 version 15 fixes those two issues.

Comment 74

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 14 to version 15.

Interdiff between versions 14 and 15 of part 1 attached.

Comment 75

[Patrick Walton (:pcwalton)]

Comment on attachment 496701 [details] [diff] [review]
Interdiff for patch part 1, version 14 to version 15.

That wasn't an interdiff.

Comment 76

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 14 to version 15.

Interdiff between versions 14 and 15 of patch part 1 attached.

Comment 77

[Mihai Sucan [:msucan]]

Comment on attachment 496575 [details] [diff] [review] (part 1, version 12):

- In setMessageType():
+    if ('category' in aMessageNode) {

Double quotes.

- You are overwriting the clipboard text when the network request status comes in. This means you lose the GET/POST (HTTP method) string shown before the URL. So, when I copy the output line I don't get the HTTP method. Please fix this.

Otherwise, you have addressed all the changes I requested. Thanks!

Comment on attachment 496688 [details] [diff] [review] (part 1, version 14):

+// The lowest HTTP response code (inclusive) that is considered an error.
+const MIN_ERROR_CODE = 400;
+// The highest HTTP response code (exclusive) that is considered an error.
+const MAX_ERROR_CODE = 600;

The min/max error code, as a constant name, doesn't specifically refer to the HTTP response code. I would prefer it to be MIN/MAX_HTTP_STATUS (or HTTP_ERROR).

Comment 78

[Mihai Sucan [:msucan]]

Comment on attachment 496579 [details] [diff] [review]
Proposed patch, part 2, version 7: CSS changes.

Good work! I like the improved code consistency, and the clear visual difference between failed and successful network requests.

Comment 79

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 16 addresses Mihai's comments.

Comment 80

[Shawn Wilsher :sdwilsh]

Comment on attachment 496687 [details] [diff] [review]
Interdiff for patch part 1, version 13 to version 14.

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>+    if (aCategory != CATEGORY_CSS && aCategory != CATEGORY_JS) {
>+      throw new Error("HUDService.reportPageError(): unsupported category " +
>+                      "(must be one of CSS or JS)");
>+    }
Also send the result code Cr.NS_ERROR_INVALID_ARG, and set the stack to the caller (error shows up at the call site instead of here).

>+    } else if (aClipboardText == null) {
>+      throw new Error("HUDService.createMessageNode(): DOM node supplied " +
>+                      "without any clipboard text");
Move this to the top of the method please, plus all the stuff I said above.

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_580400_groups.js
>   is(outputNode.querySelectorAll(".webconsole-new-group").length, 0,
>-    "one group exists after the first console message");
>+    "no group dividers exist after the first console message");
> 
>   jsterm.execute("1");
>   let timestamp1 = Date.now();
>   if (timestamp1 - timestamp0 < 5000) {
>     is(outputNode.querySelectorAll(".webconsole-new-group").length, 0,
>-      "only one group still exists after the second console message");
>+      "no group dividers exist after the second console message");
nit: one more space indent (should be after the (, not even with)

>   jsterm.execute("2");
>   is(outputNode.querySelectorAll(".webconsole-new-group").length, 1,
>-    "two groups exist after the third console message");
>+    "one group divider exists after the third console message");
nit: one more space indent (should be after the (, not even with)

Comment 81

[Shawn Wilsher :sdwilsh]

(In reply to comment #80)
> >+    } else if (aClipboardText == null) {
> >+      throw new Error("HUDService.createMessageNode(): DOM node supplied " +
> >+                      "without any clipboard text");
> Move this to the top of the method please, plus all the stuff I said above.
You moved it to the top in attachment 496687 [details] [diff] [review], but the other comments apply.

Comment 82

[Patrick Walton (:pcwalton)]

Attachment: Interdiff for patch part 1, version 15 to version 16.

Comment 83

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh, with previous comments addressed.

Comment 84

[Patrick Walton (:pcwalton)]

(In reply to comment #80)
> Comment on attachment 496687 [details] [diff] [review]
> Interdiff for patch part 1, version 13 to version 14.
> 
> >+++ b/toolkit/components/console/hudservice/HUDService.jsm
> >+    if (aCategory != CATEGORY_CSS && aCategory != CATEGORY_JS) {
> >+      throw new Error("HUDService.reportPageError(): unsupported category " +
> >+                      "(must be one of CSS or JS)");
> >+    }
> Also send the result code Cr.NS_ERROR_INVALID_ARG, and set the stack to the
> caller (error shows up at the call site instead of here).

How should I do that? I can't seem to find the pattern by searching MXR. Do you mean "throw Components.Exception(message, resultCode || Cr.NS_ERROR_INVALID_ARG);"?

Comment 85

[Shawn Wilsher :sdwilsh]

(In reply to comment #84)
> How should I do that? I can't seem to find the pattern by searching MXR. Do you
> mean "throw Components.Exception(message, resultCode ||
> Cr.NS_ERROR_INVALID_ARG);"?
The second and third arguments will set it:
https://developer.mozilla.org/en/Components.Exception

For an example, see NetUtil.jsm:
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#82

Comment 86

[Patrick Walton (:pcwalton)]

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

Patch part 1, version 17 addresses review comments.

Comment 87

[Patrick Walton (:pcwalton)]

Going to go ahead and request that part 1 be checked in before part 2 is reviewed. The styling isn't much worse than what we have now, and it'll minimize rebasing problems between now and when Dão gets to the review.

Comment 88

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 8: CSS changes.

Patch part 2, version 8 removes the "background-color: white" from the winstripe theme, which fixes high-contrast mode (bug 615791).

Comment 90

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 1, version 18: Code changes.

Patch part 1, version 18 updates to tip.

Comment 91

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

Comment on attachment 499397 [details] [diff] [review]
[checked-in] Proposed patch, part 1, version 18: Code changes.

http://hg.mozilla.org/mozilla-central/rev/a27d1a62bef3

Comment 92

[Dão Gottwald [:dao]]

Comment on attachment 497824 [details] [diff] [review]
Proposed patch, part 2, version 8: CSS changes.

>+.hud-output-node, .jsterm-input-node {

nit: line break after comma

>+  font: 12px DejaVu Sans Mono, monospace;

The syntax is broken here, needs quotes.

This patch adds a bunch of hardcoded color values, please use system colors instead.

Comment 94

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 9: CSS changes.

Patch part 2, version 9 addresses review comments. Some of the colors don't seem to have corresponding system colors: these are the color codes for Net, CSS, JS, and Web Developer, which were chosen to be "Firefoxy". The rest have been turned into system colors.

Comment 95

[Johnathan Nightingale [:johnath]]

Does this bug still need final icons? It's keyworded and whiteboarded to suggest as much. If so, let's get them from UX *today*. This patch needs to land asap, especially since it has strings impact.

Comment 96

[Dão Gottwald [:dao]]

The string changes have landed.

Comment 97

[Johnathan Nightingale [:johnath]]

(In reply to comment #96)
> The string changes have landed.

Ah, and were just removals, k.

Comment 98

[Stephen Horlander [:shorlander]]

(In reply to comment #95)
> Does this bug still need final icons? It's keyworded and whiteboarded to
> suggest as much. If so, let's get them from UX *today*. This patch needs to
> land asap, especially since it has strings impact.

I have updated icons and a patch I need to update for attachment 501904 [details] [diff] [review] over on bug 609372. It doesn't seem that they have to land together? Although my patch/icons could probably be rolled into this one.

Comment 99

[Dão Gottwald [:dao]]

(In reply to comment #98)
> Although my patch/icons could probably be rolled into this one.

Please don't.

Comment 100

[Patrick Walton (:pcwalton)]

This has to be a hard blocker, because it contains the solution for bug 613280, a hard blocker. We really need a review on this.

Comment 101

[Dão Gottwald [:dao]]

Comment on attachment 501904 [details] [diff] [review]
Proposed patch, part 2, version 9: CSS changes.

>diff --git a/toolkit/themes/gnomestripe/global/webConsole.css b/toolkit/themes/gnomestripe/global/webConsole.css
>--- a/toolkit/themes/gnomestripe/global/webConsole.css
>+++ b/toolkit/themes/gnomestripe/global/webConsole.css

>+.hud-msg-node {
>+  margin: 0;
>+  padding: 0;
>+}

richlistitems don't have margin/padding by default, so this seems redundant.

>+.webconsole-marker {
>+  margin: 0;
>+  padding: 0;
>+  min-width: 6px;
>+}

margin/padding:0 seem redundant as well here.

You're setting 6px wide borders on these markers. Why is a min-width needed here?
Maybe the marker nodes can be dropped entirely? Seems like you could use a border on webconsole-msg-icon-container instead.

>+.webconsole-msg-icon {
>+  list-style-image: url(chrome://global/skin/icons/webconsole.png);
>+  margin: 4px 3px;
>+  max-width: 10px;
>+  max-height: 10px;
>+}

Why max-width/max-height rather than width/height?

>+.webconsole-msg-body {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 6px;
>+  padding: 2px 0 0;
> }

On Linux (didn't test elsewhere), this seems to put the msg body text one pixel lower than the time stamp.

>+.webconsole-location {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 0;
>+  -moz-margin-end: 6px;
>+  width: 10em;
>+  text-align: end;
>+  left: 0;
> }

"left: 0;" seems bogus, not sure what it's supposed to do.

>+.hud-msg-node[selected="true"] > .webconsole-timestamp,
>+.hud-msg-node[selected="true"] > .webconsole-location {
>+  color: HighlightText;
> }

Can you use "color: inherit;" here?

> .hud-output-node {
>+  -moz-appearance: none;
>   border-bottom: 1px solid #ddd;
>   border-top: 1px solid #ddd;

Can you use ThreeDShadow or ThreeDLightShadow as the border color?

>-  overflow-x: auto;
>-  overflow: auto;
>-  font: 1em monospace;
>+  margin: 0;
>   background-color: white;

richlistboxes default to "background-color: -moz-Field". What's "background-color: white" supposed to achieve here?

>   width: 100%;

This seems bogus too, since this is flexible box layout.

Comment 102

[Dão Gottwald [:dao]]

I'm seeing a broken msg icon when evaluating "1 1". The node for the syntax error has these classes: hud-msg-node webconsole-msg-output webconsole-msg-error hud-null

Comment 103

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 10: CSS changes.

Patch part 2 version 10 addresses review comments. The marker has been removed.

Comment 104

[Dão Gottwald [:dao]]

Is comment 102 still an issue?

You seem to have missed these:

>+.hud-msg-node {
>+  margin: 0;
>+  padding: 0;
>+}

richlistitems don't have margin/padding by default, so this seems redundant.

>+.webconsole-msg-icon {
>+  list-style-image: url(chrome://global/skin/icons/webconsole.png);
>+  margin: 4px 3px;
>+  max-width: 10px;
>+  max-height: 10px;
>+}

Why max-width/max-height rather than width/height?

>+.webconsole-msg-body {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 3px;
>+  -moz-margin-end: 6px;
>+  padding: 2px 0 0;
> }

On Linux (didn't test elsewhere), this seems to put the msg body text one pixel
lower than the time stamp.

>-  overflow-x: auto;
>-  overflow: auto;
>-  font: 1em monospace;
>+  margin: 0;
>   background-color: white;

richlistboxes default to "background-color: -moz-Field". What's
"background-color: white" supposed to achieve here?

Comment 105

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 11: CSS changes.

Patch part 2, version 11 addresses review comments.

Comment 106

[Patrick Walton (:pcwalton)]

(In reply to comment #102)
> I'm seeing a broken msg icon when evaluating "1 1". The node for the syntax
> error has these classes: hud-msg-node webconsole-msg-output
> webconsole-msg-error hud-null

This should be fixed now.

Comment 107

[Dão Gottwald [:dao]]

Attachment: screenshot

Vertical alignment within the richlistitems seems to have gotten worse. If this turns out to be the last remaining issue, I'll r+ the patch, but please file a bug to track this.

Comment 108

[Dão Gottwald [:dao]]

Comment on attachment 503640 [details] [diff] [review]
Proposed patch, part 2, version 11: CSS changes.

>--- a/toolkit/themes/gnomestripe/global/webConsole.css
>+++ b/toolkit/themes/gnomestripe/global/webConsole.css

> .hud-output-node {
>-  border-bottom: 1px solid #ddd;
>-  border-top: 1px solid #ddd;
>-  overflow-x: auto;
>-  overflow: auto;
>-  font: 1em monospace;
>+  -moz-appearance: none;
>+  border-bottom: 1px solid ThreeDShadow;
>+  border-top: 1px solid ThreeDShadow;
>+  margin: 0;
>   background-color: white;
>-  width: 100%;
> }

Missed background-color: white; here.

>+.webconsole-msg-console.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon,
>+.webconsole-msg-output.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon {

Wouldn't just .webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon match the same?

Comment 109

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

> >+.webconsole-msg-console.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon,
> >+.webconsole-msg-output.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon {
> 
> Wouldn't just .webconsole-msg-error > .webconsole-msg-icon-container >
> .webconsole-msg-icon match the same?

yes it would. I'm wondering if Patrick's doing a little optimization by being more specific? Probably not needed.

so close...

Comment 110

[Patrick Walton (:pcwalton)]

(In reply to comment #109)
> > >+.webconsole-msg-console.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon,
> > >+.webconsole-msg-output.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon {
> > 
> > Wouldn't just .webconsole-msg-error > .webconsole-msg-icon-container >
> > .webconsole-msg-icon match the same?
> 
> yes it would. I'm wondering if Patrick's doing a little optimization by being
> more specific? Probably not needed.

But that would also match these:

* .webconsole-msg-network.webconsole-msg-error > .webconsole-msg-icon-container > .webconsole-msg-icon
* .webconsole-msg-cssparser.webconsole-msg-warn > .webconsole-msg-icon-container > .webconsole-msg-icon
* .webconsole-msg-exception.webconsole-msg-warn > .webconsole-msg-icon-container > .webconsole-msg-icon

Comment 111

[Patrick Walton (:pcwalton)]

Attachment: [backed-out] Proposed patch, part 2, version 12: CSS changes.

Patch version 12 removes the stray "background-color: white".

Comment 112

[Dão Gottwald [:dao]]

It would really be helpful if these were attributes, e.g. .webconsole-msg[type=css][level=warning], rather than a pile of classes following one naming schema whilst distinguishing the messages at different levels :(

Comment 113

[Patrick Walton (:pcwalton)]

(In reply to comment #112)
> It would really be helpful if these were attributes, e.g.
> .webconsole-msg[type=css][level=warning], rather than a pile of classes
> following one naming schema whilst distinguishing the messages at different
> levels :(

Can we do this in a followup on this? That's a big undertaking and this patch has been outstanding a long time.

Comment 114

[Dão Gottwald [:dao]]

Sure, I'm just commenting on why parts of your patch are hard to follow.

Comment 115

[Dão Gottwald [:dao]]

Comment on attachment 505125 [details] [diff] [review]
[backed-out] Proposed patch, part 2, version 12: CSS changes.

For the record, vertical alignment remains an issue.

Comment 116

[Patrick Walton (:pcwalton)]

(In reply to comment #115)
> Comment on attachment 505125 [details] [diff] [review]
> Proposed patch, part 2, version 12: CSS changes.
> 
> For the record, vertical alignment remains an issue.

Will get to that as well as the classes/attributes issue in a followup. Thanks for your help!

Comment 117

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

Comment on attachment 505125 [details] [diff] [review]
[backed-out] Proposed patch, part 2, version 12: CSS changes.

http://hg.mozilla.org/mozilla-central/rev/754ea885a0f8

Comment 118

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

Comment on attachment 505125 [details] [diff] [review]
[backed-out] Proposed patch, part 2, version 12: CSS changes.

Backed-out in:
http://hg.mozilla.org/mozilla-central/rev/def11b0e968c

Investigating what broke.

Comment 119

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, part 2, version 13: CSS changes.

New patch fixes the failing test.

Comment 120

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

Comment on attachment 505187 [details] [diff] [review]
[checked-in] Proposed patch, part 2, version 13: CSS changes.

http://hg.mozilla.org/mozilla-central/rev/6f1b5f8082d2

Comment 121

[Dão Gottwald [:dao]]

(In reply to comment #116)
> (In reply to comment #115)
> > Comment on attachment 505125 [details] [diff] [review]
> > Proposed patch, part 2, version 12: CSS changes.
> > 
> > For the record, vertical alignment remains an issue.
> 
> Will get to that as well as the classes/attributes issue in a followup.

Have these bugs been filed yet?

Comment 122

[Patrick Walton (:pcwalton)]

(In reply to comment #121)
> (In reply to comment #116)
> > (In reply to comment #115)
> > > Comment on attachment 505125 [details] [diff] [review]
> > > Proposed patch, part 2, version 12: CSS changes.
> > > 
> > > For the record, vertical alignment remains an issue.
> > 
> > Will get to that as well as the classes/attributes issue in a followup.
> 
> Have these bugs been filed yet?

Bug 628018, bug 628019.

Comment 123

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre