Closed Bug 841470 Opened 7 years ago Closed 7 years ago

Missing tabopen animation since bug 822266, with newtab preloading enabled

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox20 --- verified
firefox21 --- verified

People

(Reporter: ttaubert, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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?
Flags: needinfo?(jfkthame)
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-
Flags: needinfo?(jfkthame)
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: nobody → jfkthame
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.
Attachment #714826 - Flags: review?(roc)
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
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.
https://hg.mozilla.org/mozilla-central/rev/fe1aa97b77a6
https://hg.mozilla.org/mozilla-central/rev/1a9f0e934370
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
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?
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
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.
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.