Last Comment Bug 762996 - Add errors count to the Web Console button in the developer toolbar
: Add errors count to the Web Console button in the developer toolbar
Status: RESOLVED FIXED
[devtb]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 764852 (view as bug list)
Depends on: 761257
Blocks: 764746
  Show dependency treegraph
 
Reported: 2012-06-08 11:19 PDT by Mihai Sucan [:msucan]
Modified: 2012-06-16 03:49 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[in-fx-team] proposed patch (31.16 KB, patch)
2012-06-13 11:38 PDT, Mihai Sucan [:msucan]
paul: review+
Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2012-06-08 11:19:47 PDT
Add errors count to the Web Console button in the developer toolbar.
Comment 1 Mihai Sucan [:msucan] 2012-06-13 11:38:01 PDT
Created attachment 632786 [details] [diff] [review]
[in-fx-team] proposed patch

This is the proposed patch with a test included. The Web Console code needed really minimal changes to make sure that both the dev toolbar and the Web Console can reuse the same HUDService-content.js script at the same time.

Paul: please let me know if you can also review this code or if I should also ask someone else for a review. Thank you!

I would also appreciate your UI magic on this patch. Currently I only show the errors count like this: "Web Console (N)". If you have time, please make a patch on top of this one that changes how the errors count is displayed. Or please give me UI suggestions on how you'd like this be - I can implement it if you are busy. Thank you very much!


Try push:
https://tbpl.mozilla.org/?tree=Try&rev=2768240e842e
Comment 2 Mihai Sucan [:msucan] 2012-06-13 11:45:48 PDT
Do note the Web Console patch queue I currently have:
https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/series
Comment 3 Paul Rouget [:paul] 2012-06-14 02:44:56 PDT
Comment on attachment 632786 [details] [diff] [review]
[in-fx-team] proposed patch

This is an awesome feature :) Thank you Mihai.

Nits, not blocking:

+  if (errors) {
+    this._webConsoleButton.label =
+      this._webConsoleButtonLabel + " (" + errors + ")";
+  }

Can you add a <label> element in the button instead?
(I can do it later if you want)

+DeveloperToolbar.prototype._onPageError =
+function DT__onPageError(aTabId, aPageError)
+{
+  if (aPageError.category == "CSS Parser" ||
+      aPageError.category == "CSS Loader" ||
+      (aPageError.flags & aPageError.warningFlag) ||
+      (aPageError.flags & aPageError.strictFlag)) {
+    return; // just a CSS or JS warning
+  }
+
+  this._errorsCount[aTabId]++;
+};

Instead of a black list, can we have a white list?
For clarity and in case we add new kind of logs in the future.
Comment 4 Mihai Sucan [:msucan] 2012-06-14 03:13:20 PDT
(In reply to Paul Rouget [:paul] from comment #3)
> Comment on attachment 632786 [details] [diff] [review]
> proposed patch
> 
> This is an awesome feature :) Thank you Mihai.

Thanks! I'm glad you like it.

> Nits, not blocking:
> 
> +  if (errors) {
> +    this._webConsoleButton.label =
> +      this._webConsoleButtonLabel + " (" + errors + ")";
> +  }
> 
> Can you add a <label> element in the button instead?
> (I can do it later if you want)

Sure!

> +DeveloperToolbar.prototype._onPageError =
> +function DT__onPageError(aTabId, aPageError)
> +{
> +  if (aPageError.category == "CSS Parser" ||
> +      aPageError.category == "CSS Loader" ||
> +      (aPageError.flags & aPageError.warningFlag) ||
> +      (aPageError.flags & aPageError.strictFlag)) {
> +    return; // just a CSS or JS warning
> +  }
> +
> +  this._errorsCount[aTabId]++;
> +};
> 
> Instead of a black list, can we have a white list?
> For clarity and in case we add new kind of logs in the future.

Good point, but this copies the way the Web Console works. We should keep these in sync. These PageError messages are nsIScriptErrors we receive from HUDService-content.js.

Thank you for the r+!
Comment 5 Mihai Sucan [:msucan] 2012-06-14 05:07:42 PDT
Comment on attachment 632786 [details] [diff] [review]
[in-fx-team] proposed patch

https://hg.mozilla.org/integration/fx-team/rev/59c5ee47dc45
Comment 6 Mihai Sucan [:msucan] 2012-06-14 05:08:30 PDT
Tried adding a label element into the toolbarbutton as suggested, but it doesn't show up. Leaving style work to happen in bug 764746. Thank you!
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-14 09:00:14 PDT
*** Bug 764852 has been marked as a duplicate of this bug. ***
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-06-16 03:45:01 PDT
https://hg.mozilla.org/mozilla-central/rev/59c5ee47dc45

Note You need to log in before you can comment on or make changes to this bug.