Closed Bug 788445 Opened 12 years ago Closed 11 years ago

Add a tooltip for webconsole count on Developer Tool bar

Categories

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

Product:
DevTools Graveyard
Component:
Graphic Commandline and Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 21

People

(Reporter: automatedtester, Assigned: tyronekc)

Details

Attachments

(2 files, 3 obsolete files)

[REVISED-3] Patch adding tooltip to the DeveloperToolbar error count and included warning count(s) in the tooltip
4.90 KB, patch
msucan
: review+
Details | Diff | Splinter Review
patch with tests
15.57 KB, patch
msucan
: checkin+
Details | Diff | Splinter Review 
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!
is this still valid?
(In reply to Tyrone Chong from comment #1)
> is this still valid?

Yes.
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.
(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.
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!
I'll build it right now. How do i assign this bug to me?
Assigned.
Assignee: nobody → tyronekc
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Hardware: x86 → All
Version: unspecified → Trunk
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
(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).
Hey is there a specific text you want
Hey Paul, Mihai,

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

3 errors, 10 warnings

Mihai, do you think that covers correctly the information we have?
Flags: needinfo?(mihai.sucan)
(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.
Flags: needinfo?(mihai.sucan)
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?
As of right now I have it showing up as..

'N errors. Click to open developer toolbox.'
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 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.
Attachment #703181 - Flags: review?(mihai.sucan) → feedback+
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).
Submitted [REVISED] Patch with the following suggestions/changes you mentioned. Thanks!
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;
...
Attachment #703723 - Flags: review?(mihai.sucan)
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
Attachment #704395 - Flags: review?(mihai.sucan)
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.
Attachment #704395 - Flags: review?(mihai.sucan)
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
I was thinking about getting the defaultTooltipText when developer tool loads? then save that globally?
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 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.
Attachment #704770 - Flags: review?(mihai.sucan) → review+
Attached patch patch with tests — — Splinter Review 
Updated the patch to include tests.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=0479468b5b9d
Attachment #703181 - Attachment is obsolete: true
Attachment #703723 - Attachment is obsolete: true
Attachment #704395 - Attachment is obsolete: true
You're welcome, thank you for the guidance :D let me know if this patch gets accepted.
(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 :)
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/65494544bdeb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Is it just me or this string should have support plural forms?
https://developer.mozilla.org/en/docs/Localization_and_Plurals
Francesco: thank you for pointing out the problem. Reported bug 834721.
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: