781349 - Add convenient engine manager APIs

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: New engine manager APIs, v1

I was looking the engine manager API and managed to fix a few DRY violations.

Comment 1

[Richard Newman [: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

Comment 2

[Richard Newman [:rnewman]]

Still want to do this, gps?

Comment 3

[Gregory Szorc [:gps]]

Meh.

I reckon this will be addressed naturally as part of larger refactors.