Closed Bug 974616 Opened 7 years ago Closed 7 years ago

Window content doesn't rescale properly moving between Retina and non-Retina display with hardware acceleration off

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

This bug is spun off from bug 962528 comment #10 and following, because it's a separate and unrelated bug.

Starting with the 2014-02-12-03-02-01-mozilla-central nightly, if "use hardware acceleration when available" is unchecked, moving a Firefox window between a Retina and a non-Retina display causes it to rescale incorrectly.  Instead of changing to the correct display mode, the contents actually changes size (inside a same-sized window).  So the content doubles in height and width moving from a non-Retina to a Retina display, and halves in height and width moving from a Retina to a non-Retina display.

Unlike with bug 962528, this effect is "permanent" -- it doesn't get corrected until you explicitly force the window to redraw (for example by resizing it).

This bug was triggered by the following patch:

https://hg.mozilla.org/mozilla-central/rev/08b4b4b4aef3

user:        Bill McCloskey <wmccloskey@mozilla.com>
date:        Tue Feb 11 09:01:08 2014 -0800
summary:     Bug 960783 - Support "new out-of-process window" menu item in nightly (r=felipe,bsmedberg,trevor,ted)
Attachment #8378546 - Attachment is obsolete: true
The bug doesn't happen if you set both browser.tabs.remote and browser.tabs.remote.autostart to false.  So, though the bug can happen with what's supposedly a non-e10s window, I strongly suspect e10s code is involved.

I'll try to find exactly which code.
Benoit Girard tells me that in what I've called an e10s window (one where each tab's content is displayed by a content process), the tab's "title" is underlined for each tab that actually has content.

Normally a tab's title isn't underlined.
Summary: Window content doesn't rescale properly moving between Retina and non-Retina display → Window content doesn't rescale properly moving between Retina and non-Retina display with hardware acceleration off
Markus, do you have any idea what's going on here?
Attached patch FixSplinter Review
I've actually found a fix for this.  Turns out we were mistakenly using the Basic compositor for a non-e10s window.

Bill, I've asked you to review because you're the author of the patches for bug 960783.  If you'd prefer to pass the review to someone else, please do so.

I've started an all-platform tryserver build.  The results should be available here by tomorrow morning:

https://tbpl.mozilla.org/?tree=Try&rev=c2838d8c16e3
Assignee: nobody → smichaud
Attachment #8378664 - Flags: review?(wmccloskey)
> https://tbpl.mozilla.org/?tree=Try&rev=c2838d8c16e3

There were no non-spurious test failures, except possibly for some of the Linux Opt Ripc tests.  I ran that set of tests twice and got identical failures in some reftests.  Since my patch can change which compositor gets used, these failures might be related to my patch.

It's possible I'll need to ifdef my patch to only effect certain platforms, or only OS X.

Here's a Mac tryserver build to test with:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-c2838d8c16e3/try-macosx64/firefox-30.0a1.en-US.mac.dmg
Comment on attachment 8378664 [details] [diff] [review]
Fix

Review of attachment 8378664 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Steve. Unfortunately, this break electrolysis on platforms without accelerated layers. I've posted an alternate patch in bug 974007 that should fix the problem.
Attachment #8378664 - Flags: review?(wmccloskey)
> this break electrolysis on platforms without accelerated layers

I think you misunderstood my patch.  As best I can tell, it only changes our behavior for non-e10s windows, on any platform.

But I'll try your patches at bug 974007.
(In reply to Steven Michaud from comment #10)
> > this break electrolysis on platforms without accelerated layers
> 
> I think you misunderstood my patch.  As best I can tell, it only changes our
> behavior for non-e10s windows, on any platform.
> 
> But I'll try your patches at bug 974007.

Let's say we open a new window in an e10s browser. The window widget lives in the parent process, and it needs to decide whether to enable OMTC. Your patch makes it so that we skip over the basic compositor in that case. If there's no accelerated compositor, then we won't get OMTC at all for that window. Later on, the child process will try to create its puppet widget and it will try to use the basic compositor. But by that point, OMTC has already been disabled for the window and it's too late.
> The window widget lives in the parent process

I didn't know this.  From glancing through the code I assumed it lived in the child process.

I stand corrected.  Thanks for the info.

How do you tell if a given window widget is for an e10s window?  In other words, how can you tell if a given window widget will have puppet widget children (in the e10s child process)?
(In reply to Steven Michaud from comment #12)
> How do you tell if a given window widget is for an e10s window?  In other
> words, how can you tell if a given window widget will have puppet widget
> children (in the e10s child process)?

The second patch in bug 974007 adds a flag to nsBaseWidget to do that. It's based off the nsIWebBrowserChrome::CHROME_REMOTE_WINDOW flag on the chrome window.
See Also: → 978913
Steve, can you please confirm that this is fixed now?
I tested today's mozilla-central nightly (on OS X 10.7.5), and yes, this bug is fixed there (presumably by the patches for bug 974007).
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Works for me on OS X 10.8.5 too - the flickering from bug 962528 is pretty bad, but the content ends up the correct size.
Depends on: 974007
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.