Add convenient engine manager APIs

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
7 years ago
5 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Created attachment 650322 [details] [diff] [review]
New engine manager APIs, v1

I was looking the engine manager API and managed to fix a few DRY violations.
Attachment #650322 - Flags: review?(rnewman)
Comment on attachment 650322 [details] [diff] [review]
New engine manager APIs, v1

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

::: services/sync/modules/engines.js
@@ +393,5 @@
>    },
> +
> +  get names() {
> +    for (let name in Engines._engines) {
> +      yield name;

Unnecessarily fancy. I would guess that the generator approach is more expensive than filling and returning a 7-item array, and you might well introduce two classes of bugs: an order-of-evaluation bug (mutating _engines during use), and a type bug (which I think you did).

@@ +407,5 @@
> +        continue;
> +      }
> +
> +      yield name;
> +    }

Same.

::: services/sync/modules/service.js
@@ +1337,5 @@
>      }
>  
>      this._ignorePrefObserver = true;
>  
> +    let enabled = Engines.enabledNamed;

Typo.

               /**
		* An array of engine names for enabled engines.
		*/
		get enabledNames() {

There should be a QA step or a test that would allow you to spot this, and get an error on line 1347 ("enabled is undefined").

@@ +1343,5 @@
>        if (engineName == "clients") {
>          // Clients is special.
>          continue;
>        }
>        let index = enabled.indexOf(engineName);

You made enabledNames a generator. As I understand it, there's no automatic coercion to Array, so I would expect this to throw:

TypeError: enabled.indexOf is not a function
Attachment #650322 - Flags: review?(rnewman) → review-
Still want to do this, gps?
Assignee: nobody → gps
Status: NEW → ASSIGNED
Flags: needinfo?(gps)
(Assignee)

Comment 3

6 years ago
Meh.

I reckon this will be addressed naturally as part of larger refactors.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: needinfo?(gps)
Resolution: --- → WONTFIX
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.