Closed Bug 1365068 Opened 7 years ago Closed 7 years ago

Style cleanup of Satchel

Categories

(Toolkit :: Form Manager, enhancement)

Product:
Toolkit
Component:
Form Manager
Platform:
Unspecified
All
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jonathanGB, Assigned: jonathanGB)

References

(
URL
)

Details

Attachments

(4 files)

Bug 1365068 - Cleanup getListSuggestions.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1365068 - Added missing brackets & refactored some conditional statements.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1365068 - normalize indentation to 2 spaces.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1365068 - import with destructuring, switch to shorthand function declarations and use array.includes.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
The method "getListSuggestions" inside "nsInputListAutoComplete.js" (see URL) could be simplified, and be more linear (less embedding of if/else structures).
Summary: Style cleanup of nsInputListAutoComplete.js → Style cleanup of Satchel
Comment on attachment 8867910 [details]
Bug 1365068 - Cleanup getListSuggestions.

https://reviewboard.mozilla.org/r/139442/#review144254

::: toolkit/components/satchel/nsInputListAutoComplete.js:29
(Diff revision 1)
> +    if (!aField || !aField.list) {
> +      return [[], []]; // empty 2-tuple
> +    }
> +
>      let values = [];
>      let labels = [];

Nit: move this below the `values` and `labels` initialization and do:
`return [values, labels];`

then you don't need the comment and it's easy for someone looking at the beginning of the method to know the return value structure.

::: toolkit/components/satchel/nsInputListAutoComplete.js:40
(Diff revision 1)
> -        for (let i = 0; i < length; i++) {
> +    for (let i = 0; i < length; i++) {
> -          let item = options.item(i);
> +      let item = options.item(i);

(Not sure if you already did this in another commit but this could be switched to for…of I think [in a different commit])
Attachment #8867910 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8868349 [details]
Bug 1365068 - Added missing brackets & refactored some conditional statements.

https://reviewboard.mozilla.org/r/139908/#review144260

Thanks for this. Btw. eslint can do some of these changes for you. Where possible it would be good to enable eslint rules to ensure nobody regresses these styles fixes.

::: toolkit/components/satchel/FormHistory.jsm:238
(Diff revision 1)
>    }
>  }
>  
>  function makeQueryPredicates(aQueryData, delimiter = " AND ") {
>    return Object.keys(aQueryData).map(function(field) {
> -    if (field == "firstUsedStart") {
> +    switch(field) {

Nit: missing space after `switch`

::: toolkit/components/satchel/FormHistory.jsm:238
(Diff revision 1)
> -    if (field == "firstUsedStart") {
> +    switch(field) {
> +      case "firstUsedStart":
> -      return "firstUsed >= :" + field;
> +        return "firstUsed >= :" + field;

Btw. some people prefer if/else over switch as if/else if more flexible for the future. I would probably leave this alone.

::: toolkit/components/satchel/FormHistory.jsm:239
(Diff revision 1)
> +      case "firstUsedStart":
> -      return "firstUsed >= :" + field;
> +        return "firstUsed >= :" + field;
> -    } else if (field == "firstUsedEnd") {
> +      case "firstUsedEnd":

You should brace `case` bodies too

::: toolkit/components/satchel/FormHistory.jsm:247
(Diff revision 1)
> -    }
> +      default:
> -    return field + " = :" + field;
> +        return field + " = :" + field;
> +    }

I think this is a case similar to "no `else` after a `return`. It should be outside the switch like it was outside the `if…else` if you want to switch to a `switch`

::: toolkit/components/satchel/nsFormAutoComplete.js:210
(Diff revision 1)
>              let self = this._self;
> -            if (topic == "nsPref:changed") {
> +
> +            if (topic != "nsPref:changed") {
> +              return;
> +            }
> +  
> -                let prefName = data;
> +            let prefName = data;
> -                self.log("got change to " + prefName + " preference");
> +            self.log("got change to " + prefName + " preference");
>  

I would revert this change so that it's easier to add a new observer here and avoid needlessly changing blame.

::: toolkit/components/satchel/nsFormAutoComplete.js:215
(Diff revision 1)
>              let self = this._self;
> -            if (topic == "nsPref:changed") {
> +
> +            if (topic != "nsPref:changed") {
> +              return;
> +            }
> +  

Nit: trailing whitespace

::: toolkit/components/satchel/nsFormAutoComplete.js:262
(Diff revision 1)
>      log(message) {
> -        if (!this._debug)
> +        if (!this._debug) {
>              return;
> +        }
>          dump("FormAutoComplete: " + message + "\n");
>          Services.console.logStringMessage("FormAutoComplete: " + message);
>      },

In another bug you could switch this to use console.jsm's maxLogLevelPref if you want. It gives nicer coloured output.
Attachment #8868349 - Flags: review?(MattN+bmo)
Comment on attachment 8868350 [details]
Bug 1365068 - normalize indentation to 2 spaces.

https://reviewboard.mozilla.org/r/139920/#review144278

::: toolkit/components/satchel/nsFormAutoComplete.js:298
(Diff revision 1)
> -        }
> +    }
>  
> -        let client = new FormHistoryClient({ formField: aField, inputName: aInputName });
> +    let client = new FormHistoryClient({ formField: aField, inputName: aInputName });
>  
> -        // If we have datalist results, they become our "empty" result.
> +    // If we have datalist results, they become our "empty" result.
> -        let emptyResult = aDatalistResult ||
> +    let emptyResult = aDatalistResult || new  FormAutoCompleteResult(

Nit: there are two spaces before `FormAutoCompleteResult`

::: toolkit/components/satchel/nsFormAutoComplete.js:299
(Diff revision 1)
>  
> -        let client = new FormHistoryClient({ formField: aField, inputName: aInputName });
> +    let client = new FormHistoryClient({ formField: aField, inputName: aInputName });
>  
> -        // If we have datalist results, they become our "empty" result.
> +    // If we have datalist results, they become our "empty" result.
> -        let emptyResult = aDatalistResult ||
> -                          new FormAutoCompleteResult(client, [],
> +    let emptyResult = aDatalistResult || new  FormAutoCompleteResult(
> +                                                client, 

Nit: trailing whitespace

::: toolkit/components/satchel/nsFormAutoComplete.js:526
(Diff revision 1)
> -            boundaryCalc += (entry.textLowerCase.indexOf(" " + token) >= 0);
> +      boundaryCalc += (entry.textLowerCase.indexOf(" " + token) >= 0);
> -        }
> +    }
> -        boundaryCalc = boundaryCalc * this._boundaryWeight;
> +    boundaryCalc = boundaryCalc * this._boundaryWeight;
> -        // now add more weight if we have a traditional prefix match and
> +    // now add more weight if we have a traditional prefix match and
> -        // multiply boundary bonuses by boundary weight
> +    // multiply boundary bonuses by boundary weight
> -        boundaryCalc += this._prefixWeight *
> +    boundaryCalc += this._prefixWeight * (entry.textLowerCase.indexOf(aSearchString) == 0);

For a future bug you could switch this to .startsWith

::: toolkit/components/satchel/nsFormAutoComplete.js:533
(Diff revision 1)
>  function FormAutoCompleteResult(client,
> -                                entries,
> +                entries,
> -                                fieldName,
> +                fieldName,
> -                                searchString,
> +                searchString,
> -                                messageManager) {
> +                messageManager) {

Align these again please
Attachment #8868350 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8868351 [details]
Bug 1365068 - import with destructuring, switch to shorthand function declarations and use array.includes.

https://reviewboard.mozilla.org/r/139922/#review144284

Thanks!

::: toolkit/components/satchel/AutoCompletePopup.jsm:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -const Cc = Components.classes;
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Nit: We normally like to include all 4 commonly used nowadays (with Cr) (in the same order)

::: toolkit/components/satchel/FormHistory.jsm:88
(Diff revision 1)
> -const Cc = Components.classes;
> +const { classes: Cc, interfaces: Ci, results: Cr } = Components;
> -const Ci = Components.interfaces;
> -const Cr = Components.results;
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Cu would be useful here (can be done separately)
Attachment #8868351 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8868349 [details]
Bug 1365068 - Added missing brackets & refactored some conditional statements.

https://reviewboard.mozilla.org/r/139908/#review144346

Thanks!
Attachment #8868349 - Flags: review?(MattN+bmo) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c53adee3f113
Cleanup getListSuggestions. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7fcc4acd547f
Added missing brackets & refactored some conditional statements. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5230c1252423
normalize indentation to 2 spaces. r=MattN
https://hg.mozilla.org/integration/autoland/rev/a6095408fced
import with destructuring, switch to shorthand function declarations and use array.includes. r=MattN
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: