Closed
Bug 841470
Opened 11 years ago
Closed 11 years ago
Missing tabopen animation since bug 822266, with newtab preloading enabled
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: ttaubert, Assigned: jfkthame)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
792.56 KB,
application/octet-stream
|
Details | |
954 bytes,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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?
Flags: needinfo?(jfkthame)
Reporter | ||
Comment 2•11 years ago
|
||
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)
Updated•11 years ago
|
Component: Tabbed Browser → Graphics
Product: Firefox → Core
Assignee | ||
Comment 3•11 years ago
|
||
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-
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 4•11 years ago
|
||
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 #714826 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jfkthame
Reporter | ||
Updated•11 years ago
|
Attachment #714790 -
Attachment is obsolete: true
Reporter | ||
Comment 5•11 years ago
|
||
Thank you for taking care of this, your patch works great!
Reporter | ||
Comment 6•11 years ago
|
||
Can reproduce on Linux, too.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #714826 -
Flags: review?(roc)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #714826 -
Attachment is obsolete: true
Attachment #714910 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
Followup bustage fix, thanks to warnings-as-errors: https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9f0e934370
Comment 11•11 years ago
|
||
(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.
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe1aa97b77a6 https://hg.mozilla.org/mozilla-central/rev/1a9f0e934370
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Reporter | ||
Comment 13•11 years ago
|
||
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?
Updated•11 years ago
|
status-firefox20:
--- → affected
status-firefox21:
--- → fixed
Updated•11 years ago
|
Attachment #714910 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 14•11 years ago
|
||
Transplanted to aurora, including the followup typecast-fix: https://hg.mozilla.org/releases/mozilla-aurora/rev/ecb5f4cc6501
Comment 15•11 years ago
|
||
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
Comment 16•11 years ago
|
||
Based on Comment 15. Verified Fixed on FF20. Thank Mitza for helping.
Comment 17•11 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•