Stop preprocessing nsSearchService.js and make it pass eslint

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Search
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Removing the preprocessing of nsSearchService.js is an advantage for developers - less rebuilds required. Now we have AppConstants (xref bug 1150859), we can easily replace the ifdefs with runtime checks. I think the cost of the checks will be small, there's nothing too significant that we're testing.

Additionally, we should then make it pass eslint, so that we can get the extra goodness that brings for development.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
There's a failure on the unified autocomplete tests that I'll look into tomorrow.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Fixed the issues in the tests - I'd missed replacing a `throw new Task.Result`.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8806483 [details]
Bug 1314373 - Change nsSearchService.js so that it doesn't need to be preprocessed.

https://reviewboard.mozilla.org/r/89898/#review89668

::: toolkit/components/search/nsSearchService.js:146
(Diff revision 2)
>  const LOCALE_PREF = "general.useragent.locale";
>  
>  const USER_DEFINED = "{searchTerms}";
>  
>  // Custom search parameters
> -#ifdef MOZ_OFFICIAL_BRANDING
> +const MOZ_OFFICIAL = AppConstants.MOZ_OFFICIAL ? "official" : "unofficial";

AppConstants.MOZ_OFFICIAL doesn't exist, you likely wanted AppConstants.MOZ_OFFICIAL_BRANDING?

::: toolkit/components/search/nsSearchService.js:242
(Diff revision 2)
> +if (AppConstants.DEBUG) {
> -function PREF_LOG(aText) {
> +  function PREF_LOG(aText) {
>    if (getBoolPref(BROWSER_SEARCH_PREF + "log", false))
>      DO_LOG(aText);
> -}
> +  }
> -var LOG = PREF_LOG;
> +  LOG = PREF_LOG;

nit: I don't see the point of defining PREF_LOG and then assigning to LOG versus directly doing
LOG = function (aText) {
  ...

::: toolkit/components/search/nsSearchService.js:2230
(Diff revision 2)
>        if (scheme == "chrome") {
>          packageName = uri.hostPort;
>          uri = gChromeReg.convertChromeURL(uri);
>        }
>  
> -#ifdef ANDROID
> +      if (AppConstants.ANDROID) {

Did you mean AppConstants.platform == "android" ?
Is there a reason to add a further constant?
Attachment #8806483 - Flags: review?(mak77)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8806484 [details]
Bug 1314373 - Fix parser issues raised by eslint for nsSearchService.js.

https://reviewboard.mozilla.org/r/89900/#review89682

::: toolkit/components/search/nsSearchService.js:1411
(Diff revision 2)
>     *
>     * @returns {Promise} A promise, resolved successfully if initializing from
>     * data succeeds, rejected if it fails.
>     */
>    _asyncInitFromFile: function SRCH_ENG__asyncInitFromFile(file) {
> -    return Task.spawn(function() {
> +    return Task.spawn(function* () {

_asyncInitFromFile: Task.async(function* (file) {
  ...
}),
This way we also avoid the bind.

Note that this happens multiple times in the patch, but I'm not commenting every instance of it to avoid noise. It would be great to convert all of them.

::: toolkit/components/search/nsSearchService.js:2770
(Diff revision 2)
>     * @returns {Promise} A promise, resolved successfully if the initialization
>     * succeeds.
>     */
>    _asyncInit: function SRCH_SVC__asyncInit() {
>      migrateRegionPrefs();
> -    return Task.spawn(function() {
> +    return Task.spawn(function* () {

ditto (I don't see a reason for migrateRegionPrefs() to be outside of the Task, since it's executes synchronously until the first yield)

::: toolkit/components/search/nsSearchService.js:2788
(Diff revision 2)
>        try {
>          yield checkForSyncCompletion(ensureKnownCountryCode(this));
> -      } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> +      } catch (ex) {
> +        if (ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> -        LOG("_asyncInit: failure determining country code: " + ex);
> +          LOG("_asyncInit: failure determining country code: " + ex);
> +        } else {

nit: personally I prefer to invert these to avoid the else branch, so:

catch (ex) {
  if (ex...) {
    throw...
  }
  do_something
}

::: toolkit/components/search/nsSearchService.js:2798
(Diff revision 2)
>          yield checkForSyncCompletion(this._asyncLoadEngines(cache));
> -      } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> +      } catch (ex) {
> +        if (ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> -        this._initRV = Cr.NS_ERROR_FAILURE;
> +          this._initRV = Cr.NS_ERROR_FAILURE;
> -        LOG("_asyncInit: failure loading engines: " + ex);
> +          LOG("_asyncInit: failure loading engines: " + ex);
> +        } else {

nit: and invert this

::: toolkit/components/search/nsSearchService.js:3494
(Diff revision 2)
>  
>      // Check whether aDir is the user profile dir
>      let isInProfile = aDir.equals(getDir(NS_APP_USER_SEARCH_DIR));
>      let dirPath = aDir.path;
>      let iterator = new OS.File.DirectoryIterator(dirPath);
> -    return Task.spawn(function() {
> +    return Task.spawn(function* () {

ditto (another case where it doesn't make sense to keep stuff out of the task)

::: toolkit/components/search/nsSearchService.js:3529
(Diff revision 2)
> -        } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> +        } catch (ex) {
> +          if (ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> -          LOG("_asyncLoadEnginesFromDir: Failed to load " + osfile.path + "!\n" + ex);
> +            LOG("_asyncLoadEnginesFromDir: Failed to load " + osfile.path + "!\n" + ex);
> -          continue;
> +            continue;
> -        }
> +          }
> +          throw ex;

in both cases we don't want to call engines.push(addedEngine); since either we throw or we continue, so why don't we move that into the try body an avoid the confusing "continue" in the catch?

::: toolkit/components/search/nsSearchService.js:3573
(Diff revision 2)
>            let uri = NetUtil.newURI(url);
>            let engine = new Engine(uri, true);
>            yield checkForSyncCompletion(engine._asyncInitFromURI(uri));
>            engines.push(engine);
> -        } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {
> +        } catch (ex) {
> +          if (ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) {

nit: I'd also invert this to avoid the else

::: toolkit/components/search/nsSearchService.js:3936
(Diff revision 2)
> -          TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
> +            TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
> -        } catch (ex) {
> +          } else {
> -          self._initObservers.reject(ex);
> +            self._initObservers.reject(ex);
> -          TelemetryStopwatch.cancel("SEARCH_SERVICE_INIT_MS");
> +            TelemetryStopwatch.cancel("SEARCH_SERVICE_INIT_MS");
> -        }
> +          }
> +        }

couldn't we move TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS") into a finally?
Attachment #8806484 - Flags: review?(mak77)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8806485 [details]
Bug 1314373 - Fix general eslint issues in nsSearchService.js.

https://reviewboard.mozilla.org/r/89902/#review89694
Attachment #8806485 - Flags: review?(mak77) → review+
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8806484 [details]
Bug 1314373 - Fix parser issues raised by eslint for nsSearchService.js.

https://reviewboard.mozilla.org/r/89900/#review89682

> couldn't we move TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS") into a finally?

Discussed over irc, and realised that the `ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED` state calls `.cancel` rather than `.finish`
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8806483 [details]
Bug 1314373 - Change nsSearchService.js so that it doesn't need to be preprocessed.

https://reviewboard.mozilla.org/r/89898/#review90042

::: toolkit/components/search/nsSearchService.js:239
(Diff revision 3)
>   */
> -function PREF_LOG(aText) {
> +var LOG = function(){};
> +
> +if (AppConstants.DEBUG) {
> +  LOG = function (aText) {
>    if (getBoolPref(BROWSER_SEARCH_PREF + "log", false))

mozreview is showing a wrong indentation here, I'm honestly not sure if it's real or just a mozreview artefact.
Attachment #8806483 - Flags: review?(mak77) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8806484 [details]
Bug 1314373 - Fix parser issues raised by eslint for nsSearchService.js.

https://reviewboard.mozilla.org/r/89900/#review90046

::: toolkit/components/search/nsSearchService.js:3059
(Diff revision 3)
> -          iterator.close();
> +        iterator.close();
> -        }
> +      }
> -      }
> +    }
>  
> -      function hasModifiedDir(aList) {
> +    function hasModifiedDir(aList) {
> -        return Task.spawn(function() {
> +      return Task.spawn(function* () {

nit: could be
let hasModifieDir = Task.async(function* (aList) {
Attachment #8806484 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #17)
> nit: could be
> let hasModifieDir = Task.async(function* (aList) {

I have a typo here, should have been hasModifiedDir
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8806483 [details]
Bug 1314373 - Change nsSearchService.js so that it doesn't need to be preprocessed.

https://reviewboard.mozilla.org/r/89898/#review90042

> mozreview is showing a wrong indentation here, I'm honestly not sure if it's real or just a mozreview artefact.

It was real, fixed.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c3a3cf3d6ad
Change nsSearchService.js so that it doesn't need to be preprocessed. r=mak
https://hg.mozilla.org/integration/autoland/rev/003e0465a68e
Fix parser issues raised by eslint for nsSearchService.js. r=mak
https://hg.mozilla.org/integration/autoland/rev/df0b813ff4fe
Fix general eslint issues in nsSearchService.js. r=mak

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c3a3cf3d6ad
https://hg.mozilla.org/mozilla-central/rev/003e0465a68e
https://hg.mozilla.org/mozilla-central/rev/df0b813ff4fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.