Closed Bug 1314373 Opened 8 years ago Closed 8 years ago

Stop preprocessing nsSearchService.js and make it pass eslint

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(3 files)

Bug 1314373 - Change nsSearchService.js so that it doesn't need to be preprocessed.
58 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1314373 - Fix parser issues raised by eslint for nsSearchService.js.
58 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1314373 - Fix general eslint issues in nsSearchService.js.
58 bytes, text/x-review-board-request
mak
: review+
Details 
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.
There's a failure on the unified autocomplete tests that I'll look into tomorrow.
Fixed the issues in the tests - I'd missed replacing a `throw new Task.Result`.
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 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 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+
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 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 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
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: