786779 - clean up of ugly overly long lines in browser/base/content/sync/setup.js

Status: RESOLVED INCOMPLETE

(Firefox :: Sync, defect)

Description

[Allison Naaktgeboren :ally]

follow up from https://bugzilla.mozilla.org/show_bug.cgi?id=762230#c7. 

The reviewer did not want the style change in bug 762230's patch, but cleaner code is good for everyone.

Comment 1

[Allison Naaktgeboren :ally]

Attachment: patch part 1/1 v0

I do miss block scope.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 658303 [details] [diff] [review]
patch part 1/1 v0

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

::: browser/base/content/sync/setup.js
@@ +872,4 @@
>                                     .DBConnection;
>          if (Weave.Engines.get("history").enabled) {
>            let daysOfHistory = 0;
> +          let localizedHistoryLabel = "";

Why do you declare this here?

@@ +887,5 @@
>            // Support %S for historical reasons (see bug 600141)
> +          localizedHistoryLabel = PluralForm.get(daysOfHistory,
> +            this._stringBundle.GetStringFromName("historyDaysCount.label"))
> +            .replace("%S", daysOfHistory).replace("#1", daysOfHistory);
> +          document.getElementById("historyCount").value = localizedHistoryLabel;

IMO this style is worse than before.

Here's what I'd do:

let label = this._stringBundle.GetStringFromName("historyDaysCount.label");
let localized = PluralForm.get(daysOfHistory, label)
                          .replace("%S", daysOfHistory)
                          .replace("#1", daysOfHistory);

document.getElementById("historyCount").value = localized;

But, this is all subjective. The original code isn't /that/ bad. I think I was being overly harsh in the original style nit.

@@ +935,5 @@
>            for each (let i in ids) {
>              if (i) {
>                blessedcount++;
>              }
>            }

While you are here:

  let blessedcount = Object.keys(ids).length;

Comment 3

[Richard Newman [:rnewman]]

Let's fix this when we rewrite that file, rather than in a separate bug.