Closed Bug 624108 Opened 9 years ago Closed 9 years ago

Land about:support

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a2

People

(Reporter: rain1, Assigned: rain1)

References

()

Details

Attachments

(1 file, 3 obsolete files)

about:support is a useful troubleshooting tool. The idea is to have a list of account settings and modified prefs in one spot, and to provide easy and safe means of communicating these settings to people who can help troubleshoot an issue.
Depends on: 623503
Depends on: 624150
Attached patch WIP (obsolete) — Splinter Review
I still need to
- figure out appropriate naming for these files
- write more tests -- in particular, test copying to clipboard and sending a message
Couldn't you use the existing aboutSupport.css from toolkit/theme? Maybe the additional definitions can be added in the now called private.css. Then platform specific styling is possible.

Additionally, themers are also able to style about:support.
(In reply to comment #2)
> Couldn't you use the existing aboutSupport.css from toolkit/theme?

Hm, probably.
> additional definitions can be added in the now called private.css.

No, it'd require a separate stylesheet. private.css is switched on and off based on a checkbox.

> Then
> platform specific styling is possible.
> 
> Additionally, themers are also able to style about:support.

I didn't follow this -- how does having all the definitions in mail/ affect this?
Oh, we have separate per-platform styles. Oops.
(In reply to comment #3)
> (In reply to comment #2)
> > Then
> > platform specific styling is possible.
> > 
> > Additionally, themers are also able to style about:support.
> 
> I didn't follow this -- how does having all the definitions in mail/ affect
> this?

When it's only in mail/ and not in mail/themes then its hard coded and can't be adapted for example a dark theme (the background would stay white and the text black).

When aboutSupport.css would be in mail/themes this would also be okay. But why not use already existing files?
Attached patch WIP #2 (obsolete) — Splinter Review
OK, I've switched to using the toolkit theme, plus our own additions in mail/themes, plus things that should not be skinnable (just showing or hiding stuff) still in the same directory. Does this make sense to you?
Attachment #502266 - Attachment is obsolete: true
For me this looks okay.
Attached patch WIP #3 (obsolete) — Splinter Review
I've split each section out into a separate file. I think this is a lot more manageable than dumping everything in one file.
Attachment #502283 - Attachment is obsolete: true
Attached patch Patch, v1Splinter Review
I haven't written tests for the compose stuff yet, nor unit tests for the socket type/auth method converters. Per <https://mail.mozilla.org/pipermail/tb-planning/2011-January/000659.html>, that should be fine.

Blake's kindly agreed to review the patch, and I'm assuming the JS modules count as API, so asking Standard8 to sr.

I'll kick off a try server build shortly.
Attachment #502337 - Attachment is obsolete: true
Attachment #502918 - Flags: ui-review?(clarkbw)
Attachment #502918 - Flags: superreview?(bugzilla)
Attachment #502918 - Flags: review?(bwinton)
I've tested the Mac version of your tryserver builds. All in all, I like it very much. But I have the following comments:
1. There is my "Mail and news accounts" part.
http://img502.imageshack.us/img502/2835/newsjg.jpg
I think the part I've marked orange looks a bit out of alignment. Is it possible to align this a bit better, maybe center it or something?
2. This is my Graphics part. 
http://img718.imageshack.us/img718/9559/graphicq.jpg
If I compare this with the values from the OS X system profiler, than the vendor ID should be 0x10de and not 0000. And the Device ID should also not 0000, it should be 0x08a0
3. Why do I have blank lines? E.g. the RAM should be 256 MB.
4. As I understand, Direct2D is Windows-only? But why is it than mentioned in my OS X build. This could confuse the normal user.
(In reply to comment #11)
> I've tested the Mac version of your tryserver builds. All in all, I like it
> very much. But I have the following comments:
> 1. There is my "Mail and news accounts" part.
> http://img502.imageshack.us/img502/2835/newsjg.jpg
> I think the part I've marked orange looks a bit out of alignment. Is it
> possible to align this a bit better, maybe center it or something?

What do you mean by "out of alignment"? We're letting HTML naturally align the text.

> 2. This is my Graphics part. 
> http://img718.imageshack.us/img718/9559/graphicq.jpg
> If I compare this with the values from the OS X system profiler, than the
> vendor ID should be 0x10de and not 0000. And the Device ID should also not
> 0000, it should be 0x08a0
> 3. Why do I have blank lines? E.g. the RAM should be 256 MB.
> 4. As I understand, Direct2D is Windows-only? But why is it than mentioned in
> my OS X build. This could confuse the normal user.

The code's copied from core, so you'll see these issues in the Firefox version too. I'm not qualified to fix them -- I'm going to assume that the graphics folks will fix them at some point.
(In reply to comment #11)
> 1. There is my "Mail and news accounts" part.
> http://img502.imageshack.us/img502/2835/newsjg.jpg
> I think the part I've marked orange looks a bit out of alignment. Is it
> possible to align this a bit better, maybe center it or something?

Assuming you're talking about vertical alignment, each row is one account. There's no outgoing server for the Local Folders account (for obvious reasons), so you see a blank row there.
Depends on: 625126
I guess we could make that a little more obvious, though.
(In reply to comment #12)
> (In reply to comment #11)
> > I've tested the Mac version of your tryserver builds. All in all, I like it
> > very much. But I have the following comments:
> > 1. There is my "Mail and news accounts" part.
> > http://img502.imageshack.us/img502/2835/newsjg.jpg
> > I think the part I've marked orange looks a bit out of alignment. Is it
> > possible to align this a bit better, maybe center it or something?
> 
> What do you mean by "out of alignment"? We're letting HTML naturally align the
> text.

I mean, for my eyes it doesn't look so nice, that the title (Connection security, Authentication method...) has not the same center point as the text block beneath (STARTTLS, Normal password...).
(In reply to comment #15)
> I mean, for my eyes it doesn't look so nice, that the title (Connection
> security, Authentication method...) has not the same center point as the text
> block beneath (STARTTLS, Normal password...).

That tends to be normal layout for tables, IMO its probably not going to look better fully centred.
One thing that might help would be a heavier line between the headings and the content. That would probably divide things up a little better.
Comment on attachment 502918 [details] [diff] [review]
Patch, v1

>+++ b/mail/components/about-support/Makefile.in
>@@ -0,0 +1,21 @@
>+# Include the platform-specific module
>+ifeq ($(MOZ_WIDGET_TOOLKIT), cocoa)
>+EXTRA_JS_MODULES += aboutSupportMac.js
>+else
>+ifeq ($(MOZ_WIDGET_TOOLKIT), windows)
>+EXTRA_JS_MODULES += aboutSupportWin32.js
>+else
>+EXTRA_JS_MODULES += aboutSupportUnix.js
>+endif
>+endif

Some indentation might be nice here, although not mandatory.
And would it make sense to put these in the Windows/Mac/Unix order you use down below?

>+++ b/mail/components/about-support/aboutSupport.js
>@@ -0,0 +1,191 @@
>+// Platform-specific includes
>+if ("@mozilla.org/windows-registry-key;1" in Components.classes)
>+  Components.utils.import("resource:///modules/aboutSupportWin32.js");
>+else if ("nsILocalFileMac" in Components.interfaces)
>+  Components.utils.import("resource:///modules/aboutSupportMac.js");
>+else
>+  Components.utils.import("resource:///modules/aboutSupportUnix.js");


>+var AboutSupport = {
>+  __proto__: AboutSupportPlatform,

Changing your base class depending on which platform we're on…  Nice.  :)
(Although, I do seem to remember running into problems doing that at a previous company.  On the other hand, I think we were using C++ templates, so it's not really applicable here.)

>+      if (idA > idB)
>+        return 1;
>+      else if (idA < idB)
>+        return -1;
>+      else
>+        return 0;

Can't you just return "idB - idA" here, instead of the if-elses?

>+++ b/mail/components/about-support/aboutSupportMac.js
>@@ -0,0 +1,49 @@
>+  /**
>+   * Given an nsIFile, gets the file system type. The type is returned as a
>+   * string. Possible values are "network", "local" and "unknown".

and null.

>+  getFileSystemType: function ASPWin32_getFileSystemType(aFile) {
>+    // Not implemented
>+    return null;

I wonder if returning "unknown" here would be better?  :)

>+++ b/mail/components/about-support/aboutSupportUnix.js
>@@ -0,0 +1,149 @@
>+      // Yes, we want function scoping for variables we need to free in the
>+      // finally block. I think this is better than declaring lots of variables
>+      // on top.
>+      var filePath = g_filename_from_utf8(aFile.path, -1, null, null, null);

I think it's a little confusing, but you've explained it pretty clearly here.

>+      if (glibFileInfo.isNull()) {
>+        throw new Error("Unabled to retrieve GLib file info for " + aFile.path);
>+      }

You don't really need the {}s here, but I'm okay with leaving them in.

>+++ b/mail/components/about-support/aboutSupportWin32.js
>@@ -0,0 +1,106 @@
>+const BOOL = ctypes.int32_t;
>+const MAX_PATH = 260;

I don't see any other uses of MAX_PATH.  Can you remove it?

>+++ b/mail/components/about-support/content/export.js
>@@ -0,0 +1,257 @@
>+ * The Original Code is aboutSupport.xhtml.

Typo?

>+function createTextForElement(elem, aHidePrivateData) {>+  // Trim extraneous whitespace before newlines, then squash extraneous
>+  // blank lines.
>+  text = text.replace(/[ \t]+\n/g, "\n");

Should that regex just be "/\s+\n/g"?

>+  text = text.replace(/\n\n\n+/g, "\n\n");

And I think this one can be "/\n{3,}/g"…

>+++ b/mail/components/about-support/content/gfx.js
>@@ -0,0 +1,172 @@
>+ * Portions created by the Initial Developer are Copyright (C) 2009

2010?

>+  let bundle = Services.strings.createBundle("chrome://global/locale/aboutSupport.properties");

Maybe we could break this line into two.

>+++ b/mail/components/about-support/content/prefs.js
>@@ -0,0 +1,145 @@
>+  function comparePrefs(pref1, pref2) {
>+    if (pref1.name < pref2.name)
>+      return -1;
>+    if (pref1.name > pref2.name)
>+      return 1;
>+    return 0;

Could you replace this with "return pref1.name.localeCompare(pref2.name);"?

>+  let sortedPrefs = modifiedPrefs.sort(comparePrefs);

I think sort sorts the array in place, so you don't need to assign it to sortedPrefs.

>+function formatPrefValue(prefValue) {
>+  // Some pref values are really long and don't have spaces.  This can cause
>+  // problems when copying and pasting into some WYSIWYG editors.  In general
>+  // the exact contents of really long pref values aren't particularly useful,
>+  // so we truncate them to some reasonable length.
>+  let maxPrefValueLen = 120;
>+  let text = "" + prefValue;
>+  if (text.length > maxPrefValueLen)
>+    text = text.substring(0, maxPrefValueLen) + ELLIPSIS;

I think it would be better to use the first 80 characters and the last 20, with an ellipsis in between.

>+++ b/mail/components/about-support/content/private.css
>@@ -0,0 +1,44 @@
>+.data-private {
>+  display: none;
>+}
>+
>+.data-public {
>+  display: inline;
>+}


>+++ b/mail/components/about-support/content/public.css
>@@ -0,0 +1,40 @@
>+.data-public {
>+  display: none;
>+}

I think private.css would be more accurately named hide-private.css and similarly public.css should be show-private.css.

>+++ b/mail/test/mozmill/content-tabs/test-about-support.js
>@@ -0,0 +1,330 @@
>+// After every test we want to close the about:support tab so that failures
>+// don't cascade.
>+function teardownTest(module) {
>+  mc.tabmail.closeOtherTabs(mc.tabmail.tabInfo[0]);
>+}

Well, we really only want to close it after failures, but this is fine, too.  :)

>+/*
>+ * Helpers
>+ */
>+
>+/**
>+ * Asserts that the given text is present on the about:support page.
>+ */
>+function assert_about_support_text_present(aTab, aText) {
>+/**
>+ * Asserts that the given text is absent on the about:support page.
>+ */
>+function assert_about_support_text_absent(aTab, aText) {
>+/**
>+ * Returns the current "display" style property of an element.
>+ */
>+function get_element_display(aTab, aElem) {
>+/**
>+ * Asserts that the given element is hidden from view on the page.
>+ */
>+function assert_element_hidden(aTab, aElem) {
>+/**
>+ * Asserts that the given element is visible on the page.
>+ */
>+function assert_element_visible(aTab, aElem) {
>+/**
>+ * Waits for the element's display property to be the given value.
>+ */
>+function wait_for_element_display_value(aTab, aElem, aValue) {

These all seem like things that should go in a common place.
How about mail/test/mozmill/shared-modules/test-display-helpers.js?

>+ * Test that a modified preference on the whitelist but not on the blacklist
>+ * doesn't show up.

I think you mean "does show up".

>+++ b/mail/test/mozmill/shared-modules/test-content-tab-helpers.js
>@@ -130,16 +131,17 @@ function open_content_tab_with_click(aEl
>   let expectedNewTab = aController.tabmail.tabInfo[preCount];
>   folderDisplayHelper.assert_selected_tab(expectedNewTab);
>+  folderDisplayHelper.assert_tab_mode_name(expectedNewTab, "contentTab");

I'm a little concerned about this change breaking other tests.
On the other hand, it seems like a reasonable constraint.


>+++ b/mail/themes/pinstripe/jar.mn
>@@ -6,16 +6,17 @@ classic.jar:
>+  skin/classic/messenger/aboutSupport.css                        (../qute/mail/aboutSupport.css)

Uh…  How about you move that file into a non-qute directory?  (Yeah, I know they do it that way in toolkit, but I unless you've got a good reason not to, I really think we should keep cross-platform files in cross-platform locations, otherwise people can easily get confused, and add Windows-only stuff to it.)

So, I don't think I've asked for anything particularly hard here, so I'm going to say r=me with the bigger changes made.

Later,
Blake.
Attachment #502918 - Flags: review?(bwinton) → review+
Attachment #502918 - Flags: ui-review?(clarkbw) → ui-review+
> 
> Some indentation might be nice here, although not mandatory.

I don't think we can indent makefile commands.

> 
> >+  getFileSystemType: function ASPWin32_getFileSystemType(aFile) {
> >+    // Not implemented
> >+    return null;
> 
> I wonder if returning "unknown" here would be better?  :)

We treat "unknown" and null differently. "unknown" is actually displayed as "(Unknown location)" in the frontend, which is abnormal since we should normally be able to detect the location on Windows and Unix. null isn't displayed at all.

> 
> >+++ b/mail/components/about-support/aboutSupportWin32.js
> >@@ -0,0 +1,106 @@
> >+const BOOL = ctypes.int32_t;
> >+const MAX_PATH = 260;
> 
> I don't see any other uses of MAX_PATH.  Can you remove it?

Yeah, pretty sure it's vestigial.

> 
> >+++ b/mail/components/about-support/content/export.js
> >@@ -0,0 +1,257 @@
> >+ * The Original Code is aboutSupport.xhtml.
> 
> Typo?

The code's copied over from the Firefox version, so I retained the original code block.

> 
> >+function createTextForElement(elem, aHidePrivateData) {
> …
> >+  // Trim extraneous whitespace before newlines, then squash extraneous
> >+  // blank lines.
> >+  text = text.replace(/[ \t]+\n/g, "\n");
> 
> Should that regex just be "/\s+\n/g"?

No, \s matches \n.

>+++ b/mail/components/about-support/content/gfx.js
> >@@ -0,0 +1,172 @@
> >+ * Portions created by the Initial Developer are Copyright (C) 2009
> 
> 2010?

No, again the code's copied over from Firefox.

> 
> >+  let bundle = Services.strings.createBundle("chrome://global/locale/aboutSupport.properties");
> 
> Maybe we could break this line into two.

I want to keep the graphics code identical to the core version.

> 
> >+function formatPrefValue(prefValue) {
> >+  // Some pref values are really long and don't have spaces.  This can cause
> >+  // problems when copying and pasting into some WYSIWYG editors.  In general
> >+  // the exact contents of really long pref values aren't particularly useful,
> >+  // so we truncate them to some reasonable length.
> >+  let maxPrefValueLen = 120;
> >+  let text = "" + prefValue;
> >+  if (text.length > maxPrefValueLen)
> >+    text = text.substring(0, maxPrefValueLen) + ELLIPSIS;
> 
> I think it would be better to use the first 80 characters and the last 20, with
> an ellipsis in between.

It's what Firefox does, but if you really want it this way, I can change it.

> 
> >+++ b/mail/test/mozmill/content-tabs/test-about-support.js
> >@@ -0,0 +1,330 @@
> >+// After every test we want to close the about:support tab so that failures
> >+// don't cascade.
> >+function teardownTest(module) {
> >+  mc.tabmail.closeOtherTabs(mc.tabmail.tabInfo[0]);
> >+}
> 
> Well, we really only want to close it after failures, but this is fine, too. 
> :)

Yeah, it's helpful to think of teardownTest as a means to achieve an invariant at the beginning of each test (that only the first tab should be open).

> 
> >+/*
> >+ * Helpers
> >+ */
> >+
> >+/**
> >+ * Asserts that the given text is present on the about:support page.
> >+ */
> >+function assert_about_support_text_present(aTab, aText) {
> >+/**
> >+ * Asserts that the given text is absent on the about:support page.
> >+ */
> >+function assert_about_support_text_absent(aTab, aText) {

These two are kind of specific to about:support, so I don't really see the point of moving them. I've moved the others into test-content-tab-helpers.js.

> 
> >+++ b/mail/themes/pinstripe/jar.mn
> >@@ -6,16 +6,17 @@ classic.jar:
> >+  skin/classic/messenger/aboutSupport.css                        (../qute/mail/aboutSupport.css)
> 
> Uh…  How about you move that file into a non-qute directory?  (Yeah, I know
> they do it that way in toolkit, but I unless you've got a good reason not to, I
> really think we should keep cross-platform files in cross-platform locations,
> otherwise people can easily get confused, and add Windows-only stuff to it.)

We don't really have such a directory here. Note that the file needs to be in themes/ to be skinnable: see comment 5.
Comment on attachment 502918 [details] [diff] [review]
Patch, v1

sr=Standard8 on the api additions.
Attachment #502918 - Flags: superreview?(bugzilla) → superreview+
(In reply to comment #19)
> > Some indentation might be nice here, although not mandatory.
> I don't think we can indent makefile commands.

Okay.

> > >+  getFileSystemType: function ASPWin32_getFileSystemType(aFile) {
> > >+    // Not implemented
> > >+    return null;
> > I wonder if returning "unknown" here would be better?  :)
> We treat "unknown" and null differently. "unknown" is actually displayed as
> "(Unknown location)" in the frontend, which is abnormal since we should
> normally be able to detect the location on Windows and Unix. null isn't
> displayed at all.

Okay.

> > >+++ b/mail/components/about-support/content/export.js
> > >@@ -0,0 +1,257 @@
> > >+ * The Original Code is aboutSupport.xhtml.
> > Typo?
> The code's copied over from the Firefox version, so I retained the original
> code block.

Okay.

> > >+function createTextForElement(elem, aHidePrivateData) {
> > …
> > >+  // Trim extraneous whitespace before newlines, then squash extraneous
> > >+  // blank lines.
> > >+  text = text.replace(/[ \t]+\n/g, "\n");
> > Should that regex just be "/\s+\n/g"?
> No, \s matches \n.

Drats!  Wait, even in single-line mode?  (Either way, I'm okay with you leaving it the way it is.)

> >+++ b/mail/components/about-support/content/gfx.js
> > >@@ -0,0 +1,172 @@
> > >+ * Portions created by the Initial Developer are Copyright (C) 2009
> > 2010?
> No, again the code's copied over from Firefox.

Okay.

> > >+  let bundle = Services.strings.createBundle("chrome://global/locale/aboutSupport.properties");
> > Maybe we could break this line into two.
> I want to keep the graphics code identical to the core version.

Okay.

> > >+function formatPrefValue(prefValue) {
> > >+  // Some pref values are really long and don't have spaces.  This can cause
> > >+  // problems when copying and pasting into some WYSIWYG editors.  In general
> > >+  // the exact contents of really long pref values aren't particularly useful,
> > >+  // so we truncate them to some reasonable length.
> > >+  let maxPrefValueLen = 120;
> > >+  let text = "" + prefValue;
> > >+  if (text.length > maxPrefValueLen)
> > >+    text = text.substring(0, maxPrefValueLen) + ELLIPSIS;
> > 
> > I think it would be better to use the first 80 characters and the last 20, with
> > an ellipsis in between.
> It's what Firefox does, but if you really want it this way, I can change it.

I won't insist on it, so you can change it or not as you prefer.

> > >+function assert_about_support_text_present(aTab, aText) {
> > >+function assert_about_support_text_absent(aTab, aText) {
> These two are kind of specific to about:support, so I don't really see the
> point of moving them. I've moved the others into test-content-tab-helpers.js.

The only about_support specific stuff I saw in them was a string in the error message.  And I think other tests might want to check for present or absent text.  In the interests of landing the patch, I won't insist on the change, but I do think it would be a good one to make.

> > >+++ b/mail/themes/pinstripe/jar.mn
> > >@@ -6,16 +6,17 @@ classic.jar:
> > >+  skin/classic/messenger/aboutSupport.css                        (../qute/mail/aboutSupport.css)
> > 
> > Uh…  How about you move that file into a non-qute directory?  (Yeah, I know
> > they do it that way in toolkit, but I unless you've got a good reason not to, I
> > really think we should keep cross-platform files in cross-platform locations,
> > otherwise people can easily get confused, and add Windows-only stuff to it.)
> 
> We don't really have such a directory here. Note that the file needs to be in
> themes/ to be skinnable: see comment 5.

Okay.

Thanks,
Blake.
Status: NEW → ASSIGNED
(In reply to comment #21)
> 
> The only about_support specific stuff I saw in them was a string in the error
> message.  And I think other tests might want to check for present or absent
> text.  In the interests of landing the patch, I won't insist on the change, but
> I do think it would be a good one to make.

OK, I've moved both out into test-content-tab-helpers.js.

Landed: https://hg.mozilla.org/comm-central/rev/285ca4c14963
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
Depends on: 628204
You need to log in before you can comment on or make changes to this bug.