Last Comment Bug 788445 - Add a tooltip for webconsole count on Developer Tool bar
: Add a tooltip for webconsole count on Developer Tool bar
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Graphic Commandline and Toolbar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 21
Assigned To: Tyrone Chong
:
: Joe Walker [:jwalker] (needinfo me or ping on irc)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-05 03:04 PDT by David Burns :automatedtester
Modified: 2013-01-25 07:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip. (3.44 KB, patch)
2013-01-16 23:33 PST, Tyrone Chong
mihai.sucan: feedback+
Details | Diff | Splinter Review
[REVISED] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip (4.76 KB, patch)
2013-01-17 19:21 PST, Tyrone Chong
no flags Details | Diff | Splinter Review
[REVISED-2] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip (4.64 KB, patch)
2013-01-20 19:43 PST, Tyrone Chong
no flags Details | Diff | Splinter Review
[REVISED-3] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip (4.90 KB, patch)
2013-01-21 22:10 PST, Tyrone Chong
mihai.sucan: review+
Details | Diff | Splinter Review
patch with tests (15.57 KB, patch)
2013-01-22 08:45 PST, Mihai Sucan [:msucan]
mihai.sucan: checkin+
Details | Diff | Splinter Review

Description David Burns :automatedtester 2012-09-05 03:04:59 PDT
Currently the count on the Developer toolbar only shows the count for JavaScript errors. It is not obvious that is what it is for so a tooltip would be great!
Comment 1 Tyrone Chong 2013-01-09 00:37:31 PST
is this still valid?
Comment 2 Paul Rouget [:paul] 2013-01-09 00:47:55 PST
(In reply to Tyrone Chong from comment #1)
> is this still valid?

Yes.
Comment 3 Tyrone Chong 2013-01-09 01:03:13 PST
Cool. I was wondering if i can pick this up for my school project. We need to submit a fix and get it approved on a open source project. If i can tackle this thatll be great.
Comment 4 Paul Rouget [:paul] 2013-01-09 01:22:38 PST
(In reply to Tyrone Chong from comment #3)
> Cool. I was wondering if i can pick this up for my school project. We need
> to submit a fix and get it approved on a open source project. If i can
> tackle this thatll be great.

Come to IRC (irc.mozilla.org #devtools). Myself and msucan will be able to help you.
Comment 5 Mihai Sucan [:msucan] 2013-01-09 01:52:42 PST
Tyrone: thanks for your interest in this bug.

Overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch


If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

1. The developer toolbar is implemented in browser/devtools/shared/DeveloperToolbar.jsm.

2. The toolbar UI is in browser/base/content/browser.xul - search for id="developer-toolbar".

3. The error counter can be found in DeveloperToolbar.jsm - search for the _updateErrorsCount() method.

4. The _errorCounterButton element is #developer-toolbar-toolbox-button from browser.xul.

5. You just need to update the button tooltiptext in _updateErrorsCount().

Please let us know if you have further questions. Thanks!
Comment 6 Tyrone Chong 2013-01-09 08:04:03 PST
I'll build it right now. How do i assign this bug to me?
Comment 7 Mihai Sucan [:msucan] 2013-01-09 08:10:29 PST
Assigned.
Comment 8 Tyrone Chong 2013-01-10 01:18:38 PST
Hey,

I can't seem to see any error counts for Javascript. I successfully build the project and I made sure my changes get updated(edited a javascript file to see if my changes apply, and it does get applied). When I edited .jsm and .xul file do I have to build the project for the changes to get applied?

-Tyrone
Comment 9 Paul Rouget [:paul] 2013-01-10 01:49:58 PST
(In reply to Tyrone Chong from comment #8)
> Hey,
> 
> I can't seem to see any error counts for Javascript. I successfully build
> the project and I made sure my changes get updated(edited a javascript file
> to see if my changes apply, and it does get applied). When I edited .jsm and
> .xul file do I have to build the project for the changes to get applied?

Yes. You need to rebuild. But not everything. Just the browser/devtools directory. In your obj dir, do: "make -C obj/browser/devtools" (the obj directory might be named differently depending on how you built Firefox).
Comment 10 Tyrone Chong 2013-01-13 10:54:52 PST
Hey is there a specific text you want
Comment 11 Tyrone Chong 2013-01-14 01:20:17 PST
Hey Paul, Mihai,

Is there any specific text we should put into the tooltip? Let me know thanks.
Comment 12 Paul Rouget [:paul] 2013-01-14 03:26:30 PST
Maybe something like:

3 errors, 10 warnings

Mihai, do you think that covers correctly the information we have?
Comment 13 Mihai Sucan [:msucan] 2013-01-14 03:51:38 PST
(In reply to Paul Rouget [:paul] from comment #12)
> Maybe something like:
> 
> 3 errors, 10 warnings
> 
> Mihai, do you think that covers correctly the information we have?

Yes, but then Tyrone has to make a small change to also count the warnings. If he can do that, that's great. Otherwise just showing "N errors" is fine.

Also note that this is a toolbox toggle button. The tooltip should also include something to that effect. "N errors, M warnings. Click to open the developer toolbox." or something else.
Comment 14 Tyrone Chong 2013-01-15 17:05:57 PST
Sure I can try to add the warning count. I'm assuming I have to initiate a _warningsCount and do everything similar to how the errorCount is set up. I'm trying to find the global _errorCount. Is that in another file?
Comment 15 Tyrone Chong 2013-01-15 17:57:38 PST
As of right now I have it showing up as..

'N errors. Click to open developer toolbox.'
Comment 16 Tyrone Chong 2013-01-16 00:52:14 PST
Hey Mihai and Paul,

I was able to get the warning count also. I used a site with warning messages on there to test if its working correctly and it seems like it is :)

Referencing the link just in case i forget: http://www.carsdirect.com/honda/accord
Comment 17 Tyrone Chong 2013-01-16 23:33:54 PST
Created attachment 703181 [details] [diff] [review]
Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip.
Comment 18 Mihai Sucan [:msucan] 2013-01-17 04:04:52 PST
Comment on attachment 703181 [details] [diff] [review]
Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip.

Review of attachment 703181 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch Tyrone! This is looking good.

Just a few comments below for improvements.

Paul: Tyrone needs to add a new string. Where should he do it? Can he reuse toolbox.properties or do we want a new toolbar.properties?

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +417,4 @@
>  DeveloperToolbar.prototype._stopErrorsCount = function DT__stopErrorsCount(aTab)
>  {
>    let tabId = aTab.linkedPanel;
> +  if (!(tabId in this._errorsCount) || !tabId in this._warningsCount) {

Please put parenthesis around the second condition.

if (!(tabId in this._errorsCount) || !(tabId in this._warningsCount)) {

@@ +557,2 @@
>        (aPageError.flags & aPageError.strictFlag)) {
>      return; // just a CSS or JS warning

We probably don't need this comment anymore, Also, strict warnings, when they are enabled, should also be counted.

@@ +557,4 @@
>        (aPageError.flags & aPageError.strictFlag)) {
>      return; // just a CSS or JS warning
>    }
> +  if(aPageError.flags & aPageError.warningFlag) {

Coding style: please put a space after "if".

@@ +607,4 @@
>  function DT__updateErrorsCount(aChangedTabId)
>  {
>    let tabId = this._chromeWindow.getBrowser().selectedTab.linkedPanel;
> +  

Please make sure you do not add any trailing whitespace in the file.

@@ +615,3 @@
>    let errors = this._errorsCount[tabId];
> +  let warnings = this._warningsCount[tabId];
> +  let tooltiptext = errors + " errors, " + warnings +" warnings. Click to open developer toolbox.";

Please make sure this string is localizable. You can learn more about string bundles and .properties files from:

https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Property_Files

@@ +638,5 @@
>      this._updateErrorsCount(tabId);
>    }
> +  if (tabId in this._warningsCount) {
> +    this._warningsCount[tabId] = 0;
> +    this._updateErrorsCount(tabId);

I suggest you put the warnings count reset in the condition above - so if tabId in _errorsCount, then you can safely do warningsCount[tabId] = 0 without checking for the existence of tabId in _warningsCount. Also, you don't need to call _updateErrorsCount() twice.
Comment 19 Mihai Sucan [:msucan] 2013-01-17 04:28:44 PST
Tyrone: I asked Joe Walker about where would be the best place for you to add the new string (Paul is currently away): the conclusion we came to is that you best add the new string to the toolbox.properties (use |hg locate| to find its location in the codebase).
Comment 20 Tyrone Chong 2013-01-17 19:21:36 PST
Created attachment 703723 [details] [diff] [review]
[REVISED] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip
Comment 21 Tyrone Chong 2013-01-17 19:22:34 PST
Submitted [REVISED] Patch with the following suggestions/changes you mentioned. Thanks!
Comment 22 Mihai Sucan [:msucan] 2013-01-18 09:36:11 PST
Comment on attachment 703723 [details] [diff] [review]
[REVISED] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Review of attachment 703723 [details] [diff] [review]:
-----------------------------------------------------------------

Tyrone, thank you for the patch update!

More comments for your patch:

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +575,1 @@
>    this._errorsCount[aTabId]++;

Why do you increase errors count for warnings?

@@ +630,4 @@
>    if (errors) {
>      this._errorCounterButton.setAttribute("error-count", errors);
> +    this._errorCounterButton.setAttribute("tooltiptext", tooltiptext +
> +                                          toolboxStrings("toolboxDockButtons.open.tooltip"));

This approach doesn't make the tooltip fully translatable into other languages.

Please put the entire string in the properties file and use formatStringFromName(). Learn more here:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIStringBundle

@@ +634,2 @@
>    } else {
>      this._errorCounterButton.removeAttribute("error-count");

What happens with the tooltiptext once there are no errors?

@@ +652,2 @@
>    if (tabId in this._errorsCount) {
>      this._errorsCount[tabId] = 0;

Why not:

if (tabId in this._errorsCount || tabId in this._warningsCount) {
  this._warningsCount[tabId] = 0;
  this._errorsCount[tabId] = 0;
...
Comment 23 Tyrone Chong 2013-01-20 19:43:19 PST
Created attachment 704395 [details] [diff] [review]
[REVISED-2] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Hey Mihai,

Here's the patch with your comments/suggestions mentioned for the revised one. Let me know if there are any more issues. Thanks
Comment 24 Mihai Sucan [:msucan] 2013-01-21 13:44:10 PST
Comment on attachment 704395 [details] [diff] [review]
[REVISED-2] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Review of attachment 704395 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your update.

More comments below.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +625,3 @@
>    } else {
>      this._errorCounterButton.removeAttribute("error-count");
> +    this._errorCounterButton.removeAttribute("tooltiptext");

Once you do this the button will no longer have any tooltip, not even the default one.

Before you change the tooltiptext, there's a default tooltiptext in the XUL.

::: browser/locales/en-US/chrome/browser/devtools/toolbox.properties
@@ +1,4 @@
>  toolboxDockButtons.bottom.tooltip=Dock to bottom of browser window
>  toolboxDockButtons.side.tooltip=Dock to side of browser window
>  toolboxDockButtons.window.tooltip=Show in separate window
> +toolboxDockButtons.errorsCount.tooltip=%S errors, %S warnings. Click to open developer toolbox
\ No newline at end of file

Click to toggle the developer tools.
Comment 25 Tyrone Chong 2013-01-21 21:45:59 PST
Hey Mihai,

Are you on at this time by anychance? If not. Whats a good way to deal with that should I set the tooltiptext back to "Click to toggle developer tools."? Or is there a way to set it back?

-Tyrone
Comment 26 Tyrone Chong 2013-01-21 21:55:35 PST
I was thinking about getting the defaultTooltipText when developer tool loads? then save that globally?
Comment 27 Tyrone Chong 2013-01-21 22:10:30 PST
Created attachment 704770 [details] [diff] [review]
[REVISED-3] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip
Comment 28 Tyrone Chong 2013-01-21 22:12:32 PST
I got the default tooltip and created a variable for it, then I made the text "Click to toggle developer tools." in a new line, is this correct? let me know if you still me to made adjustments and changes. Thanks Mihai
Comment 29 Mihai Sucan [:msucan] 2013-01-22 04:37:37 PST
Comment on attachment 704770 [details] [diff] [review]
[REVISED-3] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Thank you for the updated patch.
Comment 30 Mihai Sucan [:msucan] 2013-01-22 08:45:39 PST
Created attachment 704908 [details] [diff] [review]
patch with tests

Updated the patch to include tests.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=0479468b5b9d
Comment 31 Tyrone Chong 2013-01-22 21:29:11 PST
You're welcome, thank you for the guidance :D let me know if this patch gets accepted.
Comment 32 Paul Rouget [:paul] 2013-01-23 01:27:19 PST
(In reply to Tyrone Chong from comment #31)
> You're welcome, thank you for the guidance :D let me know if this patch gets
> accepted.

Because you got a "r+", that means your patch has been accepted. It will be in Firefox as soon as the status of this bug becomes "RESOLVED FIXED". Thank you :)
Comment 33 Mihai Sucan [:msucan] 2013-01-23 02:25:28 PST
Comment on attachment 704908 [details] [diff] [review]
patch with tests

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/65494544bdeb
Comment 34 Panos Astithas [:past] 2013-01-24 00:18:23 PST
https://hg.mozilla.org/mozilla-central/rev/65494544bdeb
Comment 35 Francesco Lodolo [:flod] 2013-01-25 00:08:44 PST
Is it just me or this string should have support plural forms?
https://developer.mozilla.org/en/docs/Localization_and_Plurals
Comment 36 Mihai Sucan [:msucan] 2013-01-25 07:30:22 PST
Francesco: thank you for pointing out the problem. Reported bug 834721.

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