Closed Bug 813616 Opened 12 years ago Closed 12 years ago

Reflow-on-zoom pref change may be missed

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(firefox20 verified)

VERIFIED FIXED
Firefox 20
Tracking Status
firefox20 --- verified

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

I noticed this while looking at some other code; it's possible for the pref change to get missed if the BrowserApp.isContentDocumentDisplayed() check returns false.
Attachment #683622 - Flags: review?(sjohnson)
Comment on attachment 683622 [details] [diff] [review]
Move the pref check up

Review of attachment 683622 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +3840,5 @@
> +    } else if (aTopic == "nsPref:changed") {
> +      if (aData == "browser.zoom.reflowOnZoom") {
> +        this.updateReflozPref();
> +      }
> +      return;

Wow, good catch. I didn't see that at all. This definitely seems wonky. Perhaps we should encapsulate all of the stuff below this into a separate function so it's obvious that this assumption is made?

r=jwir3 for this change, though.
Attachment #683622 - Flags: review?(sjohnson) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e12602b7c60

(In reply to Scott Johnson (:jwir3) from comment #1)
> Perhaps we should encapsulate all of the stuff below this into a separate
> function so it's obvious that this assumption is made?

That seems like a good idea, yeah.
(In reply to Kartikaya Gupta (:kats) from comment #2)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7e12602b7c60
> 
> (In reply to Scott Johnson (:jwir3) from comment #1)
> > Perhaps we should encapsulate all of the stuff below this into a separate
> > function so it's obvious that this assumption is made?
> 
> That seems like a good idea, yeah.

Filed as bug 813640.
Attached patch Refactor code a bit (obsolete) — Splinter Review
Attachment #683647 - Flags: review?(sjohnson)
Comment on attachment 683647 [details] [diff] [review]
Refactor code a bit

Review of attachment 683647 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, so you can make it r=jwir3, but I didn't realize you were working on it in this bug... so I filed bug 813640 to track it. I'd recommend making this patch an attachment to that bug instead.
Attachment #683647 - Flags: review?(sjohnson) → review+
Comment on attachment 683647 [details] [diff] [review]
Refactor code a bit

Obsoleting this attachment; I uploaded it to bug 813640.
Attachment #683647 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7e12602b7c60
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Pref changed is no longer missing on the latest Nightly. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-28)
Device: Galaxy Nexus
OS: Android 4.1.1
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: