Closed Bug 386363 Opened 17 years ago Closed 14 years ago

Remember zoom value on a site-specific basis (Port fix for bug 378549 to SeaMonkey)

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: raj, Assigned: kairo)

References

Details

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070509 MultiZilla/1.8.3.2i SeaMonkey/1.1.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070509 MultiZilla/1.8.3.2i SeaMonkey/1.1.2

Bug 108391 -- text size (zoom) should be remembered across browser sessions, per-site -- was fixed by bug 378549 for Firefox.  According to bug 108391 comment 50 this is for Firefox only but that the content prefs service is Toolkit so can be ported to SuiteRunner.

Reproducible: Didn't try
This is still a port of Firefox bug 378549 but I'm copying the latter's Summary to make it clearer what this bug is about.

Sm-trunk prefs are indeed being ported to a Toolkit backend: some parts of it are already done, others not. I'm not sure where this belongs in Sm prefs but if it's in Appearance or one of its subpanes they are already ported.

Adding a few CC but not sure I made the right choices -- feel free to add others or to remove yourself.
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: General → Preferences
Ever confirmed: true
QA Contact: general → prefs
Summary: Port fix for bug 378549 to SuiteRunner → Remember text zoom value on a site-specific basis
Version: unspecified → Trunk
Just putting back the link to the firefox bug in the summary.
Summary: Remember text zoom value on a site-specific basis → Remember text zoom value on a site-specific basis (Port fix for bug 378549 to SuiteRunner)
Do we have Myk's site preference service on trunk?
Depends on: 378549
(In reply to comment #3)
> Do we have Myk's site preference service on trunk?

The patch from bug 378547 only touched toolkit/ so I guess the answer is Yes (I didn't check any further, though).
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-fullZoom.js
is where this functionality sits in Firefox, and this is included into
browser.js - should not be too hard to port this over.

BTW, the content prefs this sets can be managed with the new Data Manager I'm
developing.
Depends on: 583317
I have a version of this working on the browser side, including a switch to toolkit's ZoomManager, but I need to do some more work for mailnews (ZommManager now uses getBrowser() and that seems to not exist there atm).
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Target Milestone: --- → seamonkey2.1a3
As part of work on bug 583317, there is likely relevant code in suite/browser/nsBrowserStatusHandler.js, leaving it here as a memo to include as part of patch for this bug. I'm going to remove it from my patch as it fills my console with errors.

in function onUpdateCurrentBrowser:

+  onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
+    if (FullZoom.updateBackgroundTabs)
+      FullZoom.onLocationChange(gBrowser.currentURI, true);
(In reply to comment #8)
> +    if (FullZoom.updateBackgroundTabs)
> +      FullZoom.onLocationChange(gBrowser.currentURI, true);

Yes, leave those out of your patch, I'm adding that over here.
Depends on: 588895
Here's a WIP patch as a checkpoint. With the patches in the dependencies applied, this works, even in mailnews, but when used there, it currently leaves error messages in the error console about gInPrintPreviewMode being not defined when only used in the if () clauses - which confuses me because I thought this being not defined would just be interpreted like false (actually undefined) in an if.
Hah, attaching a patch and looking at it makes one see interesting stuff - the addition of fullZoom.js is a remnant and not needed, it's dead even in the patch. Removed locally.
This patch depends on bug 583317 to apply changed prefs when changing tabs.

Apart from that, nobody told me here, but I finally found out that using window.varname, I can catch undefined vars (nicely, I can check for the var being true or false when actually defined at the same time).

With that, this patch seems to work fine in both browser an mailnews.
Attachment #467499 - Attachment is obsolete: true
Attachment #472268 - Flags: review?(neil)
Comment on attachment 472268 [details] [diff] [review]
v1: switch to toolkit ZoomManager, save site-specific zoom

>   // simulate all change notifications after switching tabs
>   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
>+    if (FullZoom.updateBackgroundTabs)
>+      FullZoom.onLocationChange(gBrowser.currentURI, true);
I don't understand this; onUpdateCurrentBrowser only fires for the foreground tab. I also couldn't see where else updateBackgroundTabs gets used.

You can avoid the dependency on the other bug by moving the tab switch update code to the same place as the star button.
(In reply to comment #13)
> >   // simulate all change notifications after switching tabs
> >   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
> >+    if (FullZoom.updateBackgroundTabs)
> >+      FullZoom.onLocationChange(gBrowser.currentURI, true);
> I don't understand this; onUpdateCurrentBrowser only fires for the foreground
> tab. I also couldn't see where else updateBackgroundTabs gets used.

It gets fired when background tabs that should be updated come into the foreground, right? While they're in the background, we don't need to update them, as they're not visible and we don't need to waste perf on them, but when they come into the foreground, we need to update their zoom level.

> You can avoid the dependency on the other bug by moving the tab switch update
> code to the same place as the star button.

That would make us depart from the code paths that FF uses as well, I guess you rarely do code-porting jobs where the code needs to be maintained into the future. OTOH, we are so far off with a lot of this code already that nobody who is sane can follow code changes on their side anyhow to backport them.
(In reply to comment #14)
> (In reply to comment #13)
> > >   // simulate all change notifications after switching tabs
> > >   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
> > >+    if (FullZoom.updateBackgroundTabs)
> > >+      FullZoom.onLocationChange(gBrowser.currentURI, true);
> > I don't understand this; onUpdateCurrentBrowser only fires for the foreground
> > tab. I also couldn't see where else updateBackgroundTabs gets used.
> It gets fired when background tabs that should be updated come into the
> foreground, right? While they're in the background, we don't need to update
> them, as they're not visible and we don't need to waste perf on them, but when
> they come into the foreground, we need to update their zoom level.
That sounds great, but I don't see where you avoid updating tabs that are in the background.

> > You can avoid the dependency on the other bug by moving the tab switch update
> > code to the same place as the star button.
> That would make us depart from the code paths that FF uses as well, I guess you
> rarely do code-porting jobs where the code needs to be maintained into the
> future. OTOH, we are so far off with a lot of this code already that nobody who
> is sane can follow code changes on their side anyhow to backport them.
Ah, so Firefox developers having arbitrarily decided whether a particular UI update (e.g. PlacesStarButton) belongs either in onLocationChange or onUpdateCurrentBrowser, we then have to slavishly follow their decision, rather than keeping all our UI updates in one place?
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > >   // simulate all change notifications after switching tabs
> > > >   onUpdateCurrentBrowser: function (aStateFlags, aStatus, aMessage, aTotalProgress) {
> > > >+    if (FullZoom.updateBackgroundTabs)
> > > >+      FullZoom.onLocationChange(gBrowser.currentURI, true);
> > > I don't understand this; onUpdateCurrentBrowser only fires for the foreground
> > > tab. I also couldn't see where else updateBackgroundTabs gets used.
> > It gets fired when background tabs that should be updated come into the
> > foreground, right? While they're in the background, we don't need to update
> > them, as they're not visible and we don't need to waste perf on them, but when
> > they come into the foreground, we need to update their zoom level.
> That sounds great, but I don't see where you avoid updating tabs that are in
> the background.

I don't see a place I added here that actually updates background tabs. Is that enough?

> > > You can avoid the dependency on the other bug by moving the tab switch update
> > > code to the same place as the star button.
> > That would make us depart from the code paths that FF uses as well, I guess you
> > rarely do code-porting jobs where the code needs to be maintained into the
> > future. OTOH, we are so far off with a lot of this code already that nobody who
> > is sane can follow code changes on their side anyhow to backport them.
> Ah, so Firefox developers having arbitrarily decided whether a particular UI
> update (e.g. PlacesStarButton) belongs either in onLocationChange or
> onUpdateCurrentBrowser, we then have to slavishly follow their decision, rather
> than keeping all our UI updates in one place?

Erm, onLocationChange is only fired when the location of a tab changes, or only if the location of the location bar changes? So, say, I have two tabs open with the same URL and zoom one of them, having the default setting of site-specific zoom on. Now I switch to the other one. Does onLocationChange fire so I can update the zoom level of that tab?
(In reply to comment #16)
> (In reply to comment #15)
> > That sounds great, but I don't see where you avoid updating tabs that are in
> > the background.
> I don't see a place I added here that actually updates background tabs. Is that
> enough?
Then I don't understand the point of the pref at all.

> Erm, onLocationChange is only fired when the location of a tab changes, or only
> if the location of the location bar changes? So, say, I have two tabs open with
> the same URL and zoom one of them, having the default setting of site-specific
> zoom on. Now I switch to the other one. Does onLocationChange fire so I can
> update the zoom level of that tab?
Yes, it does fire whenever the location bar changes. (Otherwise the PlacesStarButton wouldn't update correctly!)
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > That sounds great, but I don't see where you avoid updating tabs that are in
> > > the background.
> > I don't see a place I added here that actually updates background tabs. Is that
> > enough?
> Then I don't understand the point of the pref at all.

Well, I just ported having the pref - I for myself am fully happy with always updating tabs that come from the background, regardless of a pref.

> > Erm, onLocationChange is only fired when the location of a tab changes, or only
> > if the location of the location bar changes? So, say, I have two tabs open with
> > the same URL and zoom one of them, having the default setting of site-specific
> > zoom on. Now I switch to the other one. Does onLocationChange fire so I can
> > update the zoom level of that tab?
> Yes, it does fire whenever the location bar changes. (Otherwise the
> PlacesStarButton wouldn't update correctly!)

Well, so, how does the location bar update when I switch between two tabs that that have the same page loaded, then?
(In reply to comment #18)
> > Then I don't understand the point of the pref at all.
> Well, I just ported having the pref - I for myself am fully happy with always
> updating tabs that come from the background, regardless of a pref.
I guess I'd better read the Firefox code to see whether it makes sense...

> > Yes, it does fire whenever the location bar changes. (Otherwise the
> > PlacesStarButton wouldn't update correctly!)
> Well, so, how does the location bar update when I switch between two tabs that
> that have the same page loaded, then?
OK, so I was being a bit simplistic: it fires when the location bar might change (so for instance it fires when you browse Google Maps).
(In reply to comment #19)
> I guess I'd better read the Firefox code to see whether it makes sense...
No, it makes no sense; the pref seems to be utterly useless.
Comment on attachment 472268 [details] [diff] [review]
v1: switch to toolkit ZoomManager, save site-specific zoom

Minusing because the updateBackgroundTabs pref doesn't work.

Note: An updateBackgroundTabs pref is not necessary for the initial feature.
Attachment #472268 - Flags: review?(neil) → review-
(In reply to comment #21)
> Comment on attachment 472268 [details] [diff] [review]
> v1: switch to toolkit ZoomManager, save site-specific zoom
> 
> Minusing because the updateBackgroundTabs pref doesn't work.
> 
> Note: An updateBackgroundTabs pref is not necessary for the initial feature.

I actually disagree, as that's one of the great things this feature does. But if SeaMonkey sucks, what do I care, just one reason more to switch myself to Firefox, as much as it will hurt my pride for a while.
(In reply to comment #21)
> (From update of attachment 472268 [details] [diff] [review])
> Minusing because the updateBackgroundTabs pref doesn't work.
So, it turns out that this pref has nothing to do with updating tabs in the background, or updating tabs that are in the background. Instead its job is to update tabs that were in the background. No wonder I was confused...
Yes, that's exactly it. :)

This patch uses onLocationChange and tries to explain the pref better, I hope that's in a shape that we can take it.
Attachment #472268 - Attachment is obsolete: true
Attachment #494578 - Flags: review?(neil)
Comment on attachment 494578 [details] [diff] [review]
v1.1: use onLocationChange, explain pref better

>@@ -377,16 +377,20 @@ onLocationChange : function
>+    // When background tab comes into foreground, might want to update zoom.
>+    if (FullZoom.updateBackgroundTabs)
>+      FullZoom.onLocationChange(gBrowser.currentURI, true);
There are three problems with this. The first is that you're catching changes for frames, not just top-level changes. Fortunately there's a convenient block of code that already listens for top-level changes where you could move this. Secondly this listens for foreground as well as tab-switch changes. The difference is that aRequest is null for a tab-switch change. So you could pass !aRequest as the aIsTabSwitch parameter and forget the all tabs listener. Finally you don't pass the browser to onLocationChange. I'm concerned that this will cause problems if you switch tabs quickly. Can you not pass in browser (conveniently defined earlier in onLocationChange)?

>+  _handleMouseScrolled: function FullZoom__handleMouseScrolled(event) {
[I don't like the internal double underscores]

>+  onLocationChange: function FullZoom_onLocationChange(aURI, aIsTabSwitch, aBrowser) {
>+    if (!aURI || (aIsTabSwitch && !this.siteSpecific))
When site specific preferences are off, what happens in a non-tab-switch case? All I can see is calls to _applyPrefToSetting which test for siteSpecific too.

>+      let isSaneURI = (aBrowser && aBrowser.currentURI) ?
>+        aURI.equals(aBrowser.currentURI) : false;
aURI.equals(null) is false, so no need to null-test currentURI.

>+      if (!aBrowser || isSaneURI)
You could then go on to replace the three lines with
!aBrowser || aURI.equals(aBrowser.currentURI)

>+  Services.prefs.setCharPref("toolkit.zoomManager.zoomValues",
>+                             zoomBundle.getString("values").split(",")
>+                                       .map(function(aVal) {return aVal/100;})
>+                                       .join(","));
Why not use zoomFactors instead of splitting twice?

>+  var factorOther = zoomOther.value || zoomBundle.getString("valueOther");
IIRC zoomOther.value doesn't work on the Mac, use getAttribute instead.

>+      if (item.getAttribute("value") == parseInt(ZoomManager.zoom * 100))
Should we make it so that the value attribute is the zoom as a fraction?
Comment on attachment 494578 [details] [diff] [review]
v1.1: use onLocationChange, explain pref better

>+  var zoomType =  ZoomManager.useFullZoom ? "fullZoom" : "textZoom";
Nit: double space

>+  var menuLabel = zoomBundle.getString(zoomType).replace(/%zoom%/, parseInt(ZoomManager.zoom * 100));
...
>+      if (item.getAttribute("value") == parseInt(ZoomManager.zoom * 100))
These need to round. Otherwise 90% becomes 89% (89.99999976158142+).
(In reply to comment #26)
>(From update of attachment 494578 [details] [diff] [review])
>>+  var menuLabel = zoomBundle.getString(zoomType).replace(/%zoom%/, parseInt(ZoomManager.zoom * 100));
>...
>>+      if (item.getAttribute("value") == parseInt(ZoomManager.zoom * 100))
>These need to round. Otherwise 90% becomes 89% (89.99999976158142+).
Or you could consider toFixed() or toPrecision().
(In reply to comment #25)
> There are three problems with this. The first is that you're catching changes
> for frames, not just top-level changes. Fortunately there's a convenient block
> of code that already listens for top-level changes where you could move this.
> Secondly this listens for foreground as well as tab-switch changes. The
> difference is that aRequest is null for a tab-switch change. So you could pass
> !aRequest as the aIsTabSwitch parameter and forget the all tabs listener.
> Finally you don't pass the browser to onLocationChange. I'm concerned that this
> will cause problems if you switch tabs quickly. Can you not pass in browser
> (conveniently defined earlier in onLocationChange)?

I guess using onUpdateCurrentBrowser would have solved all those. :P
Still, I'll just apply all those changes you are wishing for here and hope it works out.

> >+  onLocationChange: function FullZoom_onLocationChange(aURI, aIsTabSwitch, aBrowser) {
> >+    if (!aURI || (aIsTabSwitch && !this.siteSpecific))
> When site specific preferences are off, what happens in a non-tab-switch case?
> All I can see is calls to _applyPrefToSetting which test for siteSpecific too.

Well, it's faster to early abort if we know we can, right?

> >+  Services.prefs.setCharPref("toolkit.zoomManager.zoomValues",
> >+                             zoomBundle.getString("values").split(",")
> >+                                       .map(function(aVal) {return aVal/100;})
> >+                                       .join(","));
> Why not use zoomFactors instead of splitting twice?

To actually be using the same things as toolkit does. And yes, I know, it's somewhat of a hack, but I didn't want to go and change the values in our menu. I should file a bug though to actually use the pref correctly and not hack it in this way (I'm glad I remember this somewhat, this work is months back).

> >+      if (item.getAttribute("value") == parseInt(ZoomManager.zoom * 100))
> Should we make it so that the value attribute is the zoom as a fraction?

I played around with those things somewhat when I worked on this, and IIRC found that this would be a good step once we switch to using the toolkit.zoomManager.zoomValues pref correctly, but probably ends up in more work as long as we're using the values from the bundle as we have been up to now.
In the end, IIRC, some accesskey hell made me decide to leave things with the values defined in the bundle for now.

(In reply to comment #26)
> >+  var menuLabel = zoomBundle.getString(zoomType).replace(/%zoom%/, parseInt(ZoomManager.zoom * 100));
> ...
> >+      if (item.getAttribute("value") == parseInt(ZoomManager.zoom * 100))
> These need to round. Otherwise 90% becomes 89% (89.99999976158142+).

Hmm, yes, I guess Math.round() is best here, we really want integers, any more precision is not too useful in this case.
OK, I hope this one fixes the remaining comments.
Attachment #494578 - Attachment is obsolete: true
Attachment #495921 - Flags: review?(neil)
Attachment #494578 - Flags: review?(neil)
(In reply to comment #28)
>(In reply to comment #25)
>>Finally you don't pass the browser to onLocationChange. I'm concerned that this
>>will cause problems if you switch tabs quickly. Can you not pass in browser
>>(conveniently defined earlier in onLocationChange)?
>I guess using onUpdateCurrentBrowser would have solved all those. :P
Well, going by your previous patch, I would have still asked you to pass in the browser (assuming onUpdateCurrentBrowser knows which one it is).

>>>+  onLocationChange: function FullZoom_onLocationChange(aURI, aIsTabSwitch, aBrowser) {
>>>+    if (!aURI || (aIsTabSwitch && !this.siteSpecific))
>>When site specific preferences are off, what happens in a non-tab-switch case?
>>All I can see is calls to _applyPrefToSetting which test for siteSpecific too.
>Well, it's faster to early abort if we know we can, right?
Sure, but why abort early only in the aIsTabSwitch case?

>>>+  Services.prefs.setCharPref("toolkit.zoomManager.zoomValues",
>>>+                             zoomBundle.getString("values").split(",")
>>>+                                       .map(function(aVal) {return aVal/100;})
>>>+                                       .join(","));
>>Why not use zoomFactors instead of splitting twice?
>To actually be using the same things as toolkit does. And yes, I know, it's
>somewhat of a hack, but I didn't want to go and change the values in our menu.
That wasn't actually my point. You assign zoomFactors to be zoomBundle.getString("values").split(",") later on in the function.
(In reply to comment #28)
> Still, I'll just apply all those changes you are wishing for here and hope it
> works out.
The change looks right at first glance.
Blocks: 519120
(In reply to comment #30)
> Sure, but why abort early only in the aIsTabSwitch case?

You're right. We don't do anything in that function that would be specific to this case only here. If it would not have some potential value for keeping FF compat, we could actually kill that parameter completely...
For reference, Firefox added it for private browsing in bug 487656 and their _applyPrefToSetting is actually executed when in private browsing mode even if .siteSpecific is off. If SeaMonkey ever implements private browsing, I hope whoever works on that will not forget this case.

> That wasn't actually my point. You assign zoomFactors to be
> zoomBundle.getString("values").split(",") later on in the function.

Ah, thanks, I somehow missed that. Fixed in a new patch.
Here's the patch with those two updates.
Attachment #495921 - Attachment is obsolete: true
Attachment #496499 - Flags: review?(neil)
Attachment #495921 - Flags: review?(neil)
Attachment #496499 - Flags: review?(neil) → review+
Thanks, pushed as http://hg.mozilla.org/comm-central/rev/61c697891f6c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: Remember text zoom value on a site-specific basis (Port fix for bug 378549 to SuiteRunner) → Remember zoom value on a site-specific basis (Port fix for bug 378549 to SeaMonkey)
Target Milestone: seamonkey2.1a3 → seamonkey2.1b2
No longer depends on: 583317
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.