Closed Bug 1189565 Opened 9 years ago Closed 9 years ago

[e10s] APZ seriously messes up mouse coordinates on secondary display in e10s mode on OS X with Retina Mac

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Version:
42 Branch
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
e10s ? ---
firefox42 --- unaffected
firefox43 --- fixed

People

(Reporter: smichaud, Assigned: BenWa)

References

Details

(Whiteboard: see comment #61 for workarounds)

Attachments

(3 files, 1 obsolete file)

Logging patch
5.00 KB, patch
Details | Diff | Splinter Review
Fix
1.44 KB, patch
mstange
: review+
Details | Diff | Splinter Review
patch
1.79 KB, patch
kats
: review+
Details | Diff | Splinter Review 
As of the patch for bug 1157746, when a Firefox window created on one display is moved to another, the locations effected by mouse events are seriously offset from the actual mouse cursor's location, in e10s mode.

1) Run a Firefox m-c nightly dated 2015-07-23 or later on a system with a secondary display attached.  Make sure "enable multi-process nightly" is selected.
2) Drag the Firefox window from the main display to the secondary display.
3) Notice that the effect of mouse-moves and mouse-clicks is offset up and to the left of the actual mouse cursor location.

The displacement seems to grow larger as you move the mouse cursor lower and to the right.  Only Gecko chrome and content are effected.  The mouse works correctly to hover or click the close, minimize and zoom buttons, to drag the window by its titlebar, and to resize the window by its edges or "grab region" (to the lower right).

The bug disappears when you resize the window.  It reappears when you drag it from one display to the other.

Turning HiDPI on or off makes no difference.  The bug only happens with e10s on (not with it off).

The workaround is to set layers.async-pan-zoom.enabled to false in about:config.
I'm not able to reproduce this on my OS X laptop with a secondary display attached. There must be more factors involved here.

(In reply to Steven Michaud [:smichaud] from comment #0)
> 3) Notice that the effect of mouse-moves and mouse-clicks is offset up and
> to the left of the actual mouse cursor location.
> The displacement seems to grow larger as you move the mouse cursor lower and
> to the right.

This sounds like it's off by a scaling factor of some sort with the origin anchored at the top-left of the window. Do you know if your two displays have different densities or would result in a different nsIWidget::GetDefaultScale()? You said it doesn't make a difference with HiDPI which surprises me.
Kats, what version of OS X were you testing with?  And where was your secondary display "arranged"?

I tested on OS X 10.7.5, on a Retina MacBook Pro, with a non-Retina (and larger) secondary display arranged to the left.  Later I'll try on different versions of OS X, with different "arrangements", to see if that makes a difference.

I tested with a clean profile.

To test without HiDPI mode, I right-clicked on the distro and chose Get Info : Open in Low Resolution.

Looking at the source code, it seems that nsChildView/nsCocoaWindow::GetDefaultScale() always returns 1.0 for non-HiDPI displays and 2.0 for HiDPI ones (in HiDPI mode).  With a clean profile, that is.  I doubt nsIWidget::GetDefaultScale() is involved here -- unless it's being misused somewhere in APZ code.
I tested on 10.9.5, with the secondary display "above" my primary display. The right edges of the secondary and primary displays are vertically aligned, and the secondary display is wider (so it's left edge extends beyond the left edge of the primary display).
I also see the bug on OS X 10.10.4.  On both 10.7.5 and 10.10.4, I see it (with exactly the same symptoms) regardless of where the secondary display is "arranged" -- left, right, above or below.

None of the edges are aligned -- I'll check if that makes a difference.  I'll also test on 10.9.5.
Arranging my secondary display like yours (above and with the right edges aligned) makes no difference -- I still see the bug.  I also see the bug (with exactly the same symptoms) on OS X 10.9.5..

My secondary display has an unusual horizontal to vertical ratio:  4 to 3, like in traditional CRTs (though my display isn't a CRT) -- not the "cinema" ratio that's almost universal these days (and which frankly I hate).  My Retina MacBook Pro's display, of course, has the "cinema" ratio.

My secondary display is an NEC LCD2190UXp.

Are you in an office, Kats?  If so could you look around with a 4 by 3 secondary monitor to test with?
(In reply to Steven Michaud [:smichaud] from comment #5)
> Are you in an office, Kats?  If so could you look around with a 4 by 3
> secondary monitor to test with?

I am not, unfortunately. My secondary display is 1920x1200.
> My secondary display is 1920x1200.

Mine is 1600x1200.

I'm not in an office, either.  And I have no "cinema"-style external monitor to test with.
I wonder if the bug here is that our APZ code is using the "wrong" display's coordinate system in e10s mode.
Attached patch Logging patch — — Splinter Review 
If you have some time, could you apply this patch, reproduce the problem and save the console output? It would help if you moved the window to a known coordinate position like (0,0) on your secondary display. It would also be good to use a page like http://people.mozilla.org/~kgupta/grid.html and try clicking on one of the links after the window-move so that we know the approximate coordinates of the click in content CSS pixels at least.
The logs from your patch are *much* too noisy to be useful.  How about limiting them to key presses?  And each log "entry" should have a timestamp, to make it possible to edit out some of the noise.

Also you probably want to log the "native" mouse coordinates (those from the native key-down event) along with the other information.

As you'd expect, I can't click directly on any of your test page's links.  But I did manage to find the correct offsets (to click on) for the first two in the top row.
> How about limiting them to key presses?

Oops, that makes no sense.  How about limiting them to mouse clicks?
In a case like this where we have no idea what's going wrong I find it's better to have too much logging and wade through it than to target the logging and hope we guessed the right spots to log. As long as you only click on the test page twice (once while it's on the primary display, and once after moving it to the secondary display) I shouldn't have any trouble finding the relevant parts in the log spew.
(Also if you *really* want more targetted logging I can write a patch that does that but it'll have to wait until next week at least. Using the existing logging stuff we have is cheap and easy.)
Whatever I do is going to have to wait until next week, anyway -- I'm too busy with other stuff.  By the way, I *can't* do exactly as you say (click just twice on the test page).  I also need to click on it (on the titlebar) to drag it from one screen to the other.

For now this bug doesn't seem to be urgent.  I'm the only one we know is effected, and I have an easy workaround (set layers.async-pan-zoom.enabled to false).
I saw this today right after updating Nightly.  Browser was already on second screen when it restarted for the update.  But cursor focus was strangely offset on the about:newtab page.  Opened a new tab and it was fine.
Tracy, please tell us as much as you can about your secondary monitor, and the computer you're connecting it to.
The External is an HP 25xi connected to laptop via HDMI. It's a 10.9.5 MacBook Pro with retina display.

Here's the Graphics/Display data. 

NVIDIA GeForce GT 750M:
  Chipset Model:	NVIDIA GeForce GT 750M
  Type:	GPU
  Bus:	PCIe
  PCIe Lane Width:	x8
  VRAM (Total):	2048 MB
  Vendor:	NVIDIA (0x10de)
  Device ID:	0x0fe9
  Revision ID:	0x00a2
  ROM Revision:	3776
  gMux Version:	4.0.8 [3.2.8]
  Displays:
Color LCD:
  Display Type:	Retina LCD
  Resolution:	2880 x 1800
  Retina:	Yes
  Pixel Depth:	32-Bit Color (ARGB8888)
  Mirror:	Off
  Online:	Yes
  Built-In:	Yes
HP 25xi:
  Resolution:	1920 x 1080 @ 60Hz (1080p)
  Pixel Depth:	32-Bit Color (ARGB8888)
  Display Serial Number:	3CM31103BM  
  Main Display:	Yes
  Mirror:	Off
  Online:	Yes
  Rotation:	Supported
  Television:	Yes
Thanks!  Does it make a difference if you use some other type of connection than HDMI?

Also tell us if turning off e10s or APZ makes the problem go away (as it does for me).  To turn off APZ, set layers.async-pan-zoom.enabled to false in about:config.
Finally, also let us know if your problem starts with the 2015-07-23 mozilla-central nightly (as it does for me).  For that you'll need to test with a clean profile.
Kats, do you have an HDMI connector to play with?  And what kind of connection do you currently have to your external monitor?

For the record, my external monitor is connected via a Mini DisplayPort (and an adapter) to a DVI connector.
Flags: needinfo?(twalker)
Flags: needinfo?(bugmail.mozilla)
I'm connected via Thunderbolt on my laptop to a Displayport on the external monitor. I don't have an HDMI connector lying around unfortunately.
Flags: needinfo?(bugmail.mozilla)
Yes, turning off APZ made the issue go away immediately.  I'll have to dig up cables and adapters to check other connection to the monitor.
(Following up comment #20)

> For the record, my external monitor is connected via a Mini
> DisplayPort (and an adapter) to a DVI connector.

Actually this is partly wrong.  What I called a Mini DisplayPort is
actually a Thunderbolt port.

So my external monitor is connected via a Thunderbolt port (and an
adapter) to a DVI connector.
I just tried connecting two other, older (non-Retina) MacBook Pros via the same adapter and DVI connector -- one with a Thunderbolt port, one with a genuine Mini DisplayPort.  I *didn't* see the bug with either one.

So I now expect we'll only see this bug with Retina MacBook Pros -- even though the bug still happens if you turn off HiDPI support (by right-clicking on the distro and choosing Get Info : Open in Low Resolution).

However the kind of connector *might* still make a difference with Retina MacBook Pros.
Kats, you haven't yet told us whether your MacBook Pro is a Retina MacBook Pro.
Flags: needinfo?(bugmail.mozilla)
I have a non-retina macbook air.
Flags: needinfo?(bugmail.mozilla)
So, getting perhaps a little ahead of the evidence, it looks like this bug happens with *every* Retina MacBook Pro, no matter how it's connected (or otherwise set up).  That makes this a much more serious (and common) bug than I first thought.

I'll try to track it down myself.  Though I'm unfamiliar with APZ code, it sounds like it's missing notifications (of display configuration changes) that do get sent to HiDPI code.  That should make it easier to spot the problem.
Summary: [e10s] APZ seriously messes up mouse coordinates on secondary display in e10s mode on OS X → [e10s] APZ seriously messes up mouse coordinates on secondary display in e10s mode on OS X with Retina Mac
[Tracking Requested - why for this release]:

The bug is much more common than I first thought, and therefore more serious.
tracking-firefox42: --- → ?
Note that APZ is enabled #ifdef nightly, so it's not really on the 42 train and it may not make sense to track it as such.
Fair enough.  We should still "track e10s", though.
tracking-firefox42: ? → ---
The plot thickens, but I haven't yet managed to figure this bug out.

By trial and error I've found that the following patch stops this bug from happening:

diff -U 10 -r src.old/layout/base/nsDisplayList.cpp src/layout/base/nsDisplayList.cpp
--- src.old/layout/base/nsDisplayList.cpp	2015-07-31 16:10:17.000000000 -0500
+++ src/layout/base/nsDisplayList.cpp	2015-08-06 18:46:55.000000000 -0500
@@ -1299,22 +1299,22 @@
   }
   return result;
 }
 
 bool
 nsDisplayListBuilder::IsBuildingLayerEventRegions()
 {
   if (mMode == PAINTING) {
     // Note: this is the only place that gets to query LayoutEventRegionsEnabled
     // 'directly' - other code should call this function.
-    return gfxPrefs::LayoutEventRegionsEnabledDoNotUseDirectly() ||
-           mAsyncPanZoomEnabled;
+    return gfxPrefs::LayoutEventRegionsEnabledDoNotUseDirectly() /*||
+           mAsyncPanZoomEnabled*/;
   }
   return false;
 }
 
 void nsDisplayListSet::MoveTo(const nsDisplayListSet& aDestination) const
 {
   aDestination.BorderBackground()->AppendToTop(BorderBackground());
   aDestination.BlockBorderBackgrounds()->AppendToTop(BlockBorderBackgrounds());
   aDestination.Floats()->AppendToTop(Floats());
   aDestination.Content()->AppendToTop(Content());

And without that change, so does the following:

diff -U 10 -r src.old/layout/generic/nsFrame.cpp src/layout/generic/nsFrame.cpp
--- src.old/layout/generic/nsFrame.cpp	2015-07-27 10:56:09.000000000 -0500
+++ src/layout/generic/nsFrame.cpp	2015-08-06 18:49:33.000000000 -0500
@@ -2039,26 +2039,28 @@
     }
 
     MarkAbsoluteFramesForDisplayList(aBuilder, dirtyRect);
 
     // Preserve3DChildren() also guarantees that applyAbsPosClipping and usingSVGEffects are false
     // We only modify the preserve-3d rect if we are the top of a preserve-3d heirarchy
     if (Preserves3DChildren()) {
       aBuilder->MarkPreserve3DFramesForDisplayList(this, aDirtyRect);
     }
 
+#if (0)
     if (aBuilder->IsBuildingLayerEventRegions()) {
       nsDisplayLayerEventRegions* eventRegions =
         new (aBuilder) nsDisplayLayerEventRegions(aBuilder, this);
       aBuilder->SetLayerEventRegions(eventRegions);
       set.BorderBackground()->AppendNewToTop(eventRegions);
     }
+#endif
     aBuilder->AdjustWindowDraggingRegion(this);
     BuildDisplayList(aBuilder, dirtyRect, set);
   }
 
   if (aBuilder->IsBackgroundOnly()) {
     set.BlockBorderBackgrounds()->DeleteAll();
     set.Floats()->DeleteAll();
     set.Content()->DeleteAll();
     set.PositionedDescendants()->DeleteAll();
     set.Outlines()->DeleteAll();
(Following up comment #31)

Actually, what I said in comment #31 isn't quite right.  The patches I listed there don't completely get rid of this bug.  You also have to add the following patch:

diff -U 10 -r src.old/layout/ipc/RenderFrameParent.cpp src/layout/ipc/RenderFrameParent.cpp
--- src.old/layout/ipc/RenderFrameParent.cpp	2015-07-27 10:56:09.000000000 -0500
+++ src/layout/ipc/RenderFrameParent.cpp	2015-08-07 18:28:24.000000000 -0500
@@ -313,24 +313,26 @@
 
   if (XRE_IsParentProcess()) {
     // Our remote frame will push layers updates to the compositor,
     // and we'll keep an indirect reference to that tree.
     *aId = mLayersId = CompositorParent::AllocateLayerTreeId();
     if (lm && lm->GetBackendType() == LayersBackend::LAYERS_CLIENT) {
       ClientLayerManager *clientManager =
         static_cast<ClientLayerManager*>(lm.get());
       clientManager->GetRemoteRenderer()->SendNotifyChildCreated(mLayersId);
     }
+#if (0)
     if (mAsyncPanZoomEnabled) {
       mContentController = new RemoteContentController(this);
       CompositorParent::SetControllerForLayerTree(mLayersId, mContentController);
     }
+#endif
   } else if (XRE_IsContentProcess()) {
     ContentChild::GetSingleton()->SendAllocateLayerTreeId(aId);
     mLayersId = *aId;
     CompositorChild::Get()->SendNotifyChildCreated(mLayersId);
   }
   *aSuccess = true;
 }
 
 APZCTreeManager*
 RenderFrameParent::GetApzcTreeManager()

I still haven't figured out this bug.  And for the moment I'm out of time -- I'm on vacation all of next week.
Actually, the following patch plus the one in comment #33 also "fixes" this bug:

diff -U 10 -r src.old/layout/generic/nsFrame.cpp src/layout/generic/nsFrame.cpp
--- src.old/layout/generic/nsFrame.cpp	2015-07-27 10:56:09.000000000 -0500
+++ src/layout/generic/nsFrame.cpp	2015-08-07 18:38:37.000000000 -0500
@@ -2039,21 +2039,24 @@
     }
 
     MarkAbsoluteFramesForDisplayList(aBuilder, dirtyRect);
 
     // Preserve3DChildren() also guarantees that applyAbsPosClipping and usingSVGEffects are false
     // We only modify the preserve-3d rect if we are the top of a preserve-3d heirarchy
     if (Preserves3DChildren()) {
       aBuilder->MarkPreserve3DFramesForDisplayList(this, aDirtyRect);
     }
 
-    if (aBuilder->IsBuildingLayerEventRegions()) {
+    ViewportFrame *viewportFrame = do_QueryFrame(this);
+    nsBoxFrame *boxFrame = do_QueryFrame(this);
+    bool isContentProcess = XRE_IsContentProcess();
+    if (!viewportFrame && !boxFrame && aBuilder->IsBuildingLayerEventRegions()) {
       nsDisplayLayerEventRegions* eventRegions =
         new (aBuilder) nsDisplayLayerEventRegions(aBuilder, this);
       aBuilder->SetLayerEventRegions(eventRegions);
       set.BorderBackground()->AppendNewToTop(eventRegions);
     }
     aBuilder->AdjustWindowDraggingRegion(this);
     BuildDisplayList(aBuilder, dirtyRect, set);
   }
 
   if (aBuilder->IsBackgroundOnly()) {
I'm getting closer to figuring this out.  I've now found that the following patch "fixes" this bug all by itself:

diff -U 10 -r src.old/gfx/layers/apz/src/APZCTreeManager.cpp src/gfx/layers/apz/src/APZCTreeManager.cpp
--- src.old/gfx/layers/apz/src/APZCTreeManager.cpp	2015-07-27 10:56:08.000000000 -0500
+++ src/gfx/layers/apz/src/APZCTreeManager.cpp	2015-08-18 18:05:55.000000000 -0500
@@ -403,20 +410,21 @@
         apzc = node->GetApzc();
         break;
       }
     }
 
     // The APZC we get off the layer may have been destroyed previously if the
     // layer was inactive or omitted from the layer tree for whatever reason
     // from a layers update. If it later comes back it will have a reference to
     // a destroyed APZC and so we need to throw that out and make a new one.
     bool newApzc = (apzc == nullptr || apzc->IsDestroyed());
+    newApzc = true;
     if (newApzc) {
       apzc = MakeAPZCInstance(aLayersId, state->mController);
       apzc->SetCompositorParent(aState.mCompositor);
       if (state->mCrossProcessParent != nullptr) {
         apzc->ShareFrameMetricsAcrossProcesses();
       }
       MOZ_ASSERT(node == nullptr);
       node = new HitTestingTreeNode(apzc, true, aLayersId);
     } else {
       // If we are re-using a node for this layer clear the tree pointers
(In reply to Steven Michaud [:smichaud] from comment #28)
> [Tracking Requested - why for this release]:
> 
> The bug is much more common than I first thought, and therefore more serious.

Yes, I've experienced this more often than just after update. It has been happening on wake from sleep too.

The simple work around is to resize the affected Window.  Fortunately, the Window edge grab is not offset.

Steven, sorry I haven't got back to check alternate cabling methods.  Do you still need that from me?  Anything else I can do to help here?
Flags: needinfo?(twalker)
> Anything else I can do to help here?

I should shortly have a fix for this bug.  You can test it :-)
And don't bother with the cabling -- I no longer think it's relevant.
Attached patch Fix — — Splinter Review 
Even after the big clue in comment #35, it still took me a while to finish figuring this bug out.  Finally, though, I noticed that nsView::WindowResized() was never getting called in the main process when dragging a window from an HiDPI screen to a non-HiDPI screen (or vice versa) -- only (somehow) in the content process (in e10s mode).  So when e10s is on, there's a mismatch between the window size information in the main/chrome process and the content process.  Hence this bug.

Once you think of it this way, the fix is obvious.  Moving a window between an HiDPI screen and a non-HiDPI screen causes its size to change in device pixels (though not in display pixels).  So all along we should have been calling ReportSizeEvent() from nsChildView::BackingScaleFactorChanged() and nsCocoaWindow::BackingScaleFactorChanged().  But somehow this didn't cause any (clearly visible) trouble until APZ got turned on.

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9bfca45fbc5
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8650144 - Flags: review?(mstange)
(Following up comment #0)

> Turning HiDPI on or off makes no difference.

I still can't explain why this would happen.  But in any case it's no longer true.  Testing with today's mozilla-central nightly (still on OS X 10.7.5), this bug doesn't happen when it's set to "open in low resolution".
Nice, thanks for tracking this down! The fix looks sane to me. (Adjusting flags since APZ is only enabled #ifdef nightly, and so right now this bug should be affecting 43 and not 42).
status-firefox42: affected → unaffected
status-firefox43: --- → affected
Component: Panning and Zooming → Widget: Cocoa
(In reply to Steven Michaud [:smichaud] from comment #42)
> Tracy, please test with this build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> e9bfca45fbc5/try-macosx64/firefox-43.0a1.en-US.mac.dmg

The cases I could reproduce with latest Nightly are not reproducible with the try build.
Comment on attachment 8650144 [details] [diff] [review]
Fix

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

Looks good!
Attachment #8650144 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/2e731191c692
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
I think landing this has regressed other position and sizing issues.  With about:home loaded, if I move the window from external monitor to the retina display.  I get a home page about 1/3rd expected size plus random images flickering in the rest of the window content area.  These images can be anything from what is in a browser on another desktop to screen saver images.  (maybe cached in the video driver?)

Also, moving browser window back and forth between displays now gets a much slower resizing of content before settling on final correct size.  The double resizing was happening prior to this landing, but it was much quicker than now.
Tracy, do you have hardware acceleration off?  Or any other non-default settings?

And what version of OS X are you testing on?

I didn't see any such problems testing on OS X 10.7.5.
Also, of course, please test with a clean profile.
And finally, please look for a regression range for your problems.  They may not have started when my patch landed.
(Following up comment #50)

Also test if you see these problems with my tryserver build from comment #42.
I had a random Nightly from the 18th available. It displayed the flickering of cached images when browser moved to retina display.  So I'll NI myself to investigate each of the things I mentioned above and file individual bugs on them.

Steven, how do I check on hardware acceleration settings?
Flags: needinfo?(twalker)
Preferences : Advanced : General : Use hardware acceleration when available

> I had a random Nightly from the 18th available.

This bug's patch landed on the trunk only as of today's mozilla-central nightly.  See comment #46.
(In reply to Steven Michaud [:smichaud] from comment #53)
> Preferences : Advanced : General : Use hardware acceleration when available
> 
> > I had a random Nightly from the 18th available.
> 
> This bug's patch landed on the trunk only as of today's mozilla-central
> nightly.  See comment #46.

Yes, that's why I'll investigate the issues separately.  As at least the odd flickering one is not caused by your check-in here.
While investigating bug 1194462, I've run into the sizing issues that Tracy is talking about in comment 47, using the latest Firefox 43 Nightly. Mozregression pointed to this bug as the cause. I've filed bug 1201053 for these issues.
I've asked KWierso (the current sheriff) to back this out on m-c.

After playing around for a couple of hours, I still haven't found any leads on bug 1201053.
I'm probably not going to be able to fix this bug before I retire at the end of this month.
Assignee: smichaud → nobody
Flags: needinfo?(twalker)
Component: Widget: Cocoa → Panning and Zooming
To repeat what I said at bug 1201053 comment #5:

It's pretty clear my patch for this bug was correct, at least in a general way -- we do need to send Gecko a resize event when the backing scale factor changes.  But it's also clear we can't do that without making other changes -- possibly a lot of them, very likely in cross-platform code.
For long term workaround, per comment #14, (set layers.async-pan-zoom.enabled to false). 

Short term workaround is to resize the affected window.
Whiteboard: see comment #61 for workarounds
Since we now have a few duplicates of this bug, I think at least a few people are affected. 

Tracking this for 43 since it's a regression.  We should probably find another owner for this bug. 

Steven, I didn't realize you're retiring! We'll miss you (especially the mac users)  :)
status-firefox43: fixed → affected
tracking-firefox43: --- → +
So since resizing manually works I decided to try and see if calling Resize() works. I add +1 to make sure we never ignore this resize call.

This fixes all the issues that I have with the exception of one garbage frame, which is probably in unrelated code.

Now to fully understand why this works and work backwards to a clean fix.

If anyone can weight-in, please do.
Attachment #8660941 - Attachment is patch: true
Assignee: nobody → bgirard
Comment on attachment 8660941 [details] [diff] [review]
Hacky working patch (now why does it work?)

Does this bring back bug 1201053?
No, adjusting mBounds.width/height was required to fix this.
Alright it turns out that we also have to fix this form APZ:

http://hg.mozilla.org/mozilla-central/annotate/5d61714cf5c3/gfx/layers/apz/src/AsyncPanZoomController.cpp#l2847

Basically what happens is when change the content scale but APZ doesn't require another paint. I think this code is trying to properly adjust an event when there's a resolution change in-flight.

I can fix this either by resizing, or using APZ such that it dispatches a new FM/paint request.

What should happen here to trigger APZ to update this FM/paint request to the new zoom without having the user interaction trigger it.
Flags: needinfo?(botond)
Attached patch patch — — Splinter Review 
Asking kats to review this since botond help me write this patch. Removing needinfo since we talked in person.

This patch here is sufficient to fully fix this bug AFAIK. However there's some useful bits from smichaud's patch + my additions that help fix painting issue when switching content scale. I'll move the patch and continue working on that part in a different bug since it's not related to this bug and I don't want to conflate them.
Attachment #8660941 - Attachment is obsolete: true
Flags: needinfo?(botond)
Attachment #8661928 - Flags: review?(bugmail.mozilla)
e10s and APZ are currently tied together. I'd like to track e10s instead of 43 just to make sure this doesn't slide through the cracks. The fix is pretty much ready anyways.
tracking-e10s: - → ?
tracking-firefox43: + → ---
Comment on attachment 8661928 [details] [diff] [review]
patch

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

LGTM, thanks!

Just for the record on IRC we discussed that another potential fix may be to call mLastDispatchedPaintMetrics.SetDevPixelsPerCSSPixel(aLayerMetrics.GetDevPixelsPerCSSPixel()); inside the block at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=5d4380c90c05#2957. This patch is probably a better fix though.
Attachment #8661928 - Flags: review?(bugmail.mozilla) → review+
I filed bug 1205372 to follow up with smichaud patch which addresses a different worthwhile problem. I cleared the CC' list when cloning because the issue is fairly different.

I imagine it's likely a dupe but I couldn't find a similar bug.
See Also: → 1205372
https://hg.mozilla.org/mozilla-central/rev/c83c2741197a
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: