Closed
Bug 978913
Opened 9 years ago
Closed 9 years ago
[e10s] Dragging e10s window between HiDPI and non-HiDPI displays causes text to be scaled incorrectly
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: cpeterson, Assigned: mconley)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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.
Comment 1•9 years ago
|
||
This can happen even with a non-e10s window, thanks to a bug in e10s code. See bug 974616.
See Also: → 974616
Comment 2•9 years ago
|
||
Actually bug 974616 has now been fixed (by the patches for bug 974007).
Reporter | ||
Comment 3•9 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.
Comment 5•9 years ago
|
||
I can reproduce this bug as of 31.0a1 (2014-03-19).
Reporter | ||
Updated•9 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•9 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•9 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•9 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•9 years ago
|
||
Clearing needinfo on smaug for now, based on above.
Flags: needinfo?(bugs)
Assignee | ||
Comment 11•9 years ago
|
||
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•9 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 | ||
Comment 14•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8426663 -
Attachment is obsolete: true
Attachment #8426663 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 15•9 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 16•9 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=? >+ virtual void ClearBackingScaleCache() { Nit, { goes to its own line
Attachment #8427090 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Nit fixed! Thanks smaug! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1068bf18e64
Comment 18•9 years ago
|
||
Backed out for bustage on most platforms. https://hg.mozilla.org/integration/mozilla-inbound/rev/ad797cc2147a https://tbpl.mozilla.org/php/getParsedLog.php?id=40212911&tree=Mozilla-Inbound
Assignee | ||
Comment 19•9 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•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•