Created attachment 714015 [details] Screencast of broken tabopen animation The tabopen animation on my Mac has basically only been a single frame for the last couple of days and I bisected it down to bug 822266. This horribly regressed for some reason if you have browser.newtab.preload=true.
It seems like the two lines from bug 822266 shouldn't have any impact when I'm not dragging anything or dragging between same-dpi screens, right?
Created attachment 714790 [details] [diff] [review] don't invalidate layers if DPI value hasn't changed I don't know much about the nsPresContext but this seems like a safe thing to do. We shouldn't invalidate the root frame if the DPI didn't actually change.
Attachment #714790 - Flags: review?(jfkthame)
Component: Tabbed Browser → Graphics
Product: Firefox → Core
Comment on attachment 714790 [details] [diff] [review] don't invalidate layers if DPI value hasn't changed No, I'm afraid we can't do this. That is exactly how the code here used to be, but the if() was removed in bug 829963. Restoring it will regress both bug 829963 and bug 822266, according to my testing. The problem is that in some scenarios, it's possible that other code (e.g. an nsView) may have called CheckDPIChange on the same nsDeviceContext -before- the presContext gets its turn; if that has happened, then CheckDPIChange will now return false, as it is already up-to-date, but the presContext is then left with a stale appUnitsPerDevPixel value. I think the proper fix is to make AppUnitsPerDevPixelChanged check whether the value has in fact changed or not, and simply return if it is already up to date. I'll post an alternative patch shortly, after testing that it works as I expect.
Attachment #714790 - Flags: review?(jfkthame) → review-
Created attachment 714826 [details] [diff] [review] don't invalidate layers and caches if DPI value has not actually changed I believe this should fix the issue here without regressing the other related bugs mentioned above. Tryserver build in progress at https://tbpl.mozilla.org/?tree=Try&rev=3499941b32ed.
Attachment #714790 - Attachment is obsolete: true
Thank you for taking care of this, your patch works great!
Can reproduce on Linux, too.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
Hmm, this failed a mochitest on tryserver: 30466 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_pixel_lengths.html | Checking width in mozmm at 192 DPI - got 96, expected 192 So further investigation is needed. Cancelling r? for now, though any comments would still be welcome.
Created attachment 714910 [details] [diff] [review] don't call AppUnitsPerDevPixelChanged unless the value has actually changed OK, here's a version that targets -only- the resolution-changed case, and doesn't risk disturbing zoom behavior. Tryserver: https://tbpl.mozilla.org/?tree=Try&rev=60b2473796cc.
Attachment #714910 - Flags: review?(roc)
Attachment #714826 - Attachment is obsolete: true
Attachment #714910 - Flags: review?(roc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1aa97b77a6 Tim, this is now on inbound, headed for FF21... is this something that would be important to uplift to FF20 (along with bug 822266 and the various other hidpi-related fixes), or should we just let it ride the FF21 train? If you think it needs to be uplifted, please nominate accordingly.
Target Milestone: --- → mozilla21
Followup bustage fix, thanks to warnings-as-errors: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9f0e934370
(In reply to Jonathan Kew (:jfkthame) from comment #9) > Tim, this is now on inbound, headed for FF21... is this something that would > be important to uplift to FF20 I don't have a great idea of the risk involved, but the symptom described in comment 0 seems like something worth uplifting a fix for.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 714910 [details] [diff] [review] don't call AppUnitsPerDevPixelChanged unless the value has actually changed [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 822266 User impact if declined: unnecessary freezes when dragging a tab or when opening new tabs with browser.newtab.preload=true Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: None
Attachment #714910 - Flags: approval-mozilla-aurora?
status-firefox20: --- → affected
status-firefox21: --- → fixed
Attachment #714910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Transplanted to aurora, including the followup typecast-fix: https://hg.mozilla.org/releases/mozilla-aurora/rev/ecb5f4cc6501
status-firefox20: affected → fixed
Verified fixed on the latest beta, Firefox 20 beta 6. (Build ID: 20130320062118) User Agent: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Based on Comment 15. Verified Fixed on FF20. Thank Mitza for helping.
status-firefox20: fixed → verified
I confirm the fix is verified on FF 21b3 on Mac OS 10.8, Ubuntu 12.04 and Windows 7 x64. Based on this -> Verified Fixed
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
You need to log in before you can comment on or make changes to this bug.