486810 - Engines (and their trackers) are created with every new window

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Ed Lee :Mardak]

This behavior causes a lot of tracker activity such as every page visit results in X onVisit verbose logs.. one for each tracker added (from opening a window).

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Have Engines.register take the engine object instance of an instance so that it can grab the singleton instance of the engine. Define a singleton instance getter on the base Engine, but make Engine "virtual" while creating only one instance of the sub-engine.

Comment 2

[Dan Mills [:thunder]]

Comment on attachment 371026 [details] [diff] [review]
v1

We should not try to register engines on every window open, this patch doesn't fix that.

Furthermore, the .instance trick is clever, but it would be simpler to just have EngineManager error/warn when attempting to replace an engine already set.

Comment 3

[Ed Lee :Mardak]

Attachment: v2

Moved engine registration to onStartup

Comment 4

[Dan Mills [:thunder]]

Comment on attachment 371683 [details] [diff] [review]
v2


>   register: function EngMgr_register(engineObject) {
>     if (Utils.isArray(engineObject))
>       return engineObject.map(this.register, this);
> 
>     try {
>-      let engine = new engineObject();
>+      let engine = engineObject.prototype.instance;
>+
>+      if (engine.name in this._engines)
>+        this._log.warn("Engine '" + engine.name + "' is already registered!");
>+
>       this._engines[engine.name] = engine;
>     }

why not do this instead?

if (engineObj.name in this._engines)
  this._log.error("Engine '" + engine.name + "' is already registered!");
else
  this._engines[engineObj.name] = new engineObj()

It's simpler and does the same thing basically.  The instance getter is unnecessary because we already have the name which must be unique.

The rest looks great, so with the change above, r+

Comment 5

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/e75c8e36d1e3

Register the built-in engines on service start-up instead of from the overlay, and have Engines.register check if the engine has already been registered.