788445 - Add a tooltip for webconsole count on Developer Tool bar

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[David Burns :automatedtester]

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]

is this still valid?

Comment 2

[Paul Rouget [:paul]]

(In reply to Tyrone Chong from comment #1)
> is this still valid?

Yes.

Comment 3

[Tyrone Chong]

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]]

(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]]

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]

I'll build it right now. How do i assign this bug to me?

Comment 7

[Mihai Sucan [:msucan]]

Assigned.

Comment 8

[Tyrone Chong]

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]]

(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]

Hey is there a specific text you want

Comment 11

[Tyrone Chong]

Hey Paul, Mihai,

Is there any specific text we should put into the tooltip? Let me know thanks.

Comment 12

[Paul Rouget [:paul]]

Maybe something like:

3 errors, 10 warnings

Mihai, do you think that covers correctly the information we have?

Comment 13

[Mihai Sucan [:msucan]]

(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]

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]

As of right now I have it showing up as..

'N errors. Click to open developer toolbox.'

Comment 16

[Tyrone Chong]

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]

Attachment: Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip.

Comment 18

[Mihai Sucan [:msucan]]

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]]

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]

Attachment: [REVISED] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Comment 21

[Tyrone Chong]

Submitted [REVISED] Patch with the following suggestions/changes you mentioned. Thanks!

Comment 22

[Mihai Sucan [:msucan]]

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]

Attachment: [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]]

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]

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]

I was thinking about getting the defaultTooltipText when developer tool loads? then save that globally?

Comment 27

[Tyrone Chong]

Attachment: [REVISED-3] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip

Comment 28

[Tyrone Chong]

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]]

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]]

Attachment: patch with tests

Updated the patch to include tests.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=0479468b5b9d

Comment 31

[Tyrone Chong]

You're welcome, thank you for the guidance :D let me know if this patch gets accepted.

Comment 32

[Paul Rouget [:paul]]

(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]]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/65494544bdeb

Comment 35

[Francesco Lodolo [:flod]]

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]]

Francesco: thank you for pointing out the problem. Reported bug 834721.