[e10s] Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: mconley)

Tracking

(Blocks 2 bugs)

unspecified
mozilla32
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
I have a Retina MacBook Pro with a non-HiDPI external display. When I drag an e10s window from the external display to the MacBook Pro's Retina display, the page text becomes very tiny. If I drag an e10s window from the Retina display to the external display, the page text becomes very large.
This can happen even with a non-e10s window, thanks to a bug in e10s code.  See bug 974616.
See Also: → 974616
Actually bug 974616 has now been fixed (by the patches for bug 974007).
(Reporter)

Comment 3

5 years ago
I can still repro this bug with an e10s window in Nightly 30 build 2014-03-04. Non-e10s windows work correctly.
Are there any kind of notifications that fire when a window is dragged across monitors? Maybe we're just not passing the notification to the content process.
I can reproduce this bug as of 31.0a1 (2014-03-19).
(Reporter)

Updated

5 years ago
tracking-e10s: --- → +
(Assignee)

Updated

5 years ago
Assignee: nobody → mconley
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

5 years ago
The event that billm is referring to is windowDidChangeScreen, which is received by nsCocoaWindow, and fires mGeckoWindow->BackingScaleFactorChanged();.

BenWa helped me figure that one out, and we determined that the child process is definitely not having BackingScaleFactorChanged on it, so we might need to forward this to the child.
(Assignee)

Comment 7

5 years ago
So here's what I've found out:

When an nsCocoaWindow hears windowDidChangeScreen, it calls its own BackingScaleFactorChanged function, which in turn calls nsPresContext::UIResolutionChanged for the associated nsPresContext, which runs this function in a runnable:

void
nsPresContext::UIResolutionChangedInternal()
{
  mPendingUIResolutionChanged = false;

  mDeviceContext->CheckDPIChange();
  if (mCurAppUnitsPerDevPixel != AppUnitsPerDevPixel()) {
    AppUnitsPerDevPixelChanged();
  }

  mDocument->EnumerateSubDocuments(UIResolutionChangedSubdocumentCallback,
                                   nullptr);
}

That UIResolutionChangedSubdocumentCallback is this:

/*static*/ bool
nsPresContext::UIResolutionChangedSubdocumentCallback(nsIDocument* aDocument,
                                                      void* aData)
{
  nsIPresShell* shell = aDocument->GetShell();
  if (shell) {
    nsPresContext* pc = shell->GetPresContext();
    if (pc) {
      pc->UIResolutionChangedInternal();
    }
  }
  return true;
}

I think this is a spot (hopefully the right one) where we can call the TabParents for all of these documents, to signal to their TabChildren to call UIResolutionChanged on all of their nsIDocument's nsPresContexts.

So, the question is: is it possible to somehow go from each nsIDocument passed to this UIResolutionChangedSubdocumentCallback to their respective TabParents? If so, how?

billm, do you know?
Flags: needinfo?(wmccloskey)
I don't know anything helpful here. I don't see any easy way to find all the TabParents for a given window. The message manager keeps that sort of per-window data, but it doesn't look easy to get out what you need. Another option would be to enumerate over all TabParents and filter out the ones that are in the window that moved. That's not all that easy either though.

Maybe smaug has a better idea?
Flags: needinfo?(wmccloskey) → needinfo?(bugs)
(Assignee)

Comment 9

5 years ago
Talked to smaug about this yesterday, and what I'm going to try is to get to the XUL document when the UIResolutionChanged method is called. From there, get to the "window" object, and then try to broadcast via window.messageManager to all of the remote browsers to update their nsPresContexts as well.

I'm seeing a lot of message manager use via from JS, but fewer examples in C++, but it _seems_ like it should be possible.
(Assignee)

Comment 10

5 years ago
Clearing needinfo on smaug for now, based on above.
Flags: needinfo?(bugs)
(Assignee)

Comment 11

5 years ago
Posted patch WIP Patch 1 (obsolete) — Splinter Review
So originally, I was trying to iterate the subdocuments inside nsPresContext::UIResolutionChangedSubdocumentCallback to trace them to their respective TabParents to send the UIResolutionChanged signal to the children.

Turns out that's really not easy - in fact, I found some other code that uses a hack to cast the callback passed to the nsFrameMessageManager in its Init call back into the nsFrameLoader, from which we can get to the TabParent.

That code is here: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsCCUncollectableMarker.cpp#138

My suggestion is to use the same hack, and then to file a new bug to make it easier to go from an nsIMessageListenerManager back to a nsFrameLoader.

That's what I'm doing here with this WIP, and I also have to invalidate some caches, because TabParent and TabChild both cache the DPI and default scales it seems.
(Assignee)

Comment 12

5 years ago
Comment on attachment 8426663 [details] [diff] [review]
WIP Patch 1

Am I on the right track here?
Attachment #8426663 - Flags: feedback?(wmccloskey)
Comment on attachment 8426663 [details] [diff] [review]
WIP Patch 1

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

This seems fine to me.

::: layout/base/nsPresContext.cpp
@@ +1771,5 @@
>    if (mCurAppUnitsPerDevPixel != AppUnitsPerDevPixel()) {
>      AppUnitsPerDevPixelChanged();
>    }
>  
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {

Do we need this check? I didn't think we had a chrome window in the child process, but I could be wrong. Maybe you could just turn it into a MOZ_ASSERT.

@@ +1786,5 @@
> +        windowMM->GetChildAt(j, getter_AddRefs(childMM));
> +        if (!childMM) {
> +          continue;
> +        }
> +        nsCOMPtr<nsIMessageSender> strongTabMM = do_QueryInterface(childMM);

I think if you use strongTabMM.get() in the cast below, you won't need separate strong and weak variables.
Attachment #8426663 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #8426663 - Attachment is obsolete: true
Attachment #8426663 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 15

5 years ago
Comment on attachment 8427090 [details] [diff] [review]
Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly. r=?

Thanks for the feedback+, billm!

Addressed feedback, and cleaned it up some. Ready for a real review.
Attachment #8427090 - Flags: review?(bugs)
Comment on attachment 8427090 [details] [diff] [review]
Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly. r=?

>+  virtual void ClearBackingScaleCache() {
Nit, { goes to its own line
Attachment #8427090 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

5 years ago
Ok, found the issue - needed to bring in nsFrameLoader.h for non-desktop builds, I guess.

Here's a try push:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=590ac758bbd9

Weird to have to request so many tests and systems to run, but, well, that's platform I guess.
(Assignee)

Comment 20

5 years ago
Ok, I think it's landable now with that missing include.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a40b1e15ae8b
https://hg.mozilla.org/mozilla-central/rev/a40b1e15ae8b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Reporter)

Updated

4 years ago
Blocks: 1111957
(Reporter)

Updated

4 years ago
Blocks: 1215659
You need to log in before you can comment on or make changes to this bug.