Bugzilla

(In reply to dw-dev from comment #3)
> (b) / (c)  Why can't the method that was used for persisting the zoom level in a non-sites-specific tab, prior to implementing a configurable Default Zoom level, still be used?

The problem is that there wasn't a "method" for persisting it before, it was just never changed - if you disabled site-specific zoom, we just avoided ever telling the docshell any zoom level from the preferences.
Now we **do** need to do that, because by default it starts out at 1, and if the default zoom is not 1, it needs setting. I guess we could avoid calling the setter if the default zoom is `1` (ie 100%) like in the steps in comment #0, but as you already implied, that is not going to help most people, just the narrow steps in comment #0.

(In reply to Morgan Reschenberg [:morgan] from comment #2)
> Hmm so best I can tell this is happening because on refresh we call onLocationChange [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#216] which doesn't hit this case [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#235] but instead trickles down to here [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#290] where we get a cached result for the current domain and an undefined value instead of the current domain and its current value. We then try to apply this undefined value and skip the case I added in my previous patch [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#469] and go directly to "apply global zoom". 
> 
> So a few questions maybe :gijs can provide thoughts on:
> 
> (a) should we avoid calling apply on location changes for pages that are being refreshed? is that a possible solution here? or 

That wouldn't be sufficient, unfortunately.

> (b) should we adjust that caching case in onLocationChange to check if the pref has a valid value before attempting to apply it? Not sure if that'd change anything, because it seems like maybe we're just not saving the adjusted zoom values anywhere, so: 

Right. That last part ("not saving the adjusted zoom values anywhere") is the issue. Fundamentally, right now we have three (in our case, just 2) sources of information:

- the CPS per origin (disabled if sitespecific zoom is off)
- the global zoom
- the docshell / browser's zoom level int property

The app settings/preferences only control the global zoom. Each browser starts with zoom level 1 (ie 100%). The zoom controls and shortcuts control the docshell/browser (and, if site-specific zoom is on, the per-origin prefs). But there's (currently) no way after the fact to figure out if the zoom level known to the docshell/browser was the result of user actions or is just there because it's a new tab and it never got set. There's no other bit of information, just the two zoom levels, and no way to know which is "right".

When reviewing the change in bug 1603494 I don't think I realized that the codepath that was modified would also be hit on every location change, not just when the tab is opened / gets created - Sorry!

> (c) is there a way to write to CPS per tab (like, with a PID or something?) instead of per-domain so we can "save" the adjusted value of non-site-specific tabs and apply that on refresh consistently?

No, there isn't.

Fundamentally, to fix this I think we're going to want to avoid setting the zoom level on the browser when sitespecific zoom is turned on. However, we'll still need to set it when the browser is created to ensure that it gets the proper zoom level.

> then we'd apply global zoom to new tabs only? [wondering if https://bugzilla.mozilla.org/show_bug.cgi?id=1616305 is actually expected behaviour]

I don't think that's expected, no. In an ideal world, there'd be a way of only applying the zoom when the tab gets created and then leave things alone, but (i) I can't quickly find anything that we could use to distinguish that case and (ii) in any case I bet that works poorly with tab duplication / tearoff, where presumably users would expect the original and duplicate / torn-off tabs to have matching zoom levels, not to have them overridden with the global zoom.

So I'm tempted to add another bit of information... if we set some kind of bool marker on the tab to indicate that the zoom has changed on a per-tab basis, and check that before overriding it with the global zoom, I think that'd work? We could do that in `_applyZoomToPref`, which currently bails out early if `siteSpecific` is false, but could be made to simply set that bool before returning. Then we can check for that in the place where the `siteSpecific` check used to be before we removed it in bug 1603494. Then we can fix bug 1616305 by checking for the case where `siteSpecific` is false, and that flag is *not* yet set, and in that case, overwrite the zoom level anyway (even if the location change is caused by a tabswitch, which currently bails out quite early). Does that make some amount of sense? It's possible I'm still missing something...

(In reply to dw-dev from comment #3)
> (b) / (c)  Why can't the method that was used for persisting the zoom level in a non-sites-specific tab, prior to implementing a configurable Default Zoom level, still be used?

The problem is that there wasn't a "method" for persisting it before, it was just never changed - if you disabled site-specific zoom, we just avoided ever telling the docshell any zoom level from the preferences.
Now we **do** need to do that, because by default it starts out at 1, and if the default zoom is not 1, it needs setting. I guess we could avoid calling the setter if the default zoom is `1` (ie 100%) like in the steps in comment #0, but as you already implied, that is not going to help most people, just the narrow steps in comment #0.

(In reply to Morgan Reschenberg [:morgan] from comment #2)
> Hmm so best I can tell this is happening because on refresh we call onLocationChange [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#216] which doesn't hit this case [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#235] but instead trickles down to here [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#290] where we get a cached result for the current domain and an undefined value instead of the current domain and its current value. We then try to apply this undefined value and skip the case I added in my previous patch [https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/browser/base/content/browser-fullZoom.js#469] and go directly to "apply global zoom". 
> 
> So a few questions maybe :gijs can provide thoughts on:
> 
> (a) should we avoid calling apply on location changes for pages that are being refreshed? is that a possible solution here? or 

That wouldn't be sufficient, unfortunately.

> (b) should we adjust that caching case in onLocationChange to check if the pref has a valid value before attempting to apply it? Not sure if that'd change anything, because it seems like maybe we're just not saving the adjusted zoom values anywhere, so: 

Right. That last part ("not saving the adjusted zoom values anywhere") is the issue. Fundamentally, right now we have three (in our case, just 2) sources of information:

- the CPS per origin (disabled if sitespecific zoom is off)
- the global zoom
- the docshell / browser's zoom level int property

The app settings/preferences only control the global zoom. Each browser starts with zoom level 1 (ie 100%). The zoom controls and shortcuts control the docshell/browser (and, if site-specific zoom is on, the per-origin prefs). But there's (currently) no way after the fact to figure out if the zoom level known to the docshell/browser was the result of user actions or is just there because it's a new tab and it never got set. There's no other bit of information, just the two zoom levels, and no way to know which is "right".

When reviewing the change in bug 1603494 I don't think I realized that the codepath that was modified would also be hit on every location change, not just when the tab is opened / gets created - Sorry!

> (c) is there a way to write to CPS per tab (like, with a PID or something?) instead of per-domain so we can "save" the adjusted value of non-site-specific tabs and apply that on refresh consistently?

No, there isn't.

Fundamentally, to fix this I think we're going to want to avoid setting the zoom level on the browser when sitespecific zoom is turned on. However, we'll still need to set it when the browser is created to ensure that it gets the proper zoom level.

> then we'd apply global zoom to new tabs only? [wondering if https://bugzilla.mozilla.org/show_bug.cgi?id=1616305 is actually expected behaviour]

I don't think that's expected, no. In an ideal world, there'd be a way of only applying the zoom when the tab gets created and then leave things alone, but (i) I can't quickly find anything that we could use to distinguish that case and (ii) in any case I bet that works poorly with tab duplication / tearoff, where presumably users would expect the original and duplicate / torn-off tabs to have matching zoom levels, not to have them overridden with the global zoom.

So I'm tempted to add another bit of information... if we set some kind of bool marker (say, `_tabHasCustomZoom`) on the tab to indicate that the zoom has changed on a per-tab basis, and check that before overriding it with the global zoom, I think that'd work? We could do that in `_applyZoomToPref`, which currently bails out early if `siteSpecific` is false, but could be made to simply set that bool before returning. Then we can check for `_tabHasCustomZoom` in the place where the `siteSpecific` check used to be before we removed it in bug 1603494. Then we can fix bug 1616305 by checking for the case where `siteSpecific` is false, and `_tabHasCustomZoom` is *not* yet set, and in that case, overwrite the zoom level anyway (even if the location change is caused by a tabswitch, which currently bails out quite early). Does that make some amount of sense? It's possible I'm still missing something...