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)
Tracking
(firefox20 verified)
VERIFIED
FIXED
Firefox 20
Tracking | Status | |
---|---|---|
firefox20 | --- | verified |
People
(Reporter: kats, Assigned: kats)
Details
Attachments
(1 file, 1 obsolete file)
1.77 KB,
patch
|
jwir3
:
review+
|
Details | Diff | Splinter Review |
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 1•12 years ago
|
||
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+
Assignee | ||
Comment 2•12 years ago
|
||
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.
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #683647 -
Flags: review?(sjohnson)
Comment 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
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
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e12602b7c60
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Comment 8•12 years ago
|
||
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
status-firefox20:
--- → verified
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•