367596 - (about:support) Create about:support page with troubleshooting information (e.g. list of extensions)

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P3)

Description

[Chris Ilias [:cilias]]

As discussed at <http://groups.google.com/group/mozilla.dev.apps.firefox/browse_frm/thread/93d5c6843e14c2fd>...

Most users don't provide enough information about their setup, and we need to fish it out of them.  In many cases, we need to know what extensions are installed, and there's no way to simply copy and paste that info into a support post (They would need the info-lister extension). If I need a user to do something in their profile folder (eg. restore a bookmarks backup file), I can point them to a web page that lists the default locations, but the web page can't list the exact path for that specific user.  

As a method of improving user support tools, it would be nice if we (support volunteers) could direct users to a single about: page, that listed all the info a user needs for user support; and create a "Copy my support info" button on that page.

Originally intended as a way improving the about: page, it may be better to put this on a new 'about' URL. (about:support, about:setup, etc.)

Filed under Firefox:General, but if this could be done in the Toolkit, and make it so Thunderbird users could also utilize a support info-lister, that would be twice as nice. :-)

Some info that it would include:
- version
- user-agent sting
- installed add-ons (and their version numbers)
- installed plug-ins (and their version numbers)
- profile location
- and maybe the cache location

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Bug 362746 is about about:addons. I could have sworn another bug was already filed for this same request, but I can't find it.

Comment 2

[Peter Weilbacher]

Perhaps it's not even needed to display this stuff anywhere, for most support issues a "Copy my support info" button in the about-dialog or even a menu item with this text under Help could be enough. This would then just copy the info to the clipboard as plain text and maybe text/html.

Comment 3

[Dave Townsend [:mossop]]

This is something that'd be pretty simple to knock together and I tend to agree that just copying it to the clipboard in a simple text form is probably the best way to go, then users can just stick it onto pastebin or an email/forum.

I would add to the list of info the network settings including proxy.

Comment 4

[Chris Ilias [:cilias]]

I would still like an actual page. Using the example of teaching a user how to restore a bookmark backup, I can tell the user to the user to go to his/her profile folder, and direct them to the local support info page for the actual profile location, rather than a web site which lists the defaults. In such a case, copying/pasting text would just be an extra step.

Comment 5

[Percy Cabello]

I guess both should be provided:

- An easy "Copy support info" button in About Firefox or an item in the Help menu.
- A centralized informative page where all support related Firefox's details are displayed.

I think all customized preferences should be included as well. This covers comment 3.

Having a single page instead of several about:addons, about:plugins, about: buildconfig helps end users, while for developers its basically the same thing. Or are there extensions or any other kind of dependency on any of the currently available about:s?

This page should also have a printable format for the cases where a user can't access the web say at home, so s/he needs to take the information to a peer at work. Copied/printed sections are selectable.

Comment 6

[Dave Townsend [:mossop]]

All customised preferences is a bad idea I think, it would just make the report too long, particularly since to my knowledge there is no good way of separation out extension prefs.

Comment 7

[Percy Cabello]

Attachment: About:support mockup

I dubbed it about:system, but about:support, about:setup, etc. are all good names.

As stated in my previous comment, it's intended for informative purposes and so it contains lots of information. A user can select what specific sections to share (copy/print). Includes customized preferences and build details. And simple graph of the profile size which may aid users with hard disk space management (not sure if it may grow big enough to really matter).

Plugins are clickable and expand to provided MIME details (not in this mockup).

Comment 8

[Percy Cabello]

I also made the profile location clickable in the mockup (comment 7) which should ease other tasks like checking userChrome.js or user.js.

Comment 9

[Gruenhecke]

Dupe of bug 320141

Comment 10

[Chris Ilias [:cilias]]

In m.d.a.firefox, Gerv suggested about:diagnostics.

Bug 320141 looks like it is the same; but I don't want to ruin any momentum this bug may have, by resolving now. You know how Mozilla development is. Once a bug is forgotten, it's pretty tough to get it back in the limelight.

Comment 11

[Peter Weilbacher]

I really like Percy's mockup. Perhaps Dave's concern could be met by selecting only a few of the checkboxes at the top by default, and leave e.g. Customized Preferences and Profile deselected.

I am wondering if the full build info is really necessary. In most cases, just the build ID should be enough, right? In light of Bug 361734 we should make sure that the build ID listed on this page is actually the build ID and not a spoofed user-agent string (or list both).

Comment 12

[Chris Ilias [:cilias]]

In doing user support, I've never once needed to know build info, like build tools, and config arguments.

However, it is good to know when a user is spoofing their UA. That is often the cause of whatever problems they may be asking about. (For instance: bug 83376 )

Comment 13

[Chris Ilias [:cilias]]

Is there anyone capable of implementing this, that I can add to the CC list, or assign this bug to?

Comment 14

[Dave Townsend [:mossop]]

Assuming we have some kind of agreement about what to include, and that this is something that would be taken then I'd be happy to take this on. Copying in Mike, perhaps he has some useful comments to bring.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

I think that this is a great idea. Some initial thoughts ...

1) Something worries me in terms of both security/privacy and discoverability when I think about this being an about: page. Why not make it part of the Help dialog and actually pop a window which asks the user a minimal number of questions about the types of information they want to include, and for a destination output location (file? pastebin? copy buffer?).

2) It would be good to get information from people like Chris, Lucy, the crowd in #firefox, people on MozillaZine and other fora where Firefox Support is offered (apparently our Polish community is pretty kick-ass here) about:

 - what information should always be provided
 - what information should be provided on an opt-in basis
 - how that information could best be packaged & shared with the troubleshooter

Comment 16

[Jason Barnabe (np)]

I'd find the following useful
-Build identifier
-Extensions, with versions and enabled/disabled status
-Plugin information
-IDs of the last X incidents sent via Talkback
-The names of the profiles, including info on which is currently in use

I don't see any particular need for customized prefs or build flags.

Comment 17

[Chris Ilias [:cilias]]

(In reply to comment #16)
> I'd find the following useful
> -Build identifier
> -Extensions, with versions and enabled/disabled status
> -Plugin information
> -IDs of the last X incidents sent via Talkback
> -The names of the profiles, including info on which is currently in use
> 
> I don't see any particular need for customized prefs or build flags.

I agree with the above, and I would add the profile location(s).

Comment 18

[Ian]

Nightly Tester Tools add-On already does most of this so the code is written.

Comment 19

[Percy Cabello]

I don't have much experience with support but I'd think that knowing if pipelining or fast page navigation or some other about:config preferences has been tweaked or not may be helpful. A user may have activated some of those settings and just forgot about it or just don't relate a problem to the change. It has happened to myself.

Comment 20

[Chris Ilias [:cilias]]

Better safe than sorry. I'll agree with adding customized prefs. Can we agree on something, before it's too late to get this in Fx3?

Comment 21

[Paul Rouget [:paul]]

Such feature must be based on a dynamic mechanism. I thought it could be a good idea to have a set of "diagnostic modules" (addons, plugins, prefs, memory, ...). 

Each module must provide at least a method to retrieve a "text plain report", a "short name" and a "rich view". They should be registered in a category (catManager speaking).

Then, each "diagnostic modules" could be displayed through different ways:
- a "diagnostic:" url which will display something like the mockup. And a specific diagnostic could be reached from a "diagnostic:moduleShortName" url (diagnostic:addons, diagnostic:profile, ...).
- something working like the prefPane. A diagnosticPane could host diagnostic modules through the overlay mechanism.

Then, any extensions could provide their own diagnostic module.

I think such feature must be provided through an extension first, and it's ok for me to work on it.

What do you think about such idea ?

Comment 23

[David McRitchie]

Needs to be built-in to Firefox and available on any installation, in safe mode, without regard to any extensions installed.

Suggest:   
about:profile  provide live link to view profile directory in Firefox, 
  and a  link (or text link) suitable such as for Windows Explorer
  on a windows system.  So user can actually work with Firefox customizations
  and do their own backups in the manner that they prefer.

about:addons  provide list of extensions and themes

As far as an extensions providing information:
  "Infolister" extension, provides  about:info  (may be user customized)
  "Extension Manager Extended" provides additional information and links
   via context menus added within the Addons Manager. 
  
  

Comment 24

[David McRitchie]

Continuing my comment #23 for the about:profile part it should have a link
to get you into your profile so you can work with it within your system, a
working extension for Windows only, which works but is not listed as compatible is
  "Open Profile Folder" extension opens the profile folder in Windows File Manager

Comment 26

[Johnathan Nightingale [:johnath]]

I'm flagging this wanted-firefox3.6 because it will require string changes, but I don't think we should let it languish for another release.  It's a huge force multiplier to spend a moderate amount of developer time up front in order to create a more effective support organization.  I've also added it to the Firefox.next plan wiki discussion page.

Comment 28

[[:Cww]]

From a support point of view in terms of copy pasteable info, I'd like the following:

Extensions (including if they've been disabled and version numbers)
CrashIds + Dates

In terms of helping us with basic troubleshooting, the following buttons would be really awesome:

1) [Restart Firefox in Safe Mode]
2) [Open my Profile folder]

It's also very important that it be accessible from a menu.  Users have a LOT of trouble typing about:something into their location bar.

Comment 29

[Curtis Bartley [:cbartley]]

Some comments from beltzner:

I think that we might be able to find some goodness to get into Firefox 3.6 before Monday night's string freeze. It may mean landing the layout of about:support before it can actually pull in and format the data, but I think that's OK.

Looking at the list at https://wiki.mozilla.org/Support/Firefox.Next there's not a lot that I think we'd want to tackle for 3.6; all seems very string intensive, and very "self diagnostic" based which might be a little tough to do.

Instead, I think if we had about:support:

 - link to SUMO
 - list the installed plugins and add-ons in an easy to copy & paste textarea
 - list any prefs that have been changed from default or are custom in an easy to copy & paste textarea
 - list the installation history in an easy to copy & paste text area
 - list the buildconfig info in an easy to copy & paste text area

we'd have a good start. Might also want some simple stats like:

 - size of places DB
 - size of profile directory
 - age of profile directory
 - number of profiles?

Comment 30

[Curtis Bartley [:cbartley]]

Attachment: about:support screenshot showing extensions list

I've implemented a basic list of extensions.  The styling is cribbed from about:plugins.

My primary focus for the next couple of days is figuring out which features we want for 3.6 and, importantly, what wording we want for those features.  The latter is because string freeze for 3.6 is impending so we need to figure that out and finalize it as quickly as possible.

If you have opinions about layout and styling, I'd still like to hear them.  However, I may not be able to act on them right away.

Comment 31

[[:Cww]]

(In reply to comment #29)

Can I add: 
Location of profile directory (don't need a link to open for now)

Also for  
- list any prefs that have been changed from default or are custom in an easy to copy & paste textarea we'll need to sit down and decide what counts and what doesn't. We don't need to include all the prefs that are added by extensions or printing prefs or prefs that decide whether to show security or popups etc.

Comment 32

[Majken Connor [:Kensie]]

(In reply to comment #29)

> 
> Instead, I think if we had about:support:
> 
>  - link to SUMO
>  - list the installed plugins and add-ons in an easy to copy & paste textarea

Is there a benefit to making it a textarea? IMO I wouldn't want the user editing it, and if so, they can edit it wherever they're pasting it.

>  - list any prefs that have been changed from default or are custom in an easy
> to copy & paste textarea

IMO just a link to about:config and/or to open prefs.js and user.js would do, esp since listing them would make this area quite noisy. I'm not sure the context that Cww is looking at in not wanting the prefs from extensions, printing etc. Depending on the problem we will be able to rule out lots of different prefs that we wouldn't want to rule out in other cases. 

I would suggest that firefox itself include all the prefs for copying, and the filtering could be done at the support end. I.E. if the form the person is filling out has to do with a print problem, it would take all the prefs, but by default only display the print prefs.

>  - list the installation history in an easy to copy & paste text area
>  - list the buildconfig info in an easy to copy & paste text area
> 
> we'd have a good start. Might also want some simple stats like:
> 
I also want to add Location of profile. IMO this and listing add-ons in a copyable format are the most important inclusions. In a pinch we could probably use an "Open" button/link if we decide to implement that, but it'd be nicer to have "View profile" available.

Comment 33

[Johnathan Nightingale [:johnath]]

(In reply to comment #32)
> (In reply to comment #29)
> >  - list the installed plugins and add-ons in an easy to copy & paste textarea
> 
> Is there a benefit to making it a textarea? IMO I wouldn't want the user
> editing it, and if so, they can edit it wherever they're pasting it.

Not to scope creep, but this page will be privileged, so I think an entirely legitimate follow up bug, once the basic page is there, is to request a button at the top that says "copy everything that matters here to my clipboard" - easier and less error-prone.

For now, though, getting a tightly constrained set of functionality in before string freeze is priority one, and thanks to everyone helping get there (and restraining what I know are your much longer wishlists. :)

Comment 34

[Chris Ilias [:cilias]]

I still don't see the point in displaying the buildconfig info. The build ID is better.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

> (In reply to comment #29)
> Can I add: 
> Location of profile directory (don't need a link to open for now)

Good call. I missed an entire category of things around "how to help users find things on their system" like this.

> Also for  
> - list any prefs that have been changed from default or are custom in an easy
> to copy & paste textarea we'll need to sit down and decide what counts and what
> doesn't. We don't need to include all the prefs that are added by extensions or
> printing prefs or prefs that decide whether to show security or popups etc.

I don't think we want to do that ahead of time. Better to include everything that's not "standard" and that way when someone copies it in for SUMO wizards to look at, they're seeing the entire list of "what makes this not like a fresh version of Firefox with a fresh profile."

(In reply to comment #33)
> Not to scope creep, but this page will be privileged, so I think an entirely
> legitimate follow up bug, once the basic page is there, is to request a button
> at the top that says "copy everything that matters here to my clipboard" -
> easier and less error-prone.

Yes, the point was to make it easy to select all and copy. I intended for it to be a read-only textarea, but copy to clipboard is better.

(In reply to comment #34)
> I still don't see the point in displaying the buildconfig info. The build ID is
> better.

We should have both. I was presuming that the buildconfig info will help SUMO folks understand if it's a standard build or something that was done custom for users.(In reply to comment #31)

Comment 36

[Curtis Bartley [:cbartley]]

Attachment: about:support v1 (WIP)

mconnor: If you don't have time to look at this patch, feel free to pass it on to someone else.

This patch is "work in progress".  Normally I would not request formal review at this stage; however given the impending string freeze I want to identify any serious problems sooner rather than later.

Only some of the strings have been factored out into the DTD.  I'll work on that more tomorrow.  In the meantime, I'm concerned mostly about wording since we'll need to have that worked out before string freeze.  I'm concerned about design/layout inasmuch as it effects wording.

Comment 37

[Curtis Bartley [:cbartley]]

Attachment: about:support v1 screenshot

Comment 38

[Curtis Bartley [:cbartley]]

cilias says on IRC that we want the support link to go to support.mozilla.COM, rather than support.mozilla.org.

Comment 39

[Johnathan Nightingale [:johnath]]

It's great to see progress here!

I think the link to support should be broken out into its own top level introductory sentence, immediately below "about:support", basically saying:

This page contains technical information that might be useful when you're trying to solve a problem. If you are looking for answers to common questions about &brandShortName;, check out _support.mozilla.com_.

I bet that'll be wordsmithed.

I'd make the "Plugins" and "Build" labels slightly more descriptive ("Installed Plugins", "Build Configuration"). I like that you're linking to them for now, rather than blocking on getting all the pieces implemented here, for round 1.

The copy button being at the bottom is a layout thing that we can fix later, but we'll want to make sure that's easily findable for people.

Comment 40

[[:Cww]]

(In reply to comment #35)
>I don't think we want to do that ahead of time. Better to include everything
>that's not "standard" and that way when someone copies it in for SUMO wizards
>to look at, they're seeing the entire list of "what makes this not like a fresh
>version of Firefox with a fresh profile."

I disagree... we don't know what extensions are putting into prefs.js and there can be sensitive information there.  Knowing they have extensions should be enough, we don't need to know what those extension's settings are.  We can simply say "X extension" is known to cause problems, please disable.

Comment 41

[Ray Kiddy]

It is fantastic that this is being done.

Looking at the screen shot attachment from 09/11 (http://bugzilla.mozilla.org/attachment.cgi?id=399973), I have a suggestion about the top box. It can be smaller.

There is really no reason to have an entire line for each of the "Support Site", Plugins and Build URLs. Why take that much space to display a URL? Have a clickable link off to the side. People can see the URL by hovering over the link.

Thus, your top box would be half as tall. This would leave more room to have the other info be viewable more easily.

Comment 42

[[:Cww]]

Also one thing we'll have to string freeze on is what to call the help menu item.

Help > Show diagnostic information (or something)

Comment 43

[David Tenser [:djst]]

A menu item is definitely needed in order to make this easy to use. We could then use the title of that menu item as the title of the page as well (instead of "about:support").

I agree with Johnathan about moving the "copy to clipboard" button to the top, but we also need to pick a good format for how things should be copied to the clipboard in order to make this useful on SUMO. See my following attachment for how it should be formatted in order to be collapsible (so it doesn't occupy so much space by default).

(In reply to comment #38)
> cilias says on IRC that we want the support link to go to support.mozilla.COM,
> rather than support.mozilla.org.

Shouldn't we just be using the app.support.baseURL pref to get the support URL?

Comment 44

[David Tenser [:djst]]

Attachment: Formatted list of info for simple copy and paste on SUMO

This is the preferred format of the info. Headings are bold using "__" and list items start with a "*".

We will then put all this info under a collapsed-by-default box so it doesn't occupy that much space. Thanks!

Comment 45

[Curtis Bartley [:cbartley]]

Attachment: about:support v2 (WIP)

Notable changes in this patch:

Added subtitle/summary paragraph at the top with the link to support.mozilla.com, per Johnath's suggestion.  Removed NYI sections for "Installed Plugins" and "Build Configuration" since that functionality is essentially available from the existing links to about:plugins and about:buildconfig.

Added the menu item Help >> Show Diagnostics, which opens the about:support page in a new tab.

I've now factored all the strings out into entity definitions in the DTD file.

I've also messed with the layout and styling some, but you shouldn't assume this is anywhere near final.

Comment 46

[Curtis Bartley [:cbartley]]

Attachment: about:support v2 screenshot

Comment 47

[Johnathan Nightingale [:johnath]]

Comment on attachment 400124 [details] [diff] [review]
 about:support v2 (WIP) 

Not a review, just some super quick, mostly string-related nits...

> # The Initial Developer of the Original Code is
> # Curtis Bartley <[e-mail]>

The initial developer is actually Mozilla Corporation (they hold the copyright), you are the first contributor.

>+<!DOCTYPE html [
>+  <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd">  %htmlDTD;
>+  <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">  %netErrorDTD;
>+  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">  %globalDTD;
>+  <!ENTITY % aboutrobotsDTD SYSTEM "chrome://browser/locale/aboutSupport.dtd">  %aboutrobotsDTD;
>+]>

Fix the "robots" references.  :) 

>diff -r 7ed220eb3a9f browser/base/content/baseMenuOverlay.xul
>         <menuseparator id="updateSeparator"/>
>+        <menuitem id="showDiagnostics"
>+                  accesskey="&helpShowDiagnostics.accesskey;"
>+                  label="&helpShowDiagnostics.label;"
>+                  oncommand="openDiagnosticsPage()"
>+                  onclick="checkForMiddleClick(this, event);"/>
>+        <menuseparator id="updateSeparator"/>

New separator should have a better name (or, really, the one above your entry should)

>diff -r 7ed220eb3a9f browser/locales/en-US/chrome/browser/aboutSupport.dtd
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Fri Sep 11 12:34:01 2009 -0700
>@@ -0,0 +1,39 @@
>+<!ENTITY aboutSupport.pageTitle "about:support">
>+
>+<!-- LOCALIZATION NOTE (aboutSupport.pageSubtitle1):
>+     LOCALIZATION NOTE (aboutSupport.pageSubtitle2):
>+     LOCALIZATION NOTE (aboutSupport.pageSubtitle3):
>+       Usage is 
>+          &aboutSupport.pageSubtitle1;
>+          &brandShortName; 
>+          &aboutSupport.pageSubtitle2;
>+          <a href="http://support.mozilla.com">support.mozilla.com</a>
>+          &aboutSupport.pageSubtitle3;
>+-->
>+<!ENTITY aboutSupport.pageSubtitle1 "
>+  This page contains technical information that might be useful when 
>+  you're trying to solve a problem. If you are looking for answers to 
>+  common questions about
>+">
>+<!ENTITY aboutSupport.pageSubtitle2 ", check out">
>+<!ENTITY aboutSupport.pageSubtitle3 ".">

Entities are allowed to nest, so if you source brand.dtd in your page as well, you can simplify this drastically to just

<!ENTITY aboutSupport.subtitle "This page contains technical information that might be useful when you're trying to solve a problem. If you are looking for answers to common questions about &brandShortName;, check out our <a id='supportlink'>support web site</a>.">

with a localization note asking people not to change the 'supportlink' id. Then, as mentioned, you can just pull the value of the app.support.baseURL pref to populate the href in JS. Normally I'd say that generalizing to the pref value would be follow up work, but it impacts the string. 

>diff -r 7ed220eb3a9f browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd
>--- a/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd	Wed Sep 09 18:25:28 2009 -0400
>+++ b/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd	Fri Sep 11 12:34:01 2009 -0700
>@@ -17,16 +17,20 @@
> <!ENTITY helpContentsMac.label    "&brandShortName; Help">
> <!ENTITY helpForIEUsers.label     "For Internet Explorer Users">
> <!ENTITY helpForIEUsers.accesskey "I">
> <!ENTITY openHelp.commandkey      "VK_F1">
> <!ENTITY helpMac.commandkey       "?">
> 
> <!ENTITY helpReleaseNotes.label         "Release Notes">
> <!ENTITY helpReleaseNotes.accesskey     "N">
>+
>+<!ENTITY helpShowDiagnostics.label      "Show Diagnostics">
>+<!ENTITY helpShowDiagnostics.accesskey  "D">
>+

This menu item should have the ellipsis character after it, like Report Broken Web Site does. (Note, ellipsis character, not just three periods). Personally, I think "Show Diagnostics" is pretty jargony sounding, and would prefer something like "Tech Support Information..." but I don't want to go too far down the rabbit hole of wordsmithing. Either of those or some would-be third candidate is better than not making string freeze.

Comment 48

[Chris Ilias [:cilias]]

"Show Diagnostics" is definitely not indicative, because it implies that tests were run. This is more like "Profile information..."

Comment 49

[Curtis Bartley [:cbartley]]

(In reply to comment #48)
> "Show Diagnostics" is definitely not indicative, because it implies that tests
> were run. This is more like "Profile information..."

I was thinking about "Technical Information..." but I'm not sure that's right either.

Comment 50

[Curtis Bartley [:cbartley]]

Attachment: about:support v3 (WIP)

(In reply to comment #47)
> (From update of attachment 400124 [details] [diff] [review])
> Not a review, just some super quick, mostly string-related nits...
> 
> > # The Initial Developer of the Original Code is
> > # Curtis Bartley <[e-mail]>
> 
> The initial developer is actually Mozilla Corporation (they hold the
> copyright), you are the first contributor.


Fixed.


> >+<!DOCTYPE html [
> >+  <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xh...
> >+  <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd"> ...
> >+  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">  %gl...
> >+  <!ENTITY % aboutrobotsDTD SYSTEM "chrome://browser/locale/aboutSuppo...
> >+]>
> 
> Fix the "robots" references.  :) 


Fixed.


> >diff -r 7ed220eb3a9f browser/base/content/baseMenuOverlay.xul
> >         <menuseparator id="updateSeparator"/>
> >+        <menuitem id="showDiagnostics"
> >+                  accesskey="&helpShowDiagnostics.accesskey;"
> >+                  label="&helpShowDiagnostics.label;"
> >+                  oncommand="openDiagnosticsPage()"
> >+                  onclick="checkForMiddleClick(this, event);"/>
> >+        <menuseparator id="updateSeparator"/>
> 
> New separator should have a better name (or, really, the one above your entry
> should)


Fixed.


> >diff -r 7ed220eb3a9f browser/locales/en-US/chrome/browser/aboutSupport.dtd
> >--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >+++ b/browser/locales/en-US/chrome/browser/aboutSupport.dtd	Fri Sep 11 ...
> >@@ -0,0 +1,39 @@
> >+<!ENTITY aboutSupport.pageTitle "about:support">
> >+
> >+<!-- LOCALIZATION NOTE (aboutSupport.pageSubtitle1):
> >+     LOCALIZATION NOTE (aboutSupport.pageSubtitle2):
> >+     LOCALIZATION NOTE (aboutSupport.pageSubtitle3):
> >+       Usage is 
> >+          &aboutSupport.pageSubtitle1;
> >+          &brandShortName; 
> >+          &aboutSupport.pageSubtitle2;
> >+          <a href="http://support.mozilla.com">support.mozilla.com</a>
> >+          &aboutSupport.pageSubtitle3;
> >+-->
> >+<!ENTITY aboutSupport.pageSubtitle1 "
> >+  This page contains technical information that might be useful when 
> >+  you're trying to solve a problem. If you are looking for answers to 
> >+  common questions about
> >+">
> >+<!ENTITY aboutSupport.pageSubtitle2 ", check out">
> >+<!ENTITY aboutSupport.pageSubtitle3 ".">
> 
> Entities are allowed to nest, so if you source brand.dtd in your page as well,
> you can simplify this drastically to just
> 
> <!ENTITY aboutSupport.subtitle "This page contains technical information that
> might be useful when you're trying to solve a problem. If you are looking for
> answers to common questions about &brandShortName;, check out our <a
> id='supportlink'>support web site</a>.">
> 
> with a localization note asking people not to change the 'supportlink' id.
> Then, as mentioned, you can just pull the value of the app.support.baseURL...
> to populate the href in JS. Normally I'd say that generalizing to the pref
> value would be follow up work, but it impacts the string. 


Fixed (except I'm not using the pref yet).


> >diff -r 7ed220eb3a9f browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd
> >--- a/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd Wed Sep 09...
> >+++ b/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd Fri Sep 11...
> >@@ -17,16 +17,20 @@
> > <!ENTITY helpContentsMac.label    "&brandShortName; Help">
> > <!ENTITY helpForIEUsers.label     "For Internet Explorer Users">
> > <!ENTITY helpForIEUsers.accesskey "I">
> > <!ENTITY openHelp.commandkey      "VK_F1">
> > <!ENTITY helpMac.commandkey       "?">
> > 
> > <!ENTITY helpReleaseNotes.label         "Release Notes">
> > <!ENTITY helpReleaseNotes.accesskey     "N">
> >+
> >+<!ENTITY helpShowDiagnostics.label      "Show Diagnostics">
> >+<!ENTITY helpShowDiagnostics.accesskey  "D">
> >+
> 
> This menu item should have the ellipsis character after it, like Report Broken
> Web Site does. (Note, ellipsis character, not just three periods). Personally,


Fixed.


> I think "Show Diagnostics" is pretty jargony sounding, and would prefer
> something like "Tech Support Information..." but I don't want to go too far
> down the rabbit hole of wordsmithing. Either of those or some would-be third
> candidate is better than not making string freeze.


Maybe just "Technical Information..."  But maybe that doesn't have as good discoverability.

Comment 51

[Curtis Bartley [:cbartley]]

(In reply to comment #50)
> Created an attachment (id=400162) [details]
> about:support v3 (WIP)
> 
> (In reply to comment #47)
> 
> 
> > I think "Show Diagnostics" is pretty jargony sounding, and would prefer
> > something like "Tech Support Information..." but I don't want to go too far
> > down the rabbit hole of wordsmithing. Either of those or some would-be third
> > candidate is better than not making string freeze.
> 
> 
> Maybe just "Technical Information..."  But maybe that doesn't have as good
> discoverability.

Beltzner suggests "Troubleshooting Information..." since that's descriptive of what were trying to do for 3.6.  If we turn it into a true diagnostic panel later, we can always rename it then.

Comment 52

[Johnathan Nightingale [:johnath]]

(In reply to comment #51)
> Beltzner suggests "Troubleshooting Information..." since that's descriptive of
> what were trying to do for 3.6.  If we turn it into a true diagnostic panel
> later, we can always rename it then.

+1

Comment 53

[Curtis Bartley [:cbartley]]

Attachment: about:support v4 (WIP)

The menu item is now called "Troubleshooting Information...".

The link to the support website now uses the "app.support.baseURL" pref.

Comment 54

[Curtis Bartley [:cbartley]]

Attachment: about:support v4 screenshot

Comment 55

[Curtis Bartley [:cbartley]]

Attachment: about:support v5 (WIP)

Notable changes in this patch:
* Implemented Non-default Preferences
* Moved the copy-to-clipboard button to the upper right corner of the page
* The page title is now "Troubleshooting Information" mirroring the Help
    menu item.  The page still appears as "about:support" in the address bar,
    however.
* Did some CSS and JavaScript cleanup.

There may not be time to implement "Installation History" and "Update History" before string freeze.  Should we throw together some static mocks in case there are a few more localized strings we'll want related to those features?

Comment 56

[Curtis Bartley [:cbartley]]

Attachment: about:support v5 screenshot

Comment 57

[Majken Connor [:Kensie]]

(In reply to comment #40)
> (In reply to comment #35)
> >I don't think we want to do that ahead of time. Better to include everything
> >that's not "standard" and that way when someone copies it in for SUMO wizards
> >to look at, they're seeing the entire list of "what makes this not like a fresh
> >version of Firefox with a fresh profile."
> 
> I disagree... we don't know what extensions are putting into prefs.js and there
> can be sensitive information there.  Knowing they have extensions should be
> enough, we don't need to know what those extension's settings are.  We can
> simply say "X extension" is known to cause problems, please disable.

That makes sense specifically for SUMO's purposes, but IMO we should think beyond SUMO when making a user visable page. Add-on authors would certainly like to know if a specific pref from their extension is causing the problem so that they can fix that.  Users would also rather be told to flip the pref rather than disable the add-on entirely. If this information is excluded at the app level rather than on the site level then those cases (extension authors troubleshooting) don't get to benefit.

Comment 58

[Johnathan Nightingale [:johnath]]

Comment on attachment 400409 [details] [diff] [review]
about:support v5 (WIP)

>diff -r 6f365b3614e6 browser/base/content/aboutSupport.xhtml
>+<!DOCTYPE html [
>+  <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd">  %htmlDTD;
>+  <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">  %netErrorDTD;

Are you using anything in netError.dtd? If not, drop it. Also, you should be sourcing chrome://branding/locale/brand.dtd to get &brandShortName;

>+    <style type="text/css"><![CDATA[

Mostly skimmed the CSS here - the screenshot looks like about:plugins, don't see any glaring issues, except:

>+      button.copy-to-clipboard {
>+        float: right;
>+      }

This button looks misaligned with, and almost clips, the title of the page. I'd suggest just putting it left-aligned below the intro paragraph (and hence, above all the information it will be copying.

>+        // Get the FUEL Application object.
>+        var cc = Components.classes;
>+        var ci = Components.interfaces;
>+        var Application =
>+            cc["@mozilla.org/fuel/application;1"].getService(ci.fuelIApplication);

These can all be const, right? Cc and Ci, to match our other source in browser/ please.

>+        // Get the support URL.
>+        var urlFormatter = cc["@mozilla.org/toolkit/URLFormatterService;1"]
>+                               .getService(ci.nsIURLFormatter);
>+        var supportUrl = urlFormatter.formatURLPref("app.support.baseURL");
>+
>+        // Get the profile directory.
>+        var propertiesService =
>+            cc["@mozilla.org/file/directory_service;1"].getService(ci.nsIProperties);

Your indenting on chained xpcom calls is non-standard, and varies within the file. We're not even close to consistent throughout browser, but reasonable arguments have been made that "Cc on first line, chained calls indenting so that leading dots align with square brackets" is at least as good as the other options, e.g.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#118

>+        // Show the list of extensions.        
>+        var trExtensions = createExtensionElements(Application.extensions.all);
>+        appendChildren(document.getElementById("extensions-tbody"), trExtensions);
>+
>+        // Show the list of modified preferences.
>+        var prefsToShow = selectModifiedPrefs(Application.prefs.all);
>+        var prefsToShowSorted = sortPreferences(prefsToShow);
>+        var trPrefs = createModifiedPreferenceElements(prefsToShowSorted);
>+        appendChildren(document.getElementById("prefs-tbody"), trPrefs);

Why write helper methods that do most of the work but then return to init() to appendChildren? Why not just just do:

populateExtensionsSection(...)
populatePreferencesSection(...)

and let the creation and appending of elements happen in the same place?

>+      }
>+
>+      function createExtensionElements(extensions)
>+      {
>+        var trExtensions = [];
>+        for (var i = 0; i < extensions.length; i++) {
>+          trExtensions.push(createExtensionElement(extensions[i]));
>+        }
>+        return trExtensions;
>+      }
>+
>+      function createExtensionElement(extension)
>+      {
>+        var tr = createParentElement("tr", [
>+          createElement("td", extension.name),
>+          createElement("td", extension.version),
>+          createElement("td", extension.enabled),
>+          createElement("td", extension.firstRun),
>+          createElement("td", extension.id),
>+        ]);
>+        return tr;
>+      }

Not sure that createExtensionElement does complicated enough things that it needs to be broken out - doing this right in the createExtensionElements loop would be more readable, at least to me. Particularly when combined with the appendChildren call, this would then be a single function that did a clear thing.

>+      function selectModifiedPrefs(prefs)
>+      {
>+        var modifiedPrefs = [];
>+        for (var i = 0; i < prefs.length; i++) {
>+          if (prefs[i].modified) {
>+            modifiedPrefs.push(prefs[i]);
>+          } 
>+        }
>+
>+        return modifiedPrefs;
>+      }

Does this introduce lag during page load? I would expect iterating over every pref might. If the page comes up in a second or less, I don't really care, it's not in a critical startup/pageload path. But if this takes 20 seconds on a slow machine, we'll have to design some kind of "please hold" feedback. That can be a follow up, if so, I just want to know if one is needed.

>+      <button class="copy-to-clipboard" onclick="alert('NYI');">

Clearly that's no good.

>+        &aboutSupport.copytToClipboard.label;

Typo "copytTo" both here and in the dtd.

>+            <td>
>+              <a href="about:plugins">about:plugins</a>
>+            </td>
...
>+            <td>
>+              <a href="about:buildconfig">about:buildconfig</a>
>+            </td>

Confirm that these links work? Some about pages don't like being linked to, but I think that restriction is content only, so you should be fine...

>+
>+      <h2 class="major-section">
>+        &aboutSupport.installationHistoryTitle;
>+      </h2>
>+      
>+      <div>
>+        <i>NYI</i>
>+      </div>
>+
>+      <!-- - - - - - - - - - - - - - - - - - - - - -->
>+
>+      <h2 class="major-section">
>+        &aboutSupport.updateHistoryTitle;
>+      </h2>
>+      
>+      <div>
>+        <i>NYI</i>
>+      </div>

I'd just nix these sections from the xhtml and file follow-ups (but land the strings, just in case).

>diff -r 6f365b3614e6 browser/base/content/baseMenuOverlay.xul
>@@ -92,16 +92,22 @@
>                   accesskey="&helpForIEUsers.accesskey;"
>                   oncommand="openHelpLink('ieusers');"/>
> #endif
>         <menuitem id="releaseNotes"
>                   accesskey="&helpReleaseNotes.accesskey;"
>                   label="&helpReleaseNotes.label;"
>                   oncommand="openReleaseNotes()"
>                   onclick="checkForMiddleClick(this, event);"/>
>+        <menuseparator id="troubleshootingSeparator"/>
>+        <menuitem id="troubleShooting"
>+                  accesskey="&helpTroubleshooting.accesskey;"
>+                  label="&helpTroubleshooting.label;"
>+                  oncommand="openTroubleshootingPage()"
>+                  onclick="checkForMiddleClick(this, event);"/>
>         <menuseparator id="updateSeparator"/>

This positioning, through no fault of your own, has the confusing effect (visible in your screenshot) of inserting yourself between "report broken website" and "report web forgery". I'd move it up to appear directly beneath "Firefox Help". I think this is consistent with the weekend conversation you had with beltzner, as well.

>diff -r 6f365b3614e6 browser/base/jar.mn
>+<!ENTITY aboutSupport.pageTitle "Troubleshooting Information">
>+
>+<!-- LOCALIZATION NOTE (aboutSupport.pageSubtitle): don't change the 'supportLink' id. -->
>+<!ENTITY aboutSupport.pageSubtitle "
>+  This page contains technical information that might be useful when you're 
>+  trying to solve a problem. If you are looking for answers to common questions
>+  about brandShortName, check out our <a id='supportLink'>support web site</a>.
>+">

brandShortName -> &brandShortName;  :)

>+<!ENTITY aboutSupport.extensionId "Id">

ID or id, please - "Id" is the pop-psychology counterpart to the "ego" and Freud has No Place in our source tree.

>+<!ENTITY aboutSupport.appBasicsTitle "Application Basics">
>+<!ENTITY aboutSupport.appBasicsName "Name">
>+<!ENTITY aboutSupport.appBasicsVersion "Version">
>+<!ENTITY aboutSupport.appBasicsProfileDir "Profile Directory">
>+<!ENTITY aboutSupport.appBasicsPlugins "Installed Plugins">
>+<!ENTITY aboutSupport.appBasicsBuildConfig "Build Configuration">
>+
>+<!ENTITY aboutSupport.nonDefaultPrefsTitle "Non-default preferences">

Most of your titles have been Title Case, this one should be, too.

>+<!ENTITY aboutSupport.installationHistoryTitle "Installation History">
>+<!ENTITY aboutSupport.updateHistoryTitle "Update History">

As I said above, I'd keep these in even if we don't land the sections in the first pass.

>+<!ENTITY aboutSupport.copytToClipboard.label "Copy this information to the clipboard">

Reminder to fix the typo.

r- because this version can't land: l10n issues, unimplemented features, Freud, &c. But it's close.

Comment 59

[David Tenser [:djst]]

(In reply to comment #58)
> This button looks misaligned with, and almost clips, the title of the page. I'd
> suggest just putting it left-aligned below the intro paragraph (and hence,
> above all the information it will be copying.

+1

Really great to see all the amazing progress on this!

Comment 60

[Robert Kaiser]

Comment on attachment 400409 [details] [diff] [review]
about:support v5 (WIP)

>+++ b/browser/base/content/aboutSupport.xhtml	Sun Sep 13 17:23:35 2009 -0700

Hmm, would it make sense to put this into toolkit, so other apps like Thunderbird and SeaMonkey could also use it in some way?

>+    <style type="text/css"><![CDATA[

Ugh, can't you put this in a theme .css file so that it's accessible to theme authors, just like about: or about:plugins are?

Comment 61

[Johnathan Nightingale [:johnath]]

(In reply to comment #60)
> (From update of attachment 400409 [details] [diff] [review])
> >+++ b/browser/base/content/aboutSupport.xhtml	Sun Sep 13 17:23:35 2009 -0700
> 
> Hmm, would it make sense to put this into toolkit, so other apps like
> Thunderbird and SeaMonkey could also use it in some way?

In a follow up bug, absolutely - but right now toolkit is string frozen whereas browser is not. Swift action in browser gets this feature in for Firefox 3.6 users, where it will no doubt see certain evolutions. I definitely think we should move it, though - you want to file that bug?

Comment 62

[Vlado Valastiak [:wladow]]

(In reply to comment #61)
> In a follow up bug, absolutely - but right now toolkit is string frozen whereas
> browser is not.
The fact is that the only consumers of 1.9.2 toolkit are firefox 3.6 and fennec 1.0, none of them were already announced to be firmly string frozen, there's no opt-in thread in .l10n for these. So if string frozen toolkit is the only reason to land it to browser and put it into toolkit later, I'd suggest to put into toolkit just right now. This way you'll also avoid unnessecary l10n bussiness with moving files in 70+ localizations.
 
Pike?

Comment 63

[Robert Kaiser]

Johnath, OK, I fully see the "get something in for Firefox 3.6" argument, I was mostly thinking in trunk terms here, where doing it right is more relevant than release schedules. And Thunderbird as well as SeaMonkey might just skip 1.9.2 and directly go 1.9.3 in any case. What Vlado says is still a valid concern, though.

Comment 64

[Axel Hecht [:Pike]]

I don't want more string freeze breaks. The opt-in thread for fennec doesn't have anything to do with that.

Comment 65

[Curtis Bartley [:cbartley]]

Attachment: about:support v6

(In reply to comment #58)
> (From update of attachment 400409 [details] [diff] [review])
> >diff -r 6f365b3614e6 browser/base/content/aboutSupport.xhtml
> >+<!DOCTYPE html [
> >+  <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd">  %htmlDTD;
> >+  <!ENTITY % netErrorDTD SYSTEM "chrome://global/locale/netError.dtd">  %netErrorDTD;
> 
> Are you using anything in netError.dtd? If not, drop it. Also, you should be
> sourcing chrome://branding/locale/brand.dtd to get &brandShortName;


Removed netError.dtd, brought in brand.dtd.


> >+    <style type="text/css"><![CDATA[
> 
> Mostly skimmed the CSS here - the screenshot looks like about:plugins, don't
> see any glaring issues, except:
> 
> >+      button.copy-to-clipboard {
> >+        float: right;
> >+      }
> 
> This button looks misaligned with, and almost clips, the title of the page. I'd
> suggest just putting it left-aligned below the intro paragraph (and hence,
> above all the information it will be copying.


Done.


> >+        // Get the FUEL Application object.
> >+        var cc = Components.classes;
> >+        var ci = Components.interfaces;
> >+        var Application =
> >+            cc["@mozilla.org/fuel/application;1"].getService(ci.fuelIApplication);
> 
> These can all be const, right? Cc and Ci, to match our other source in browser/
> please.


Done.


> >+        // Get the support URL.
> >+        var urlFormatter = cc["@mozilla.org/toolkit/URLFormatterService;1"]
> >+                               .getService(ci.nsIURLFormatter);
> >+        var supportUrl = urlFormatter.formatURLPref("app.support.baseURL");
> >+
> >+        // Get the profile directory.
> >+        var propertiesService =
> >+            cc["@mozilla.org/file/directory_service;1"].getService(ci.nsIProperties);
> 
> Your indenting on chained xpcom calls is non-standard, and varies within the
> file. We're not even close to consistent throughout browser, but reasonable
> arguments have been made that "Cc on first line, chained calls indenting so
> that leading dots align with square brackets" is at least as good as the other
> options, e.g.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#118


Done.


> >+        // Show the list of extensions.        
> >+        var trExtensions = createExtensionElements(Application.extensions.all);
> >+        appendChildren(document.getElementById("extensions-tbody"), trExtensions);
> >+
> >+        // Show the list of modified preferences.
> >+        var prefsToShow = selectModifiedPrefs(Application.prefs.all);
> >+        var prefsToShowSorted = sortPreferences(prefsToShow);
> >+        var trPrefs = createModifiedPreferenceElements(prefsToShowSorted);
> >+        appendChildren(document.getElementById("prefs-tbody"), trPrefs);
> 
> Why write helper methods that do most of the work but then return to init() to
> appendChildren? Why not just just do:
> 
> populateExtensionsSection(...)
> populatePreferencesSection(...)
> 
> and let the creation and appending of elements happen in the same place?


Done.


> >+      }
> >+
> >+      function createExtensionElements(extensions)
> >+      {
> >+        var trExtensions = [];
> >+        for (var i = 0; i < extensions.length; i++) {
> >+          trExtensions.push(createExtensionElement(extensions[i]));
> >+        }
> >+        return trExtensions;
> >+      }
> >+
> >+      function createExtensionElement(extension)
> >+      {
> >+        var tr = createParentElement("tr", [
> >+          createElement("td", extension.name),
> >+          createElement("td", extension.version),
> >+          createElement("td", extension.enabled),
> >+          createElement("td", extension.firstRun),
> >+          createElement("td", extension.id),
> >+        ]);
> >+        return tr;
> >+      }
> 
> Not sure that createExtensionElement does complicated enough things that it
> needs to be broken out - doing this right in the createExtensionElements loop
> would be more readable, at least to me. Particularly when combined with the
> appendChildren call, this would then be a single function that did a clear
> thing.


Done.


> >+      function selectModifiedPrefs(prefs)
> >+      {
> >+        var modifiedPrefs = [];
> >+        for (var i = 0; i < prefs.length; i++) {
> >+          if (prefs[i].modified) {
> >+            modifiedPrefs.push(prefs[i]);
> >+          } 
> >+        }
> >+
> >+        return modifiedPrefs;
> >+      }
> 
> Does this introduce lag during page load? I would expect iterating over every
> pref might. If the page comes up in a second or less, I don't really care, it's
> not in a critical startup/pageload path. But if this takes 20 seconds on a slow
> machine, we'll have to design some kind of "please hold" feedback. That can be
> a follow up, if so, I just want to know if one is needed.


Not only does it take about 5 seconds to load on my machine, it seems to get slower every time you reload.  I don't know why that is...


> >+      <button class="copy-to-clipboard" onclick="alert('NYI');">
> 
> Clearly that's no good.


Done.  Note that pasting as text looks like crap.  Paste as HTML seems to work pretty well, though.


> >+        &aboutSupport.copytToClipboard.label;
> 
> Typo "copytTo" both here and in the dtd.


Fixed.


> >+            <td>
> >+              <a href="about:plugins">about:plugins</a>
> >+            </td>
> ...
> >+            <td>
> >+              <a href="about:buildconfig">about:buildconfig</a>
> >+            </td>
> 
> Confirm that these links work? Some about pages don't like being linked to, but
> I think that restriction is content only, so you should be fine...


They work.


> >+
> >+      <h2 class="major-section">
> >+        &aboutSupport.installationHistoryTitle;
> >+      </h2>
> >+      
> >+      <div>
> >+        <i>NYI</i>
> >+      </div>
> >+
> >+      <!-- - - - - - - - - - - - - - - - - - - - - -->
> >+
> >+      <h2 class="major-section">
> >+        &aboutSupport.updateHistoryTitle;
> >+      </h2>
> >+      
> >+      <div>
> >+        <i>NYI</i>
> >+      </div>
> 
> I'd just nix these sections from the xhtml and file follow-ups (but land the
> strings, just in case).


Done:
* Bug 516616 - Add an "Installation History" section to about:support.
* Bug 516617 - Add an "Update History" section to about:support.


> 
> >diff -r 6f365b3614e6 browser/base/content/baseMenuOverlay.xul
> >@@ -92,16 +92,22 @@
> >                   accesskey="&helpForIEUsers.accesskey;"
> >                   oncommand="openHelpLink('ieusers');"/>
> > #endif
> >         <menuitem id="releaseNotes"
> >                   accesskey="&helpReleaseNotes.accesskey;"
> >                   label="&helpReleaseNotes.label;"
> >                   oncommand="openReleaseNotes()"
> >                   onclick="checkForMiddleClick(this, event);"/>
> >+        <menuseparator id="troubleshootingSeparator"/>
> >+        <menuitem id="troubleShooting"
> >+                  accesskey="&helpTroubleshooting.accesskey;"
> >+                  label="&helpTroubleshooting.label;"
> >+                  oncommand="openTroubleshootingPage()"
> >+                  onclick="checkForMiddleClick(this, event);"/>
> >         <menuseparator id="updateSeparator"/>
> 
> This positioning, through no fault of your own, has the confusing effect
> (visible in your screenshot) of inserting yourself between "report broken
> website" and "report web forgery". I'd move it up to appear directly beneath
> "Firefox Help". I think this is consistent with the weekend conversation you
> had with beltzner, as well.


Done.

 
> >diff -r 6f365b3614e6 browser/base/jar.mn
> >+<!ENTITY aboutSupport.pageTitle "Troubleshooting Information">
> >+
> >+<!-- LOCALIZATION NOTE (aboutSupport.pageSubtitle): don't change the 'supportLink' id. -->
> >+<!ENTITY aboutSupport.pageSubtitle "
> >+  This page contains technical information that might be useful when you're 
> >+  trying to solve a problem. If you are looking for answers to common questions
> >+  about brandShortName, check out our <a id='supportLink'>support web site</a>.
> >+">
> 
> brandShortName -> &brandShortName;  :)


Done.

 
> >+<!ENTITY aboutSupport.extensionId "Id">
> 
> ID or id, please - "Id" is the pop-psychology counterpart to the "ego" and
> Freud has No Place in our source tree.


Done.


> >+<!ENTITY aboutSupport.appBasicsTitle "Application Basics">
> >+<!ENTITY aboutSupport.appBasicsName "Name">
> >+<!ENTITY aboutSupport.appBasicsVersion "Version">
> >+<!ENTITY aboutSupport.appBasicsProfileDir "Profile Directory">
> >+<!ENTITY aboutSupport.appBasicsPlugins "Installed Plugins">
> >+<!ENTITY aboutSupport.appBasicsBuildConfig "Build Configuration">
> >+
> >+<!ENTITY aboutSupport.nonDefaultPrefsTitle "Non-default Preferences">
> 
> Most of your titles have been Title Case, this one should be, too.


Fixed.


> >+<!ENTITY aboutSupport.installationHistoryTitle "Installation History">
> >+<!ENTITY aboutSupport.updateHistoryTitle "Update History">
> 
> As I said above, I'd keep these in even if we don't land the sections in the
> first pass.


Still there.


> >+<!ENTITY aboutSupport.copytToClipboard.label "Copy this information to the clipboard">
> 
> Reminder to fix the typo.


Done.

 
> r- because this version can't land: l10n issues, unimplemented features, Freud,
> &c. But it's close.

Comment 66

[Curtis Bartley [:cbartley]]

Attachment: about:support v6 screenshot

Comment 67

[Curtis Bartley [:cbartley]]

Attachment: about:support v6 - strings only

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 400644 [details] [diff] [review]
about:support v6 - strings only

>+<!ENTITY aboutSupport.nonDefaultPrefsTitle "Non-default Preferences">

^ NIT: "Modified Preferences" sounds a bit better to me, and will likely translate a little easier. You can just adjust that on check-in.

>+<!ENTITY aboutSupport.copyToClipboard.label "Copy this information to the clipboard">

^ NIT: I worry that this will be prohibitively long in German and other languages. Either change it to "Copy all to clipboard" or add a localization note that lets l10n know that they can shorten as they see fit. I think the former is easier, but leave it up to you.

Great work. r+uir+a192=beltzner

Comment 69

[Curtis Bartley [:cbartley]]

Attachment: about:support v7

(In reply to comment #68)
> (From update of attachment 400644 [details] [diff] [review])
> >+<!ENTITY aboutSupport.nonDefaultPrefsTitle "Non-default Preferences">
> 
> ^ NIT: "Modified Preferences" sounds a bit better to me, and will likely
> translate a little easier. You can just adjust that on check-in.


Done.


> >+<!ENTITY aboutSupport.copyToClipboard.label "Copy this information to the clipboard">
> 
> ^ NIT: I worry that this will be prohibitively long in German and other
> languages. Either change it to "Copy all to clipboard" or add a localization
> note that lets l10n know that they can shorten as they see fit. I think the
> former is easier, but leave it up to you.


Done.



> Great work. r+uir+a192=beltzner

Comment 70

[Curtis Bartley [:cbartley]]

Attachment: about:support v7 - strings only

Comment 71

[Curtis Bartley [:cbartley]]

Attachment: about:support v7 screenshot

Comment 72

[Justin Dolske [:Dolske]]

Comment on attachment 400663 [details] [diff] [review]
about:support v7 - strings only

Checked in this strings-only patch for string freeze.

Pushed http://hg.mozilla.org/mozilla-central/rev/fcb8940bb443

and

Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b9dc3be962c

Comment 73

[[not reading bugmail]]

So we're not going to add the Default UA string as mentioned by comment 11 and comment 12 to this page and as requested to make "support easier" with bug 361734?  

Has adding about:crashes been discussed?  I would think a link at the bottom somewhere would be helpful for bugzilla support.

Comment 74

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #73)
> So we're not going to add the Default UA string as mentioned by comment 11 and
> comment 12 to this page and as requested to make "support easier" with bug
> 361734?

If the default UA string is modified by preferences, they'll show up. Further, there's a link to about:buildconfig which contains the UA string.
  
> Has adding about:crashes been discussed?  I would think a link at the bottom
> somewhere would be helpful for bugzilla support.

That's not the purpose of this page. It's meant to be, in this first pass, a single place where someone can go to get the common diagnostic information that they're asked for when lodging a problem report. In the future it will hopefully contain self-diagnostics and repair tools.

Comment 75

[Peter Weilbacher]

about:buildconfig does not contain the UA string (at least in the newest 1.9.2 nightly it still doesn't) but from the info in about:support and about:buildconfig one can probably get all the information that the original UA string would give. The application name and version are the original ones or would they be broken when the UA string is forged (not sure what the fuelIApplication interface provides)?

Comment 76

[[not reading bugmail]]

(In reply to comment #74)
> 
> If the default UA string is modified by preferences, they'll show up. 

True.  

> Further, there's a link to about:buildconfig which contains the UA string.
> 

I don't see it on the latest nightly either in about:buildconfig, which is why I was asking.  I was just thinking how to take care of a few birds with one stone (ie the mentioned requests), by having the original UA here might be of use [I was thinking like in () next the version number or something].  Though if it should be on about:buildconfig, where did it go and is there a bug # for this?

> > Has adding about:crashes been discussed?  I would think a link at the bottom
> > somewhere would be helpful for bugzilla support.
> 
> That's not the purpose of this page. It's meant to be, in this first pass, a
> single place where someone can go to get the common diagnostic information that
> they're asked for when lodging a problem report. In the future it will
> hopefully contain self-diagnostics and repair tools.

Sounds good.  Thanks.

(In reply to comment #75)
> about:buildconfig does not contain the UA string (at least in the newest 1.9.2
> nightly it still doesn't) 

I was just looking yesterday at bug numbers that show the original navigator.* and their corresponding override prefs to reference for your comment (though I don't know if their still valid prefs).  

> but from the info in about:support and about:buildconfig one can probably get > all the information that the original UA string would give. The application 
> name and version are the original ones or would they be broken when the UA 
> string is forged (not sure what the fuelIApplication interface provides)?

Though I'd rather we just build and display the default UA somewhere and not try listing individual defaults or override prefs which already confuse people during the support process, hense the requests to always display the default UA in its entirety to make less work for everyone which is basically the purpose of bug 361734.

Comment 77

[Alexander L. Slovesnik]

> <!ENTITY aboutSupport.extensionFirstRun "FirstRun">
Can someone explain what this string means? I have no idea how to translate it.

Comment 78

[Curtis Bartley [:cbartley]]

(In reply to comment #77)
> > <!ENTITY aboutSupport.extensionFirstRun "FirstRun">
> Can someone explain what this string means? I have no idea how to translate it.

"Indicates whether this is the extension's first run after install"

I think this is to facilitate extensions that want to show an introductory page the first time they run after they've been installed.

It's probably a legitimate question as to whether this is really useful on the about:support page.  However, the data's there so I figured it wouldn't hurt to include it.

Comment 79

[Johnathan Nightingale [:johnath]]

(In reply to comment #78)
> (In reply to comment #77)
> > > <!ENTITY aboutSupport.extensionFirstRun "FirstRun">
> > Can someone explain what this string means? I have no idea how to translate it.
> 
> "Indicates whether this is the extension's first run after install"
> 
> I think this is to facilitate extensions that want to show an introductory page
> the first time they run after they've been installed.

Axel, am I right in my belief that Curtis can add a localization note to this dtd when he does his final checkin without hurting string freeze/existing translations?

Comment 80

[Axel Hecht [:Pike]]

Yes, comments are fine. 

I'm not sure if the information is conveyed by the UI string "FirstRun", btw. I kick myself for not commenting my nag about that string when I glanced at the patch. Not to say more than "removal of strings would be fine still, too" :-)

Comment 81

[Curtis Bartley [:cbartley]]

(In reply to comment #80)
> Yes, comments are fine. 
> 
> I'm not sure if the information is conveyed by the UI string "FirstRun", btw. I
> kick myself for not commenting my nag about that string when I glanced at the
> patch. Not to say more than "removal of strings would be fine still, too" :-)

In the case of "FirstRun", I think it's more important to be meaningful to our support people than that it's particularly meaningful to the user.

Comment 82

[Axel Hecht [:Pike]]

Which raises the interesting question on whether that should be DONT_TRANSLATE. I'd at least encourage leaving it in English.

Comment 83

[Johnathan Nightingale [:johnath]]

Comment on attachment 400662 [details] [diff] [review]
about:support v7

This patch doesn't apply cleanly now, because of the string landings.

>diff -r 6f365b3614e6 browser/base/content/aboutSupport.xhtml
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/base/content/aboutSupport.xhtml	Mon Sep 14 19:54:41 2009 -0700
>@@ -0,0 +1,370 @@
>+<?xml version="1.0" encoding="UTF-8"?>
>+
>+# ***** BEGIN LICENSE BLOCK *****
># Version: MPL 1.1/GPL 2.0/LGPL 2.1
>#
># The contents of this file are subject to the Mozilla Public License Version

This patch is also a little odd because these comment lines aren't prefixed with +'s, confusing the heck out of hg.

>+
>+        // Update the other sections.
>+        populateExtensionsSection(Application.extensions.all)
>+        populatePreferencesSection(Application.prefs.all)
>+      }

Based on your reply, I am really concerned about the lag these introduce on initial load. People opening a Troubleshooting menu item are likely in no mood to have the browser seem to hang for 5 seconds. Can we do a deferred load here, instead? Basically, put a throbber placeholder in the tables initially, put these things in a setTimeout() and when they're done they can replace the throbber with their contents, that way the page itself comes up instantly and gives more visible indication that it's working very hard in the background. I would also disable the copy button until that timeout call completes. No need for new graphics resource, you can just use chrome://global/skin/icons/loading_16.png 

>+      function populateExtensionsSection(extensions)
>+      {
>+        var trExtensions = [];
>+        for (var i = 0; i < extensions.length; i++) {
>+          var extension = extensions[i];
>+          var tr = createParentElement("tr", [

I don't think it really matters much in this case, where the entire function is ~10 lines, but as of javascript 1.7, variables intended to have block scope instead of function scope should be declared with 'let', not 'var'.

>+      function populatePreferencesSection(prefs)
>+      {
>+        var modifiedPrefs = [];
>+        for (var i = 0; i < prefs.length; i++) {

Ditto, particularly since you re-use i later on in this function. You re-initialize i, so there's no bug here, but 'let' is more representative of your intent. In fact, in this particular case, you should really just do:

modifiedPrefs = prefs.filter(function(pref) {return pref.modified});

>+      function copyContentsToClipboard()
>+      {
>+        var contentsDiv = document.getElementById("contents");
>+        var dataHtml = contentsDiv.innerHTML;
>+        var dataText = contentsDiv.textContent;
>+        
>+        var supportsStringClass = Cc["@mozilla.org/supports-string;1"];
>+        var ssHtml = supportsStringClass.createInstance(Ci.nsISupportsString);
>+        var ssText = supportsStringClass.createInstance(Ci.nsISupportsString);
>+
>+        var transferable = Cc["@mozilla.org/widget/transferable;1"]
>+                             .createInstance(Ci.nsITransferable);  
>+
>+        transferable.addDataFlavor("text/html");
>+        ssHtml.data = dataHtml;
>+        transferable.setTransferData("text/html", ssHtml, dataHtml.length * 2);
>+
>+        transferable.addDataFlavor("text/unicode");
>+        ssText.data = dataText;
>+        transferable.setTransferData("text/unicode", ssText, dataText.length * 2);
>+        
>+        var clipboard = Cc["@mozilla.org/widget/clipboard;1"]
>+                          .getService(Ci.nsIClipboard);
>+        clipboard.setData(transferable, null, clipboard.kGlobalClipboard);        
>+      }

I shouldn't review our clipboard code, having not used it before. Has someone like Enn or mconnor reviewed this?

>diff -r 6f365b3614e6 browser/components/about/AboutRedirector.cpp
>   { "sessionrestore", "chrome://browser/content/aboutSessionRestore.xhtml",
>-    nsIAboutModule::ALLOW_SCRIPT }
>+    nsIAboutModule::ALLOW_SCRIPT },
>+  { "support", "chrome://browser/content/aboutSupport.xhtml",
>+    nsIAboutModule::ALLOW_SCRIPT },
> };

Shouldn't have the terminal comma here

>diff -r 6f365b3614e6 browser/locales/en-US/chrome/browser/aboutSupport.dtd
>+<!ENTITY aboutSupport.extensionsTitle "Extensions">
>+<!ENTITY aboutSupport.extensionName "Name">
>+<!ENTITY aboutSupport.extensionEnabled "Enabled">
>+<!ENTITY aboutSupport.extensionVersion "Version">
>+<!ENTITY aboutSupport.extensionFirstRun "FirstRun">
>+<!ENTITY aboutSupport.extensionId "ID">

Add the localization note mentioned in bug discussion, to explain what FirstRun means. I do think it should be translated if everything else is, I just also wish that we had included a space, though, to make it look more Englishy, less Jargony. File a follow-up bug to do so on trunk if it's too late to get it in on branch? 

r=me with the nits addressed, except for the clipboard bits, but I'd like to take a look at the delayed-load stuff before calling this done.

This patch doesn't include tests, but I'm not sure what they would look like - mostly you're exercising other APIs not implementing your own. With that said, though, please flag this with in-litmus? so that QA can put together a human "does the menu item do the thing, and does it look non-broken" test.

Comment 84

[Axel Hecht [:Pike]]

This hasn't landed yet on trunk nor branch?

We have the strings in on branch, so we should make a tough call on whether this should land for beta or after. Landing it now on branch might expose problems in the existing localizations. This consideration does not affect landing on central.

Comment 85

[Jesse Ruderman]

Please don't reveal the profile directory in this page.  Giving it away defeats the defense-in-depth security feature of having a nonce.  I think a "Reveal profile folder in Finder" button would be more useful, anyway.

Comment 86

[Percy Cabello]

> Please don't reveal the profile directory in this page.  Giving it away defeats
> the defense-in-depth security feature of having a nonce.  I think a "Reveal
> profile folder in Finder" button would be more useful, anyway.

How about showing the path where the profile folder is but not the name of it? (c:\users\jsmith\roaming profile\mozilla firefox\profiles) I think there may be permissions related issues that can be easily diagnosed knowing where the profile is located while still preserving the randomized name.(In reply to comment #85)

Comment 87

[Curtis Bartley [:cbartley]]

Attachment: about:support v8

[mconnor: can you take a look at the clipboard code?]
[johnath: note that I also added some code to make pasted text (as opposed to HTML) look better.]

(In reply to comment #83)
> (From update of attachment 400662 [details] [diff] [review])
> This patch doesn't apply cleanly now, because of the string landings.
> 
> >diff -r 6f365b3614e6 browser/base/content/aboutSupport.xhtml
> >--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >+++ b/browser/base/content/aboutSupport.xhtml	Mon Sep 14 19:54:41 2009 -0700
> >@@ -0,0 +1,370 @@
> >+<?xml version="1.0" encoding="UTF-8"?>
> >+
> >+# ***** BEGIN LICENSE BLOCK *****
> ># Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >#
> ># The contents of this file are subject to the Mozilla Public License Version
> 
> This patch is also a little odd because these comment lines aren't prefixed
> with +'s, confusing the heck out of hg.


This was a line ending problem.  I've fixed it, and I'll be more careful in the future.


> >+
> >+        // Update the other sections.
> >+        populateExtensionsSection(Application.extensions.all)
> >+        populatePreferencesSection(Application.prefs.all)
> >+      }
> 
> Based on your reply, I am really concerned about the lag these introduce on
> initial load. People opening a Troubleshooting menu item are likely in no mood
> to have the browser seem to hang for 5 seconds. Can we do a deferred load here,
> instead? Basically, put a throbber placeholder in the tables initially, put
> these things in a setTimeout() and when they're done they can replace the
> throbber with their contents, that way the page itself comes up instantly and
> gives more visible indication that it's working very hard in the background. I
> would also disable the copy button until that timeout call completes. No need
> for new graphics resource, you can just use
> chrome://global/skin/icons/loading_16.png 


I rewrote the code for reading the modified prefs to use the direct prefs API.
It now seems to take no more than a few hundred milliseconds to build the
modified prefs table.  This is just as well since the throbber PNG wouldn't
animate while the older and slower prefs code was running.


> >+      function populateExtensionsSection(extensions)
> >+      {
> >+        var trExtensions = [];
> >+        for (var i = 0; i < extensions.length; i++) {
> >+          var extension = extensions[i];
> >+          var tr = createParentElement("tr", [
> 
> I don't think it really matters much in this case, where the entire function is
> ~10 lines, but as of javascript 1.7, variables intended to have block scope
> instead of function scope should be declared with 'let', not 'var'.


Fixed.


> 
> >+      function populatePreferencesSection(prefs)
> >+      {
> >+        var modifiedPrefs = [];
> >+        for (var i = 0; i < prefs.length; i++) {
> 
> Ditto, particularly since you re-use i later on in this function. You
> re-initialize i, so there's no bug here, but 'let' is more representative of
> your intent. In fact, in this particular case, you should really just do:
> 
> modifiedPrefs = prefs.filter(function(pref) {return pref.modified});


This code is quite a bit different now, but it is using let.


> 
> >+      function copyContentsToClipboard()
> >+      {
> >+        var contentsDiv = document.getElementById("contents");
> >+        var dataHtml = contentsDiv.innerHTML;
> >+        var dataText = contentsDiv.textContent;
> >+        
> >+        var supportsStringClass = Cc["@mozilla.org/supports-string;1"];
> >+        var ssHtml = supportsStringClass.createInstance(Ci.nsISupportsString);
> >+        var ssText = supportsStringClass.createInstance(Ci.nsISupportsString);
> >+
> >+        var transferable = Cc["@mozilla.org/widget/transferable;1"]
> >+                             .createInstance(Ci.nsITransferable);  
> >+
> >+        transferable.addDataFlavor("text/html");
> >+        ssHtml.data = dataHtml;
> >+        transferable.setTransferData("text/html", ssHtml, dataHtml.length * 2);
> >+
> >+        transferable.addDataFlavor("text/unicode");
> >+        ssText.data = dataText;
> >+        transferable.setTransferData("text/unicode", ssText, dataText.length * 2);
> >+        
> >+        var clipboard = Cc["@mozilla.org/widget/clipboard;1"]
> >+                          .getService(Ci.nsIClipboard);
> >+        clipboard.setData(transferable, null, clipboard.kGlobalClipboard);        
> >+      }
> 
> I shouldn't review our clipboard code, having not used it before. Has someone
> like Enn or mconnor reviewed this?


I'll track down a suitable person.


> >diff -r 6f365b3614e6 browser/components/about/AboutRedirector.cpp
> >   { "sessionrestore", "chrome://browser/content/aboutSessionRestore.xhtml",
> >-    nsIAboutModule::ALLOW_SCRIPT }
> >+    nsIAboutModule::ALLOW_SCRIPT },
> >+  { "support", "chrome://browser/content/aboutSupport.xhtml",
> >+    nsIAboutModule::ALLOW_SCRIPT },
> > };
> 
> Shouldn't have the terminal comma here


I admit it's a little bit gauche, but we have a terminal comma in a number of
places in our source code.  It's legal C++, it simplifies adding records at the
end, and it results in cleaner looking diffs when you do add another record to
the initializer list.


> >diff -r 6f365b3614e6 browser/locales/en-US/chrome/browser/aboutSupport.dtd
> >+<!ENTITY aboutSupport.extensionsTitle "Extensions">
> >+<!ENTITY aboutSupport.extensionName "Name">
> >+<!ENTITY aboutSupport.extensionEnabled "Enabled">
> >+<!ENTITY aboutSupport.extensionVersion "Version">
> >+<!ENTITY aboutSupport.extensionFirstRun "FirstRun">
> >+<!ENTITY aboutSupport.extensionId "ID">
> 
> Add the localization note mentioned in bug discussion, to explain what FirstRun
> means. I do think it should be translated if everything else is, I just also
> wish that we had included a space, though, to make it look more Englishy, less
> Jargony. File a follow-up bug to do so on trunk if it's too late to get it in
> on branch?


Added the localization note, and also added that translations should feel free to add a space between the words.  Can we just slipstream the space in?  Its presence or absence shouldn't change the translations...


> r=me with the nits addressed, except for the clipboard bits, but I'd like to
> take a look at the delayed-load stuff before calling this done.


With the new optimizations, I think the load speed is no longer an issue.


> This patch doesn't include tests, but I'm not sure what they would look like -
> mostly you're exercising other APIs not implementing your own. With that said,
> though, please flag this with in-litmus? so that QA can put together a human
> "does the menu item do the thing, and does it look non-broken" test.


Noted.

Comment 88

[Mike Connor [:mconnor]]

Comment on attachment 401794 [details] [diff] [review]
 about:support v8

Only looked at the clipboard bits...

Don't use implicit global variables. Explicitly declare them, or don't use them at all.  And if you're using a global variable, there's no need to pass it as an argument to functions... this threw me off at first.

Is there any point to |indent| at all now?  You pass that through all over the place, but if you start with "" you're just adding unnecessary complexity...

Comment 89

[Dão Gottwald [:dao]]

Comment on attachment 401794 [details] [diff] [review]
 about:support v8

>+      body {
>+        background-color: -moz-Field;
>+        color: -moz-FieldText;
>+        font: message-box;
>+      }
>+
>+      div#outside {
>+       text-align: justify;
>+       width: 90%;
>+       margin-left: 5%;
>+       margin-right: 5%;
>+      }

You could probably use "html" instead of "body" and "body" instead of "div#outside".

>+      .pref-name {
>+        width: 70%;
>+        white-space: nowrap;
>+	      overflow:hidden;
>+      }
>+      
>+      .pref-value {
>+        width: 30%;
>+        white-space: nowrap;
>+	      overflow:hidden;
>+      }

overflow:hidden lacks a space after the colon and isn't indented properly.

Speaking of indentation, I don't think the CSS and JS code needs to inherit the indentation level from the HTML code.

>+    <script type="application/x-javascript;version=1.7"><![CDATA[

"x-" should be removed.

>+        function comparePrefs(pref1, pref2) {
>+          if (pref1.name < pref2.name) return -1;
>+          if (pref1.name > pref2.name) return 1;

There should be a line break after if ().

>+        for (let i = 0; i < childNodes.length; i++) {
>+          parentElem.appendChild(childNodes[i]);
>+        }

Single lines usually don't use curly brackets in browser code.

>+      function copyContentsToClipboard()
>+      {

This should be a single line ("function copyContentsToClipboard() {"), imho.

Comment 90

[Curtis Bartley [:cbartley]]

Attachment: about:support v8

****************** mconnor


(In reply to comment #88)
> (From update of attachment 401794 [details] [diff] [review])
> Only looked at the clipboard bits...
> 
> Don't use implicit global variables. Explicitly declare them, or don't use them
> at all.  And if you're using a global variable, there's no need to pass it as
> an argument to functions... this threw me off at first.


I've renamed "textFragments" to "textFragmentAccumulator" to make it explicit about how the results are being returned from the tree traversal.  Using a receiver argument like this is a little bit unusual in JavaScript, but we do it all the time in C++ (especially with strings).  I don't think it's an "implicit global variable".


> Is there any point to |indent| at all now?  You pass that through all over the
> place, but if you start with "" you're just adding unnecessary complexity...


I've also added some comments to better illuminate what the code is doing.  For example:

  // Recurse on the child element with an extra level of indentation.
  generateTextForElement(node, indent + "  ", textFragmentAccumulator);

Hopefully this explains why "indent" is important, and why it's passed as a parameter.


******************* dao


(In reply to comment #89)
> (From update of attachment 401794 [details] [diff] [review])
> >+      body {
> >+        background-color: -moz-Field;
> >+        color: -moz-FieldText;
> >+        font: message-box;
> >+      }
> >+
> >+      div#outside {
> >+       text-align: justify;
> >+       width: 90%;
> >+       margin-left: 5%;
> >+       margin-right: 5%;
> >+      }
> 
> You could probably use "html" instead of "body" and "body" instead of
> "div#outside".


Done.


> >+      .pref-name {
> >+        width: 70%;
> >+        white-space: nowrap;
> >+	      overflow:hidden;
> >+      }
> >+      
> >+      .pref-value {
> >+        width: 30%;
> >+        white-space: nowrap;
> >+	      overflow:hidden;
> >+      }
> 
> overflow:hidden lacks a space after the colon and isn't indented properly.


Fixed.  Apparently I let some tabs slip in.


> Speaking of indentation, I don't think the CSS and JS code needs to inherit the
> indentation level from the HTML code.


Inherited indentation for JS seems to be the norm, at least for non-test .xhtml files.  There wasn't enough inline CSS in these files to draw a conclusion for CSS though.


> >+    <script type="application/x-javascript;version=1.7"><![CDATA[
> 
> "x-" should be removed.


Fixed.


> >+        function comparePrefs(pref1, pref2) {
> >+          if (pref1.name < pref2.name) return -1;
> >+          if (pref1.name > pref2.name) return 1;
> 
> There should be a line break after if ().


Done.


> >+        for (let i = 0; i < childNodes.length; i++) {
> >+          parentElem.appendChild(childNodes[i]);
> >+        }
> 
> Single lines usually don't use curly brackets in browser code.


Done.


> >+      function copyContentsToClipboard()
> >+      {
> 
> This should be a single line ("function copyContentsToClipboard() {"), imho.


There seems to be no real consensus about opening brace placement for functions, at least in the "browser/base/content/*.js" files.

Comment 91

[Dão Gottwald [:dao]]

(In reply to comment #90)
> Inherited indentation for JS seems to be the norm, at least for non-test .xhtml
> files.  There wasn't enough inline CSS in these files to draw a conclusion for
> CSS though.

It may be the prevailing style, but most likely not for a good reason. So unless it makes sense to you, you don't need to copy it. I think not doing it makes it easier to handle the code, and I'm not doing it in bug 453063.

> There seems to be no real consensus about opening brace placement for
> functions, at least in the "browser/base/content/*.js" files.

Lots of legacy code and lots of different authors, including those who primarily deal with languages where this actually makes sense. We haven't tried to maintain this within these files and consequently don't need to do it in new files.

Comment 92

[Johnathan Nightingale [:johnath]]

Comment on attachment 401977 [details] [diff] [review]
about:support v8

(In reply to comment #90)
> > Don't use implicit global variables. Explicitly declare them, or don't use them
> > at all.  And if you're using a global variable, there's no need to pass it as
> > an argument to functions... this threw me off at first.
> 
> I've renamed "textFragments" to "textFragmentAccumulator" to make it explicit
> about how the results are being returned from the tree traversal.  Using a
> receiver argument like this is a little bit unusual in JavaScript, but we do it
> all the time in C++ (especially with strings).  I don't think it's an "implicit
> global variable".

...

>diff -r fb31c1bdf4dd browser/base/content/aboutSupport.xhtml
>+      // Return the plain text representation of an element.  Do a little bit
>+      // of pretty-printing to make it human-readable.
>+      function createTextForElement(elem)
>+      {
>+        // Generate the initial text.
>+        textFragmentAccumulator = [];

textFragmentAccumulator is initialized without a 'var' keyword (function scope) or a 'let' keyword (block scope) and hence, implicitly, becomes part of the global scope.

mconnor is saying that if you intend for it to be a global variable, it should have an explicit declaration outside of the various function bodies in your script block and, being global, doesn't need to be passed as an argument to any function. If you intend, however, for it to be function-scoped and not appear in the global, long-lived namespace, then it should be "var textFragmentAccumulator..."

I suspect the latter is your intent.

Comment 93

[Curtis Bartley [:cbartley]]

Attachment: about:support v10

(In reply to comment #92)

> >diff -r fb31c1bdf4dd browser/base/content/aboutSupport.xhtml
> >+      // Return the plain text representation of an element.  Do a little bit
> >+      // of pretty-printing to make it human-readable.
> >+      function createTextForElement(elem)
> >+      {
> >+        // Generate the initial text.
> >+        textFragmentAccumulator = [];
> 
> textFragmentAccumulator is initialized without a 'var' keyword (function scope)
> or a 'let' keyword (block scope) and hence, implicitly, becomes part of the
> global scope.
> 
> mconnor is saying that if you intend for it to be a global variable, it should
> have an explicit declaration outside of the various function bodies in your
> script block and, being global, doesn't need to be passed as an argument to any
> function. If you intend, however, for it to be function-scoped and not appear
> in the global, long-lived namespace, then it should be "var
> textFragmentAccumulator..."
> 
> I suspect the latter is your intent.


Indeed.  Fixed.

Comment 94

[Axel Hecht [:Pike]]

connor, johnath, can I nag you for reviews here? We want this in for beta and the localized strings need runtime testing for the markup in aboutSupport.pageSubtitle.

Comment 95

[Mike Connor [:mconnor]]

Comment on attachment 402122 [details] [diff] [review]
 about:support v10

This is fine for beta, for the clipboard bits at least, though johnath should weigh in as well.

We'll need a followup on:

* Not copy/pasting profile directory
* Fixing the plaintext copy/paste formatting (discussion on IRC)

Comment 96

[Johnathan Nightingale [:johnath]]

Comment on attachment 402122 [details] [diff] [review]
 about:support v10

I agree we want the followups mconnor mentions filed, as well as the one I requested in comment 61 to move this to toolkit.  I think the profile directory bug should block 3.6 final, but doesn't need to block this hitting the beta.

>+        let prefs = [Application.prefs.get(prefName)
>+                            for each (prefName in prefNames)
>+                                if (prefRootBranch.prefHasUserValue(prefName))];        

Don't see a lot of array comprehension syntax in the code, but this is a nice use of it.

>+            <th>
>+              &aboutSupport.extensionFirstRun;
>+            </th>

As we've said in IRC: firstRun was mostly added because it was there, not because SUMO requested it, the string is confusing and requires a localization note, and the field itself is unlikely to be really diagnostic most of the time. I'd say we should just remove it, really.

r=me with the firstrun removal and follow up bugs filed and nominated for blocking in the one case.

Comment 97

[Curtis Bartley [:cbartley]]

Attachment: about:support v11

****************** mconnor ********************

(In reply to comment #95)
> (From update of attachment 402122 [details] [diff] [review])
> This is fine for beta, for the clipboard bits at least, though johnath should
> weigh in as well.
> 
> We'll need a followup on:
> 
> * Not copy/pasting profile directory


Bug 518601 - Troubleshooting Information page should not allow copy-and-paste of the profile directory.


> * Fixing the plaintext copy/paste formatting (discussion on IRC)


Bug 518606 - Troubleshooting Information page should have better support for copy-and-paste to plaintext.


****************** johnath ********************

(In reply to comment #96)
> (From update of attachment 402122 [details] [diff] [review])
> I agree we want the followups mconnor mentions filed, as well as the one I
> requested in comment 61 to move this to toolkit.  I think the profile directory
> bug should block 3.6 final, but doesn't need to block this hitting the beta.


Bug 518601 - Troubleshooting Information page should not allow copy-and-paste of the profile directory. (requested blocking)

Bug 518607 - Move the Troubleshooting Information page into toolkit so other apps like Thunderbird and SeaMonkey can use it.


> >+        let prefs = [Application.prefs.get(prefName)
> >+                            for each (prefName in prefNames)
> >+                                if (prefRootBranch.prefHasUserValue(prefName))];        
> 
> Don't see a lot of array comprehension syntax in the code, but this is a nice
> use of it.
> 
> >+            <th>
> >+              &aboutSupport.extensionFirstRun;
> >+            </th>
> 
> As we've said in IRC: firstRun was mostly added because it was there, not
> because SUMO requested it, the string is confusing and requires a localization
> note, and the field itself is unlikely to be really diagnostic most of the
> time. I'd say we should just remove it, really.


Done.  Note that I changed the localization note in the DTD to just "no longer in use".  Is there existing protocol for this sort of thing?


> r=me with the firstrun removal and follow up bugs filed and nominated for
> blocking in the one case.

Comment 98

[Axel Hecht [:Pike]]

(In reply to comment #97)
<...>
> > As we've said in IRC: firstRun was mostly added because it was there, not
> > because SUMO requested it, the string is confusing and requires a localization
> > note, and the field itself is unlikely to be really diagnostic most of the
> > time. I'd say we should just remove it, really.
> 
> 
> Done.  Note that I changed the localization note in the DTD to just "no longer
> in use".  Is there existing protocol for this sort of thing?

Just remove the string, that's fine for both central and 1.9.2 at this point in time. It will only yield obsolete strings for now, which don't pose any problem with shipping, and the localizations can still fix it for either Beta or RC.

Comment 99

[Johnathan Nightingale [:johnath]]

Comment on attachment 402648 [details] [diff] [review]
about:support v11

r+ with axel's suggestion to delete it, but that can be fixed on checkin, no need for follow up review.

Boat this bass.

Comment 100

[Curtis Bartley [:cbartley]]

Attachment: about:support v12

All references to FirstRun are gone.

Comment 101

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/b1d9985febb9

Comment 102

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get it on branch as well, so our l10n teams can get cracking with it in context?

Comment 103

[Mike Beltzner [:beltzner, not reading bugmail]]

(oh, nm, didn'tr realize we were still waiting for green on trunk - sorry for bugspam!)

Comment 104

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 402658 [details] [diff] [review]
about:support v12

a192=beltzner

Comment 105

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/94cd21e19cca

Comment 106

[Mike Beltzner [:beltzner, not reading bugmail]]

Glad this made it in - not marking as blocking because if it caused regressions or other problems we'd ship without it, but hoo boy, happy to have it.

Comment 107

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=9541 is a basic test case. I also updated the file menu test case to reflect the fact that "Troubleshooting Information."

Comment 108

[PikeUK]

(In reply to comment #47)
> This menu item should have the ellipsis character after it, like Report Broken
> Web Site does.

On MS Windows at least I don't think that is correct, see:

http://blogs.msdn.com/oldnewthing/archive/2004/05/17/133181.aspx

Comment 109

[Mike Connor [:mconnor]]

<offtopic>wow, it's pike!</offtopic>

Yeah... "show info" doesn't get an ellipsis.  Can someone file a followup and fix this?  I'll conduct a remedial beating^Wclass about proper ellipsis usage...

Comment 110

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Gnome's HIGs (http://library.gnome.org/devel/hig-book/stable/menus-design.html.en) are practically saying the same as Windows HIGs:

"Label the menu item with a trailing ellipsis ("...") only if the command requires further input from the user before it can be performed. Do not add an ellipsis to items that only present a confirmation dialog (such as Delete), or that do not require further input (such as Properties, Preferences or About)."

Apple's HIGs (http://developer.apple.com/mac/library/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGText/XHIGText.html) say:
Use an ellipsis in the name of a button or menu item when the associated action:
* Requires specific input from the user.
* Is performed _by the user_ in a separate window or dialog.
* Always displays an alert that warns the user of a potentially dangerous outcome and offers an alternative.
BUT NOT when:
* Does not require specific input from the user.
* Is completed by the opening of a panel.
* Occasionally displays an alert that warns the user of a potentially dangerous outcome.

So the ellipsis is wrong on all the threee platforms. i'll file the followup.

Comment 111

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

filed bug 522667