Web Console output box should be reskinned

VERIFIED FIXED in Firefox 4.0b10

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

(Depends on: 1 bug, {icon})

unspecified
Firefox 4.0b10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][eta: 1/20][fx4-fixed-bugday])

Attachments

(8 attachments, 31 obsolete attachments)

133.21 KB, image/png
Details
10.40 KB, patch
Details | Diff | Splinter Review
22.78 KB, patch
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
1.92 KB, patch
Details | Diff | Splinter Review
112.73 KB, patch
Details | Diff | Splinter Review
7.47 KB, image/png
Details
19.39 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Per Alex Faaborg's mockups in bug 599498, we need a "timeline" display for the Web Console output.
(Assignee)

Comment 1

7 years ago
Created attachment 487101 [details]
WIP screenshot.

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

Comment 2

7 years ago
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.
Keywords: icon
Screenshot looks great, Patrick!

Updated

7 years ago
Blocks: 603005

Updated

7 years ago
Duplicate of this bug: 601464

Updated

7 years ago
Duplicate of this bug: 599240
Duplicate of this bug: 601671
(Assignee)

Comment 7

7 years ago
Created attachment 489380 [details] [diff] [review]
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.
Attachment #489380 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 8

7 years ago
Created attachment 489381 [details] [diff] [review]
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 :)
Attachment #489380 - Attachment is obsolete: true
Attachment #489381 - Flags: feedback?
Attachment #489380 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 9

7 years ago
Created attachment 489382 [details] [diff] [review]
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.)
Attachment #489382 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 10

7 years ago
Nominating for blocking status, as this blocks a betaN+ blocker.
blocking2.0: --- → ?
(Assignee)

Updated

7 years ago
Attachment #489381 - Flags: feedback? → feedback?(mihai.sucan)
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)
(Assignee)

Comment 12

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

Updated

7 years ago
Depends on: 607163, 601667
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.
Attachment #489381 - Flags: feedback?(mihai.sucan) → feedback-
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?
Attachment #489382 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 15

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

Updated

7 years ago
Attachment #489382 - Flags: review?(dao)
(Assignee)

Comment 16

7 years ago
Created attachment 489669 [details] [diff] [review]
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.
Attachment #489381 - Attachment is obsolete: true
Attachment #489669 - Flags: feedback?(mihai.sucan)
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.
(Assignee)

Updated

7 years ago
Blocks: 588379
(Assignee)

Comment 18

7 years ago
Created attachment 489872 [details] [diff] [review]
Proposed patch, part 1, version 4: Code changes.

Patch part 1, version 4 addresses feedback comments. Review requested.
Attachment #489669 - Attachment is obsolete: true
Attachment #489872 - Flags: review?(gavin.sharp)
Attachment #489669 - Flags: feedback?(mihai.sucan)
(Assignee)

Updated

7 years ago
Attachment #489872 - Attachment is obsolete: true
Attachment #489872 - Flags: review?(gavin.sharp)
(Assignee)

Comment 19

7 years ago
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.
Attachment #489382 - Attachment is obsolete: true
Attachment #489382 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Depends on: 611506
(Assignee)

Updated

7 years ago
Depends on: 611795
(Assignee)

Comment 20

7 years ago
Created attachment 490275 [details] [diff] [review]
Proposed patch, part 1, version 5: Code changes.

Patch part 1, version 5 adds view-source links to the error messages.
Attachment #490275 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 21

7 years ago
Created attachment 490276 [details] [diff] [review]
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.
Attachment #490276 - Flags: feedback?(mihai.sucan)

Updated

7 years ago
Depends on: 580618
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.
Attachment #490275 - Flags: feedback?(mihai.sucan) → feedback+
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.
Attachment #490276 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Updated

7 years ago
Duplicate of this bug: 601672
(Assignee)

Updated

7 years ago
Blocks: 592309
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.
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 ;)
blocking2.0: ? → final+

Comment 27

7 years ago
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.
No longer depends on: 611795
(Assignee)

Comment 28

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

Comment 29

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

Comment 30

7 years ago
Created attachment 491700 [details] [diff] [review]
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.
Attachment #490275 - Attachment is obsolete: true
Attachment #491700 - Flags: review?(gavin.sharp)
(Assignee)

Comment 31

7 years ago
Created attachment 491701 [details] [diff] [review]
Proposed patch, part 2, version 3: CSS changes.

Proposed patch, part 2, version 3 addresses feedback. Review requested.
Attachment #490276 - Attachment is obsolete: true
Attachment #491701 - Flags: review?(dao)
(Assignee)

Comment 32

7 years ago
Created attachment 492006 [details] [diff] [review]
Proposed patch, part 1, version 7: Code changes.

Proposed patch, part 1, version 7 rebases to trunk.
Attachment #491700 - Attachment is obsolete: true
Attachment #492006 - Flags: review?(gavin.sharp)
Attachment #491700 - Flags: review?(gavin.sharp)

Updated

7 years ago
Duplicate of this bug: 578658

Comment 34

7 years ago
mass change: filter on PRIORITYSETTING
Priority: -- → P1

Comment 35

7 years ago
mass change: filter on PRIORITYSETTING
Blocks: 593956

Comment 36

7 years ago
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker
(Assignee)

Comment 37

7 years ago
Created attachment 492883 [details] [diff] [review]
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).
Attachment #492006 - Attachment is obsolete: true
Attachment #492883 - Flags: review?(gavin.sharp)
Attachment #492006 - Flags: review?(gavin.sharp)
(Assignee)

Comment 38

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

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1123]
(Assignee)

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1123] → [see comment 38 for icons needed] [patchbitrot]
(Assignee)

Comment 39

7 years ago
These patches, and the CSS, have bitrotted.
(Assignee)

Comment 40

7 years ago
Created attachment 494199 [details] [diff] [review]
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.
Attachment #492883 - Attachment is obsolete: true
Attachment #494199 - Flags: review?(gavin.sharp)
Attachment #492883 - Flags: review?(gavin.sharp)
(Assignee)

Comment 41

7 years ago
Created attachment 494200 [details] [diff] [review]
Proposed patch, part 2, version 4: CSS changes.

Proposed patch, part 2, version 4 rebases to trunk and adds styling for CSS warnings.
Attachment #491701 - Attachment is obsolete: true
Attachment #494200 - Flags: review?(dao)
Attachment #491701 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchbitrot] → [see comment 38 for icons needed] [patchclean:1130]
(Assignee)

Updated

7 years ago
Attachment #494199 - Flags: review?(gavin.sharp) → review?(sdwilsh)
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!
(Assignee)

Comment 43

7 years ago
(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

7 years ago
(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?
(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.

Updated

7 years ago
Severity: blocker → normal

Updated

7 years ago
Blocks: 604618
(Assignee)

Comment 46

7 years ago
Created attachment 495580 [details] [diff] [review]
Proposed patch, part 1, version 10: Code changes.

Proposed patch, part 1, version 10 addresses feedback.
Attachment #494199 - Attachment is obsolete: true
Attachment #495580 - Flags: feedback?(mihai.sucan)
Attachment #494199 - Flags: review?(sdwilsh)
(Assignee)

Comment 47

7 years ago
Created attachment 495581 [details] [diff] [review]
Proposed patch, part 2, version 5: CSS changes.

Proposed patch, part 2, version 5 addresses feedback comments.
Attachment #494200 - Attachment is obsolete: true
Attachment #495581 - Flags: feedback?(mihai.sucan)
Attachment #494200 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Attachment #495580 - Attachment description: Prpoposed patch, part 1, version 10: Code changes. → Proposed patch, part 1, version 10: Code changes.
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.
Attachment #495580 - Flags: feedback?(mihai.sucan) → feedback+
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.
Attachment #495581 - Flags: feedback?(mihai.sucan) → feedback+

Updated

7 years ago
Blocks: 617322
(Assignee)

Updated

7 years ago
Blocks: 617698
(Assignee)

Comment 50

7 years ago
Created attachment 496355 [details] [diff] [review]
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!)
Attachment #495580 - Attachment is obsolete: true
Attachment #496355 - Flags: review?(sdwilsh)
(Assignee)

Updated

7 years ago
Attachment #496355 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 51

7 years ago
Created attachment 496358 [details] [diff] [review]
Interdiff for patch part 1, version 10 to version 11.

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

Comment 52

7 years ago
Created attachment 496392 [details] [diff] [review]
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.
Attachment #495581 - Attachment is obsolete: true
Attachment #496392 - Flags: feedback?(mihai.sucan)
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!)
Attachment #496355 - Flags: feedback?(mihai.sucan) → feedback-
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.
Attachment #496392 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 55

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

Comment 56

7 years ago
(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

7 years ago
> 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.";
(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.
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>.
(Assignee)

Comment 60

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

Comment 61

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

Comment 62

7 years ago
Created attachment 496575 [details] [diff] [review]
Proposed patch, part 1, version 12: Code changes.

Patch part 1, version 12 addresses feedback comments.
Attachment #496355 - Attachment is obsolete: true
Attachment #496575 - Flags: review?(sdwilsh)
Attachment #496575 - Flags: feedback?(mihai.sucan)
Attachment #496355 - Flags: review?(sdwilsh)
(Assignee)

Comment 63

7 years ago
Created attachment 496579 [details] [diff] [review]
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.
Attachment #496392 - Attachment is obsolete: true
Attachment #496579 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 64

7 years ago
Created attachment 496633 [details] [diff] [review]
Proposed patch, part 1, version 13: Code changes.

Patch part 1, version 13 adds more comments.
Attachment #496575 - Attachment is obsolete: true
Attachment #496633 - Flags: review?(sdwilsh)
Attachment #496575 - Flags: review?(sdwilsh)
Attachment #496575 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 65

7 years ago
Created attachment 496634 [details] [diff] [review]
Interdiff for patch part 1, version 12 to version 13.

Interdiff between versions 12 and 13 attached.
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.
Attachment #496575 - Attachment is obsolete: false

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] → [see comment 38 for icons needed] [patchclean:1130] [strings]

Comment 67

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

Comment 68

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

Comment 69

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

Comment 70

7 years ago
Created attachment 496687 [details] [diff] [review]
Interdiff for patch part 1, version 13 to version 14.

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

Comment 71

7 years ago
Created attachment 496688 [details] [diff] [review]
Proposed patch, part 1, version 14: Code changes.

Proposed patch, part 1, version 14 addresses review feedback.
Attachment #496633 - Attachment is obsolete: true
Attachment #496688 - Flags: review?(sdwilsh)
Attachment #496633 - Flags: review?(sdwilsh)
(Assignee)

Updated

7 years ago
Attachment #496575 - Attachment is obsolete: true
(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.
(Assignee)

Comment 73

7 years ago
Created attachment 496700 [details] [diff] [review]
Proposed patch, part 1, version 15: Code changes.

Patch part 1 version 15 fixes those two issues.
Attachment #496688 - Attachment is obsolete: true
Attachment #496700 - Flags: review?(sdwilsh)
Attachment #496688 - Flags: review?(sdwilsh)
(Assignee)

Comment 74

7 years ago
Created attachment 496701 [details] [diff] [review]
Interdiff for patch part 1, version 14 to version 15.

Interdiff between versions 14 and 15 of part 1 attached.
Attachment #496358 - Attachment is obsolete: true
(Assignee)

Comment 75

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

That wasn't an interdiff.
Attachment #496701 - Attachment is obsolete: true
(Assignee)

Comment 76

7 years ago
Created attachment 496702 [details] [diff] [review]
Interdiff for patch part 1, version 14 to version 15.

Interdiff between versions 14 and 15 of patch part 1 attached.
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 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.
Attachment #496579 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Updated

7 years ago
Attachment #496579 - Flags: review?(dao)
(Assignee)

Comment 79

7 years ago
Created attachment 497191 [details] [diff] [review]
Proposed patch, part 1, version 16: Code changes.

Patch part 1, version 16 addresses Mihai's comments.
Attachment #496700 - Attachment is obsolete: true
Attachment #497191 - Flags: review?(sdwilsh)
Attachment #496700 - Flags: review?(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)
(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.
(Assignee)

Comment 82

7 years ago
Created attachment 497331 [details] [diff] [review]
Interdiff for patch part 1, version 15 to version 16.
Comment on attachment 497191 [details] [diff] [review]
Proposed patch, part 1, version 16: Code changes.

r=sdwilsh, with previous comments addressed.
Attachment #497191 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 84

7 years ago
(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);"?
(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
(Assignee)

Comment 86

7 years ago
Created attachment 497603 [details] [diff] [review]
Proposed patch, part 1, version 17: Code changes.

Patch part 1, version 17 addresses review comments.
Attachment #497191 - Attachment is obsolete: true
(Assignee)

Comment 87

7 years ago
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.

Updated

7 years ago
blocking2.0: final+ → betaN+
(Assignee)

Comment 88

7 years ago
Created attachment 497824 [details] [diff] [review]
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).
Attachment #496579 - Attachment is obsolete: true
Attachment #497824 - Flags: review?(dao)
Attachment #496579 - Flags: review?(dao)

Updated

7 years ago
Duplicate of this bug: 617322

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] [strings] → [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only]
(Assignee)

Comment 90

7 years ago
Created attachment 499397 [details] [diff] [review]
[checked-in] Proposed patch, part 1, version 18: Code changes.

Patch part 1, version 18 updates to tip.
Attachment #497603 - Attachment is obsolete: true
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
Attachment #499397 - Attachment description: Proposed patch, part 1, version 18: Code changes. → [checked-in] Proposed patch, part 1, version 18: Code changes.
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.
Attachment #497824 - Flags: review?(dao) → review-

Updated

7 years ago
Blocks: 613280
(Assignee)

Updated

7 years ago
Duplicate of this bug: 622761
(Assignee)

Comment 94

7 years ago
Created attachment 501904 [details] [diff] [review]
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.
Attachment #497824 - Attachment is obsolete: true
Attachment #501904 - Flags: review?(dao)
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.
The string changes have landed.
(In reply to comment #96)
> The string changes have landed.

Ah, and were just removals, k.
(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.
(In reply to comment #98)
> Although my patch/icons could probably be rolled into this one.

Please don't.
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only] → [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][softblocker]

Updated

7 years ago
Blocks: 611440
(Assignee)

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][softblocker] → [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][hardblocker]
(Assignee)

Comment 100

7 years ago
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.
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][hardblocker] → [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][hardblocker][needs review 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.
Attachment #501904 - Flags: review?(dao) → review-
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
(Assignee)

Comment 103

7 years ago
Created attachment 503589 [details] [diff] [review]
Proposed patch, part 2, version 10: CSS changes.

Patch part 2 version 10 addresses review comments. The marker has been removed.
Attachment #501904 - Attachment is obsolete: true
Attachment #503589 - Flags: review?(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?
(Assignee)

Comment 105

7 years ago
Created attachment 503640 [details] [diff] [review]
Proposed patch, part 2, version 11: CSS changes.

Patch part 2, version 11 addresses review comments.
Attachment #503589 - Attachment is obsolete: true
Attachment #503640 - Flags: review?(dao)
Attachment #503589 - Flags: review?(dao)
(Assignee)

Comment 106

7 years ago
(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.
Created attachment 503794 [details]
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 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?
> >+.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...
Blocks: 611795

Updated

7 years ago
Attachment #503640 - Flags: review?(dao)

Updated

7 years ago
Whiteboard: [see comment 38 for icons needed] [patchclean:1130] [strings][string removal only][hardblocker][needs review dao] → [hardblocker][needs review dao][eta: 1/20]

Updated

7 years ago
Whiteboard: [hardblocker][needs review dao][eta: 1/20] → [hardblocker][eta: 1/20]
(Assignee)

Comment 110

7 years ago
(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
Blocks: 626702
(Assignee)

Comment 111

7 years ago
Created attachment 505125 [details] [diff] [review]
[backed-out] Proposed patch, part 2, version 12: CSS changes.

Patch version 12 removes the stray "background-color: white".
Attachment #503640 - Attachment is obsolete: true
Attachment #505125 - Flags: review?(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 :(
(Assignee)

Comment 113

7 years ago
(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.
Sure, I'm just commenting on why parts of your patch are hard to follow.
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.
Attachment #505125 - Flags: review?(dao) → review+
(Assignee)

Comment 116

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

Updated

7 years ago
Keywords: checkin-needed
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
Attachment #505125 - Attachment description: Proposed patch, part 2, version 12: CSS changes. → [checked-in] Proposed patch, part 2, version 12: CSS changes.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → Firefox 4.0b10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 4.0b10 → ---
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.
Attachment #505125 - Attachment description: [checked-in] Proposed patch, part 2, version 12: CSS changes. → [backed-out] Proposed patch, part 2, version 12: CSS changes.
(Assignee)

Comment 119

7 years ago
Created attachment 505187 [details] [diff] [review]
[checked-in] Proposed patch, part 2, version 13: CSS changes.

New patch fixes the failing test.
Attachment #505125 - Attachment is obsolete: true
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
Attachment #505187 - Attachment description: Proposed patch, part 2, version 13: CSS changes. → [checked-in] Proposed patch, part 2, version 13: CSS changes.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → Firefox 4.0b10

Updated

7 years ago
Depends on: 627819
(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?
(Assignee)

Comment 122

7 years ago
(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.

Updated

7 years ago
Depends on: 628018
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker][eta: 1/20] → [hardblocker][eta: 1/20][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Updated

7 years ago
Depends on: 637604
You need to log in before you can comment on or make changes to this bug.