1512837 - cleanup logic in mailnews/base/content/dateFormat.js

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[:aceman]

Improve the logic flow in mailnews/base/content/dateFormat.js, e.g. do not set the same default values at 2 places.

Comment 1

[:aceman]

Attachment: 1512837.patch

The calls to getComplexValue(pref_name, Ci.nsIPrefLocalizedString) were missing the .data (see the third case) but they still could be converted silently to the correct value, at least in a dump().
I add them for safety as the .data is always used in other places.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9041003 [details] [diff] [review]
1512837.patch

Review of attachment 9041003 [details] [diff] [review]:
-----------------------------------------------------------------

You never cease to surprise. How did we get to this bug? I this covered by tests? Strange that `.data` was missing and it still did something useful.

::: mailnews/base/content/dateFormat.js
@@ +42,5 @@
>      // check the format option
>      if (arrayOfStrings.length != 3)  // no successful split
>      {
> +      Cu.reportError("initLocaleShortDateFormat: could not analyze the date format, defaulting to mm/dd/yyyy");
> +      return;

This early return isn't needed.

@@ +65,5 @@
>        {
>          // 12.1999.01 or 1999.12.01
>          gSearchDateFormat = arrayOfStrings[0] == "12" ? 4 : 1;
>        }
> +      return;

And this one either.

@@ +84,5 @@
>    // get a search date format option and a separator
>    try {
>      gSearchDateFormat =
>        Services.prefs.getComplexValue("mailnews.search_date_format",
> +                                     Ci.nsIPrefLocalizedString).data;

I wonder how that worked without the .data.

Comment 3

[:aceman]

(In reply to Jorg K (GMT+1) from comment #2)

This early return isn't needed.

The returns aren't needed in current state of the function, but it looked cleaner to me to exit early when done.

gSearchDateFormat =
Services.prefs.getComplexValue("mailnews.search_date_format",

•                                 Ci.nsIPrefLocalizedString).data;
  

I wonder how that worked without the .data.

I tested it with dump() and it displayed the correct string value even though gSearchDateFormat was of Object type. Probably some automatic conversion.

Comment 4

[Jorg K (CEST = GMT+2)]

I'd remove the returns.

Comment 5

[:aceman]

Attachment: 1512837.patch v1.1

Ok, thanks

Comment 6

[:aceman]

(In reply to Jorg K (GMT+1) from comment #2)

You never cease to surprise. How did we get to this bug? I this covered by
tests?

Noticed in bug 1512808.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6ba67360bdd4
clean up logic in mailnews/base/content/dateFormat.js. r=jorgk