1058618 - ZoomManager's properties zoomValues, MIN and MAX should be observed and updated while Firefox is open

Status: NEW

(Firefox :: General, defect)

Description

[u515644]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715214335

Steps to reproduce:

When the preference toolkit.zoomManager.zoomValues is altered, toolkit's ZoomManager should immediately return the updated values for ZoomManager.zoomValues, ZoomManager.MIN and ZoomManager.MAX when those are requested in currently opened windows. Right now, the values are being cached, prohibiting currently opened windows to have updated zoom values, leading to unexpected zoom behaviour.

Steps to reproduce:

1. In an already opened window navigate to about:config
2. Alter preference toolkit.zoomManager.zoomValues by delete a few of the highest zoom values, for instance, such that the new highest zoom value is 2 (200%) in stead of 3 (300%), for instance.
3. Now, zoom in as far as you can (with Ctrl-+, for instance).

Removing any zoom value will do, actually. You'll see that you will still be allowed to zoom to those levels through means that utilize ZoomManager.enlarge(), ZoomManager.reduce() and associated functionality.


Actual results:

Since ZoomManager caches ZoomManager.zoomValues, ZoomManager.MIN and ZoomManager.MAX in its associated window, you are still allowed to zoom in further than the new highest zoom value, in any currently opened window. In other words: the updated zoom values don't take effect immediately in present windows. Newly opened windows will, however, correctly halt at the new highest zoom value.

The reason is the following code:

var ZoomManager = {

  /* irrelevant code omitted */

  get MIN() {
    delete this.MIN;
    return this.MIN = this._prefBranch.getIntPref("zoom.minPercent") / 100;
  },

  get MAX() {
    delete this.MAX;
    return this.MAX = this._prefBranch.getIntPref("zoom.maxPercent") / 100;
  },

  /* irrelevant code omitted */

  get zoomValues() {
    var zoomValues = this._prefBranch.getCharPref("toolkit.zoomManager.zoomValues")
                                     .split(",").map(parseFloat);
    zoomValues.sort(function (a, b) a - b);

    while (zoomValues[0] < this.MIN)
      zoomValues.shift();

    while (zoomValues[zoomValues.length - 1] > this.MAX)
      zoomValues.pop();

    delete this.zoomValues;
    return this.zoomValues = zoomValues;
  },

  /* irrelevant code omitted */
}

As you can see, this code replaces the getters with permanent values after they have been accessed for the first time.

Since I'm currently building a zoom extension that will allow one to define custom zoom values, future consumers of this extension will currently need to reopen windows to have the zoom values take effect, unless I substitute the ZoomManager instances with my adjusted ZoomManager.
The former is far from user-friendly, as you can imagine, the later solution feels rather hackish to me as a developer and is probably prone to unforeseeable side-effects.


Expected results:

Zooming in should halt at the new highest zoom level in currently opened window. This could be achieved by altering the code of ZoomManager to this, for instance:

var ZoomManager = {

  /* irrelevant code omitted */

  get MIN() {
    return this._prefBranch.getIntPref("zoom.minPercent") / 100;
  },

  get MAX() {
    return this._prefBranch.getIntPref("zoom.maxPercent") / 100;
  },

  /* irrelevant code omitted */

  get zoomValues() {
    var zoomValues = this._prefBranch.getCharPref("toolkit.zoomManager.zoomValues")
                                     .split(",").map(parseFloat);
    zoomValues.sort(function (a, b) a - b);

    while (zoomValues[0] < this.MIN)
      zoomValues.shift();

    while (zoomValues[zoomValues.length - 1] > this.MAX)
      zoomValues.pop();

    return zoomValues;
  },

  /* irrelevant code omitted */
}

Comment 1

[u515644]

I failed to mention that ZoomManager.MIN and ZoomManager.MAX are depending on the preferences zoom.minPercent and zoom.maxPercent of course, not on toolkit.zoomManager.zoomValues.

Furthermore, perhaps my suggested code adjustments are a bit too inefficient. At a minimum though, I think ZoomManager should then observe mentioned preferences for changes and adjust its own values accordingly.

Comment 2

[u515644]

Attachment: A quick proposal for an adjusted ZoomManager. (Sorry, there's a typo on line 27: observer: should read observe:)

This proposal incorporates my suggested observables. It is a little quick and dirty though. Furthermore, the init and destroy methods should be hooked up somewhere. I have no idea where this should take place though. I don't have enough knowledge about the internals of Firefox.

Comment 3

[Marco Bonardo [:mak]]

not to stop-energy, but I feel like the cost here overcomes any intended gains. It's unlikely that someone needs to change those values, and even more unlikely that he might need to do that more than once in a long time.

I think the current code did the right choice avoiding the code bloat and perf hit needed to observe these prefs. A restart is not a big deal for such low traffic prefs.