841470 - Missing tabopen animation since bug 822266, with newtab preloading enabled

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Thank you for taking care of this, your patch works great!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Can reproduce on Linux, too.

Comment 7

[Jonathan Kew [:jfkthame]]

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.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 9

[Jonathan Kew [:jfkthame]]

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.

Comment 10

[Jonathan Kew [:jfkthame]]

Followup bustage fix, thanks to warnings-as-errors:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9f0e934370

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/fe1aa97b77a6
https://hg.mozilla.org/mozilla-central/rev/1a9f0e934370

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 14

[Jonathan Kew [:jfkthame]]

Transplanted to aurora, including the followup typecast-fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ecb5f4cc6501

Comment 15

[Mitza]

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

[Mihai Morar, (:MihaiMorar)]

Based on Comment 15. Verified Fixed on FF20.
Thank Mitza for helping.

Comment 17

[Mihai Morar, (:MihaiMorar)]

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