Missing tabopen animation since bug 822266, with newtab preloading enabled

VERIFIED FIXED in Firefox 20

Status

()

Core
Graphics
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: jfkthame)

Tracking

({regression})

Trunk
mozilla21
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox20 verified, firefox21 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 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

5 years ago
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
(Assignee)

Comment 3

5 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

5 years ago
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 #714826 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Assignee: nobody → jfkthame
(Reporter)

Updated

5 years ago
Attachment #714790 - Attachment is obsolete: true
(Reporter)

Comment 5

5 years ago
Thank you for taking care of this, your patch works great!
(Reporter)

Comment 6

5 years ago
Can reproduce on Linux, too.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 7

5 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

5 years ago
Attachment #714826 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #714826 - Attachment is obsolete: true
(Assignee)

Comment 9

5 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

5 years ago
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
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Reporter)

Comment 13

5 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?
status-firefox20: --- → affected
status-firefox21: --- → fixed
Attachment #714910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 14

5 years ago
Transplanted to aurora, including the followup typecast-fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ecb5f4cc6501
status-firefox20: affected → fixed

Comment 15

5 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
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.