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)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: automatedtester, Assigned: tyronekc)
Details
Attachments
(2 files, 3 obsolete files)
4.90 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
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!
Assignee | ||
Comment 1•11 years ago
|
||
is this still valid?
Comment 2•11 years ago
|
||
(In reply to Tyrone Chong from comment #1) > is this still valid? Yes.
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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!
Assignee | ||
Comment 6•11 years ago
|
||
I'll build it right now. How do i assign this bug to me?
Comment 7•11 years ago
|
||
Assigned.
Assignee: nobody → tyronekc
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 8•11 years ago
|
||
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•11 years ago
|
||
(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).
Assignee | ||
Comment 10•11 years ago
|
||
Hey is there a specific text you want
Assignee | ||
Comment 11•11 years ago
|
||
Hey Paul, Mihai, Is there any specific text we should put into the tooltip? Let me know thanks.
Comment 12•11 years ago
|
||
Maybe something like: 3 errors, 10 warnings Mihai, do you think that covers correctly the information we have?
Flags: needinfo?(mihai.sucan)
Comment 13•11 years ago
|
||
(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)
Assignee | ||
Comment 14•11 years ago
|
||
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?
Assignee | ||
Comment 15•11 years ago
|
||
As of right now I have it showing up as.. 'N errors. Click to open developer toolbox.'
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #703181 -
Flags: review?(mihai.sucan)
Comment 18•11 years ago
|
||
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+
Comment 19•11 years ago
|
||
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).
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #703723 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 21•11 years ago
|
||
Submitted [REVISED] Patch with the following suggestions/changes you mentioned. Thanks!
Comment 22•11 years ago
|
||
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)
Assignee | ||
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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)
Assignee | ||
Comment 25•11 years ago
|
||
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
Assignee | ||
Comment 26•11 years ago
|
||
I was thinking about getting the defaultTooltipText when developer tool loads? then save that globally?
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #704770 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 28•11 years ago
|
||
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•11 years ago
|
||
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+
Comment 30•11 years ago
|
||
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
Assignee | ||
Comment 31•11 years ago
|
||
You're welcome, thank you for the guidance :D let me know if this patch gets accepted.
Comment 32•11 years ago
|
||
(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•11 years ago
|
||
Comment on attachment 704908 [details] [diff] [review] patch with tests Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/65494544bdeb
Attachment #704908 -
Flags: checkin+
Updated•11 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 34•11 years ago
|
||
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
Comment 35•11 years ago
|
||
Is it just me or this string should have support plural forms? https://developer.mozilla.org/en/docs/Localization_and_Plurals
Comment 36•11 years ago
|
||
Francesco: thank you for pointing out the problem. Reported bug 834721.
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•