Last Comment Bug 541779 - Make site-specific zoom check asynchronous
: Make site-specific zoom check asynchronous
Status: RESOLVED FIXED
[tsnap]
: perf
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 3.7a4
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
:
Mentors:
Depends on: 539907 555211 555224 559991 559992
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-24 01:38 PST by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2010-04-16 19:06 PDT (History)
8 users (show)
rflint: in‑testsuite-
rflint: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (12.70 KB, patch)
2010-01-24 01:38 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Patch (15.24 KB, patch)
2010-02-24 18:54 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Patch v2 (16.03 KB, patch)
2010-03-18 18:06 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
myk: review+
Details | Diff | Splinter Review

Description User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-01-24 01:38:52 PST
Created attachment 423215 [details] [diff] [review]
WIP
Comment 1 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-02-24 18:54:08 PST
Created attachment 428858 [details] [diff] [review]
Patch

Mostly test changes - I've removed the print preview tests because getting them working is non-trivial and I plan to rip out in-window previewing anyway.
Comment 2 User image Dão Gottwald [:dao] 2010-02-25 00:52:36 PST
The last time browser_bug386835.js caught a print preview regression was in bug 544018 comment 23 :/
Comment 3 User image Myk Melez [:myk] [@mykmelez] 2010-03-03 13:40:58 PST
Comment on attachment 428858 [details] [diff] [review]
Patch

Sorry about the delay reviewing this!


>+    // Avoid the cps roundtrip and apply the default/global setting.

Nit: setting -> pref!


>+    var self = this;
>+    this._cps.getPref(aURI, this.name, function(aResult) {
>+      // Check that we're still where we expect to be in case this took a while.
>+      let isSaneURI = (aBrowser && aBrowser.currentURI) ?
>+        aURI.equals(aBrowser.currentURI) : false;
>+      if (!aBrowser || isSaneURI)
>+        self._applyPrefToSetting(aResult, aBrowser);
>+      else
>+        self._applyPrefToSetting();
>+    });

If the browser's current URI is different than the one for which we got the pref, is it possible that the application of the default/global pref will race the application of the site-specific pref for the new URL?

And is it necessary to apply the default/global pref at all if the URLs differ?  Presumably onLocationChange will get called again for the new URL, and it'll apply either the site-specific pref for that URL or the default/global pref.
Comment 4 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-03-18 18:06:40 PDT
Created attachment 433472 [details] [diff] [review]
Patch v2

(In reply to comment #3)
> If the browser's current URI is different than the one for which we got the
> pref, is it possible that the application of the default/global pref will race
> the application of the site-specific pref for the new URL?
> 
> And is it necessary to apply the default/global pref at all if the URLs differ?

It can indeed race, and no, it's not necessary. I think I added that while debugging something and simply forgot to remove it.
Comment 5 User image Myk Melez [:myk] [@mykmelez] 2010-03-22 17:48:05 PDT
Comment on attachment 433472 [details] [diff] [review]
Patch v2

>diff --git a/browser/base/content/browser-fullZoom.js b/browser/base/content/browser-fullZoom.js
>-    var browser = aBrowser || gBrowser.selectedBrowser;
>+    var browser = aBrowser || (gBrowser && gBrowser.selectedBrowser);

Heh, funky, but I guess it works.

r=myk!
Comment 6 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2010-03-25 04:01:00 PDT
http://hg.mozilla.org/mozilla-central/rev/a2f7186e4379
Comment 7 User image Alice0775 White 2010-03-26 08:13:05 PDT
Zoomed page does not flicker after switching back tab.
Comment 8 User image Alice0775 White 2010-03-26 08:14:45 PDT
Oops,Sorry, I have transmitted a message by mistake.
Comment 9 User image Alice0775 White 2010-03-26 09:59:34 PDT
Is variable name isSaneURI in the patch a spelling mistake of isSameURI?

Note You need to log in before you can comment on or make changes to this bug.