Botond Ballo [:botond] Assignee
	Description • 10 years ago

APZCs currently maintain a "non-transient async transform", whose purpose is to cancel out the post-scale induced by the pres shell resolution, so that increasing the pres shell resolution actually results in things on the screen getting larger (increasing the pres shell resolution is how APZC does pinch-zoom).

In the current design, the non-transient async transform is maintained by the APZC which is being zoomed, and AsyncCompositionManager applies the transform to the corresponding layer(s). 

Unfortunately, this constrains Layout to put the post-scale on the same layer as the APZC, which prevents it from doing some things we want, such as containerless scrolling for root scroll frames.

One thing we could do to resolve this is to dis-associate the non-transient async transform from APZCs, and associate them (using a different API) with a Layer instead. Layout could then make sure they are associated with the same layer as the {pres-shell-resolution}-induced-{post-scale}.

Even better, however, would be to get Layout to not induce this post-scale to begin with, and get rid of the non-transient async transform altogether.

	Botond Ballo [:botond] Assignee
	Comment 1 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #0)
> Even better, however, would be to get Layout to not induce this post-scale
> to begin with, and get rid of the non-transient async transform altogether.

I have a patch in bug 1076253 to add a new API for setting the pres-shell resolution without canceling it out with a post-scale. 

I will now try getting APZ to use that, and get rid of the non-transient async transform, in this bug.

	Botond Ballo [:botond] Assignee
	Comment 2 • 10 years ago

Here's a WIP patch that removes the nontransient async transform. 

I didn't notice any regressions during local testing. (I did notice bug 1088984 while testing the scroll thumb behaviour, but that's unrelated.) I'll wait for some Try results before asking for review: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3e9290eb9afb

	Botond Ballo [:botond] Assignee
	Comment 3 • 10 years ago

Oops, I had Kats' patches from bug 1083395 applied but not his bustage fix. New Try push:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b4304d636406

	Botond Ballo [:botond] Assignee
	Comment 4 • 10 years ago

The Try push looks OK apart from some gtest failures which I've fixed in this updated patch. Flagging for review.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 10 years ago

Comment on attachment 8512123 [details] [diff] [review]
Begone, nontransient transform!

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

Looks fine for B2G but I suspect there's some stuff missing for Fennec and will cause breakage. Dropping flag until you can confirm there's no breakage (or update the patch to fix said breakage).

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +742,5 @@
> +    //  - First, the content's resolution applies to the scrollbar as well.
> +    //    Since we don't actually want the scroll thumb's size to vary with
> +    //    the zoom (other than its length reflecting the fraction of the
> +    //    scrollable length that's in view, which is taken care of above),
> +    //    we apply a transform to cancel out this resolution.

Just to restate this in terms I'll find easier to understand if I ever come back to this: this "resolution-cancelling transform" is basically the post-scale that used to go on the root, but pushed down to the scrollbar layer (because the post-scale no longer gets set on the root). Because of the way the layer tree is structured, removing the post-scale and the nontransient async transform will have a net effect of 0 on both the content and the scrollbar. However for the scrollbar layer we have this extra asyncUntransform step which has no counter-balance, and so we need to stick in the resolutionCancellingTransform to again achieve a net effect of 0.

@@ +923,2 @@
>    ParentLayerPoint translation = userScroll - geckoScroll;
> +  Matrix4x4 treeTransform = ViewTransform(asyncZoom, -translation);

Did you check if this change does the expected thing in Fennec? I would expect that you need to change the calls to cwu.setResolution in mobile/android/chrome/content/browser.js to setResolutionAndScaleTo for this to work.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 10 years ago

To be clear that last hunk in comment 5 is for the change in TransformScrollableLayer which really only gets exercised in non-APZC codepaths.

	Botond Ballo [:botond] Assignee
	Comment 7 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> ::: gfx/layers/composite/AsyncCompositionManager.cpp
> @@ +742,5 @@
> > +    //  - First, the content's resolution applies to the scrollbar as well.
> > +    //    Since we don't actually want the scroll thumb's size to vary with
> > +    //    the zoom (other than its length reflecting the fraction of the
> > +    //    scrollable length that's in view, which is taken care of above),
> > +    //    we apply a transform to cancel out this resolution.
> 
> Just to restate this in terms I'll find easier to understand if I ever come
> back to this: this "resolution-cancelling transform" is basically the
> post-scale that used to go on the root, but pushed down to the scrollbar
> layer (because the post-scale no longer gets set on the root). Because of
> the way the layer tree is structured, removing the post-scale and the
> nontransient async transform will have a net effect of 0 on both the content
> and the scrollbar. However for the scrollbar layer we have this extra
> asyncUntransform step which has no counter-balance, and so we need to stick
> in the resolutionCancellingTransform to again achieve a net effect of 0.

Exactly.

	Botond Ballo [:botond] Assignee
	Comment 8 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> @@ +923,2 @@
> >    ParentLayerPoint translation = userScroll - geckoScroll;
> > +  Matrix4x4 treeTransform = ViewTransform(asyncZoom, -translation);
> 
> Did you check if this change does the expected thing in Fennec? I would
> expect that you need to change the calls to cwu.setResolution in
> mobile/android/chrome/content/browser.js to setResolutionAndScaleTo for this
> to work.

Good catch! The original patch did indeed break rendering on Fennec in obvious ways, and the adjustment you suggested seems to fix the problem. Updated patch.

	Botond Ballo [:botond] Assignee
	Comment 9 • 10 years ago
failures try

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f4aa65eaa5e1

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 10 years ago

Comment on attachment 8527100 [details] [diff] [review]
Begone, nontransient transform!

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

Minusing based on the two issues i saw on the Fennec try build locally. One was that when going back the page was rendered blurry, I think this can be fixed by changing ScrollFrameHelper::RestoreState to use the new SetResolutionAndScaleTo API.

The other issue was that the tab thumbnails weren't drawing properly. Not sure what the root cause of that is but AndroidBridge::CaptureThumbnail is where that happens so it would be the place to start looking.

	Botond Ballo [:botond] Assignee
	Comment 11 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> One was that when going back the page was rendered blurry

Does this always happen, or for particular pages? I browsed around a bit but was unable to reproduce the problem.

> The other issue was that the tab thumbnails weren't drawing properly.

Couldn't repro this either. Could you give some more details on what you mean by "not drawing properly"?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #11)
> Does this always happen, or for particular pages? I browsed around a bit but
> was unable to reproduce the problem.

Specific str i saw this with: load bug 1103329, click on the link to the oracle page there, scroll around for a bit, and then go back to the bug page. At this point the bug page renders at the wrong resolution so it's blurry and jumpy while scrolling.

> Couldn't repro this either. Could you give some more details on what you
> mean by "not drawing properly"?

At first they were rendering too small (i.e. taking up only a subrect of the thumbnail area) but then opening a few tabs and navigating around I ended up in a state where all the thumbnails were for the active tab and some of them had strange colors.

	Botond Ballo [:botond] Assignee
	Comment 13 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> (In reply to Botond Ballo [:botond] from comment #11)
> > Does this always happen, or for particular pages? I browsed around a bit but
> > was unable to reproduce the problem.
> 
> Specific str i saw this with: load bug 1103329, click on the link to the
> oracle page there, scroll around for a bit, and then go back to the bug
> page. At this point the bug page renders at the wrong resolution so it's
> blurry and jumpy while scrolling.

Strangely, I still can't repro this. When going back to the bug page, it renders at the same zoom that I was at when I left it (i.e. when I clicked the Oracle link), and resolution-wise it's sharp.

	Botond Ballo [:botond] Assignee
	Comment 14 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #13)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> > (In reply to Botond Ballo [:botond] from comment #11)
> > > Does this always happen, or for particular pages? I browsed around a bit but
> > > was unable to reproduce the problem.
> > 
> > Specific str i saw this with: load bug 1103329, click on the link to the
> > oracle page there, scroll around for a bit, and then go back to the bug
> > page. At this point the bug page renders at the wrong resolution so it's
> > blurry and jumpy while scrolling.
> 
> Strangely, I still can't repro this. When going back to the bug page, it
> renders at the same zoom that I was at when I left it (i.e. when I clicked
> the Oracle link), and resolution-wise it's sharp.

Oh, I've just reproduced it, after clicking a link in a bug comment rather than in the bug header fields!

	Botond Ballo [:botond] Assignee
	Comment 15 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Minusing based on the two issues i saw on the Fennec try build locally. One
> was that when going back the page was rendered blurry, I think this can be
> fixed by changing ScrollFrameHelper::RestoreState to use the new
> SetResolutionAndScaleTo API.

That change does seem to have fixed the problem - thanks again! :)

The code change belongs more properly in bug 1076253; I'll post an updated patch there.

	Botond Ballo [:botond] Assignee
	Comment 16 • 10 years ago
try

Doing a Try push with the updated bug 1076253 patch, in the hopes that it might fix the Try failures from comment 9, which I haven't been able to reproduce locally: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0ba1b1b59318

	Botond Ballo [:botond] Assignee
	Comment 17 • 10 years ago

I was able to reproduce and fix the tab thumbnail issue described in comment 12.

The problem was that AndroidBridge::CaptureThumbnail() assumed that the RenderDocument() call it makes doesn't scale the content to the pres shell resolution. This was true before this patch, but now that we scale to the resolution, CaptureThumbnail() needs to account for this when computing the scale used to scale the rendered document down to fit the thumbnail.

This is unlikely to be cause of the canvas test failure (dom/canvas/test/test_drawWindow.html), but there's a good chance the cause of that is a similar assumption elsewhere in the code. Now if only I could reproduce it and track it down...

Anyways, I updated the patch with the CaptureThumbnail() fix. Will wait until the canvas test failure is resolved too before flagging for review.

	Botond Ballo [:botond] Assignee
	Comment 18 • 10 years ago

Here's an attempt to fix the dom/canvas/test/test_drawWindow.html failure by generalizing the compensation for scaling-to-resolution from AndroidBridge::CaptureThumbnail() to happen in PresShell::RenderDocument() instead.

I'm not yet sure if this is the right approach, but I wanted to see if it at least makes the canvas test pass and doesn't break anything: https://tbpl.mozilla.org/?tree=Try&rev=829cb0a1537f

	Botond Ballo [:botond] Assignee
	Comment 19 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #18)
> Here's an attempt to fix the dom/canvas/test/test_drawWindow.html failure by
> generalizing the compensation for scaling-to-resolution from
> AndroidBridge::CaptureThumbnail() to happen in PresShell::RenderDocument()
> instead.
> 
> I'm not yet sure if this is the right approach, but I wanted to see if it at
> least makes the canvas test pass and doesn't break anything:
> https://tbpl.mozilla.org/?tree=Try&rev=829cb0a1537f

There was a typo in my patch that caused it not to build. Try push with corrected patch: https://tbpl.mozilla.org/?tree=Try&rev=4ae10590c30b

	Botond Ballo [:botond] Assignee
	Comment 20 • 10 years ago

The Android canvas test failure is gone, but there's a permafail on each of B2G and Linux.

	Botond Ballo [:botond] Assignee
	Comment 21 • 10 years ago

Here's a Try push for another approach that came out of a discussion with Kats and Timothy, where we ignore the scale-to-resolution flag unless we're using a ClientLayerManager to paint. The rationale is that if we're using a ClientLayerManager, we're also using AsyncCompositionManager to apply async transforms in the compositor, and the async transforms have been changed to expect scaling-to-resolution, but other code hasn't changed.

https://tbpl.mozilla.org/?tree=Try&rev=45cf63abb2a4

There is still one B2G failure, dom/events/test/test_bug656379-1.html. I had a look at the test, and it makes a drawWindow() call. It's not clear where the above logic breaks down for this test; I will try to debug it.

	Botond Ballo [:botond] Assignee
	Comment 22 • 10 years ago

After a few cycles of doing try pushes with logging while I wasn't able to repro the b2g test failure mentioned in comment 21, I was finally able to repro it with an emulator opt build on my office desktop.

Debugging so far has revealed:

  - My changes introduce an invalidation bug. I need to investigate this.

  - The invalidation bug manifests as the test getting into an infinite
    loop. I believe this is suboptimal behaviour for a test. The attached
    patch simplifies the test in question. As a result, the test no longer
    gets into an infinite loop, but it also doesn't fail as a result of
    my invalidation bug (which is fine, because testing invalidation isn't
    the object of the test, but it does mean I'll want to add an
    invalidation test for my bug before landing it).

	Markus Stange [:mstange]
	Comment 23 • 10 years ago

Comment on attachment 8533390 [details] [diff] [review]
Simplify a test for bug 656379

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

Well, testing invalidation might also have been part of the test, but we should just use invalidation reftests for that.

	Botond Ballo [:botond] Assignee
	Comment 24 • 10 years ago

I investigated the reason for the invalidation bug, and thought about my overall approach some more.

The change my patch is making can be though of as consisting of two parts:

  1. We're making a change to the (regular, non-shadow) transforms of certain layers
     (the ones associated with a pres shell resolution) - specifically, we're no
     longer applying a post-scale of '1 / resolution'.

  2. We're making a corresponding change to the shadow transforms of these layers -
     specifically, we're no longer applying a shadow transform of 'scale(resolution)'.

The problem is that some code can observe the state of the layer tree between (1) and (2), i.e. with the regular (non-shadow) transforms applied, but the shadow transforms not yet applied. My patch changes the behaviour of such code.

The invalidation bug in question is caused by a specific case where this happens (the clientRects fields of the argument of the MozAfterPaint event), but there could in principle be other places that cause other problems not caught by tests.

This made me question whether this approach is the right one.

I can envision another approach which preserves the behaviour for code that observes the state of the layer tree between (1) and (2), too:

  - Continue applying the '1 / resolution' post-scale unconditionally.

  - If the scale-to-resolution flag is set, save that flag, and the resolution to
    scale to, in the relevant ContainerLayer.

  - Have AsyncCompositionManager apply a scale transform corresponding to that
    resolution to the Layer that bears it.

This basically keeps around the non-transient async transform, but changes it from something APZ keeps track of, to something that Layout keeps track of. This accomplishes the objective of unblocking containerless scrolling for root scroll frames.

I discussed this with Kats and Timothy, and they agree this is a reasonable solution. I'm going to give it a try.

	Botond Ballo [:botond] Assignee
	Comment 28 • 10 years ago

These new patches implement the approach described in comment 24, with one notable change, described below.

The Part 1 patch is largely an updated version of the patch that used to be in bug 1076253 (which I've closed).
The Part 2 patch is largely an updated version of the original patch in this bug.

The change is that rather than applying the scale-to-resolution transform to the shadow transform in AsyncCompositionManager, the patch applies it to the regular transform in LayerTransactionParent::RecvUpdate. The rationale is that keeping it as part of the regular transform rather than the shadow transform allows us to keep many of the simplifications in the Part 2 patch in place, while making it part of the shadow transform would actually complicate things further {coordinate systems}-wise (since now non-APZ layers could have an "async transform" of sorts).

Manual testing on B2G looks promising. My Android build is borked at the moment so I wasn't able to test it on Android.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c224fdebcba3

I'll put the patches up for review once the Try push is green (and possibly once I've been able to test them on Android).

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 29 • 10 years ago

Comment on attachment 8535972 [details] [diff] [review]
Part 2 - Begone, nontransient transform!

There are a lot of good explanations in this bug - is enough of it captured in the code comments as well?  Perhaps even as a pointer to the bug's comments...

	Botond Ballo [:botond] Assignee
	Comment 30 • 10 years ago

(In reply to Milan Sreckovic [:milan] from comment #29)
> Comment on attachment 8535972 [details] [diff] [review]
> Part 2 - Begone, nontransient transform!
> 
> There are a lot of good explanations in this bug - is enough of it captured
> in the code comments as well?  Perhaps even as a pointer to the bug's
> comments...

Good point; I'll add a comment pointing to relevant explanations in this bug in the patch for whichever approach ends up sticking (hopefully this one!).

	Botond Ballo [:botond] Assignee
	Comment 31 • 10 years ago

Progress update:

The Try run in comment 28 showed two reftest failures on b2g:

  layout/reftests/backgrounds/background-tiling-zoom-1.html
  layout/reftests/image/background-image-zoom-2.html

I had a look at these failures. The thing that's failing is not what the test is trying to test; rather, there is some general rendering misbehaviour that is being caught by the tests.

I can't reproduce the specific failures on my B2G device (either because the tests use reftest-zoom which I can't emulate easily on a device, or because of some emulator vs. device difference), but I do see some other rendering misbehaviour on the test page, which I've reproduced to the following:

   - Load a simple "Hello, world" page that does not have
     a meta-viewport tag (e.g. [1]), so that content is 
     rendered at an initial zoom < 1.

   - On the initial page load, there is a small grey seam
     separating the top-left rectangle of the page from
     the rest. The size of the top-left rectangle appears
     to be the zoom times the size of the screen.

   - On subsequent page loads, the seam disappears, but
     sometimes it remains there, and sometimes another
     grey strip appears across the whole screen, about
     halfway down the page.

I've used :dvander's layer visualizer tool [2] to try to pin down the cause of the problem, but I didn't see anything unexpected there. Other than the change I intended to make - moving the "non-transient async transform" out of the shadow transform and into the regular (non-shadow) transform - the layer trees before and after my change are the same, and the resulting layer sizes/positions are as well.

In the absence of any better ideas for how to get to the bottom of this, I will next try using the layer dump visualization feature of BenWa's profiler [3] to see which layer texture is contributing the seams, or if it's a bug compositing the layers.

[1] http://people.mozilla.org/~bballo/btz.html
[2] http://users.alliedmods.net/~dvander/layers/
[3] https://benoitgirard.wordpress.com/2014/11/28/improving-layer-dump-visualization/

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 10 years ago

You may find an approach like https://hg.mozilla.org/try/rev/34b23b17f281 useful for debugging on try. Specifically: (a) hack configure.in to enable display-list dumping, (b) remove all the reftests except the ones you care about (c) set prefs on the tests you care about to get logging that will help you and (d) make sure the prefs are listed in all.js so that the reftest harness knows what they are. You'll get something that looks like this: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=34b23b17f281 but amongst those oranges you'll find tests that got run and dumped useful output (see R4 for example).

	Botond Ballo [:botond] Assignee
	Comment 33 • 10 years ago

I finally traced down the misbehaviour I was seeing locally to the valid region of the a tiled painted layer being incorrect. It was being set incorrectly on the client side by code in ClientTiledPaintedLayer because my Part 2 patch removed a line in that code that accounted for the non-transient async transform. This removal was appropriate for the original approaches in the bug, but not for the new approaches (comment 24 onward).

Here's a Try push with the modified patch, to see if it fixes the reftest failures (and doesn't break anything else): https://tbpl.mozilla.org/?tree=Try&rev=e4db939c31d4

I'll post the updated patch for review once it's clean.

	Botond Ballo [:botond] Assignee
	Comment 34 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #33)
> Here's a Try push with the modified patch, to see if it fixes the reftest
> failures (and doesn't break anything else):
> https://tbpl.mozilla.org/?tree=Try&rev=e4db939c31d4

Doh, I didn't mean to do '-u none'. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=8e4848936eff

	Botond Ballo [:botond] Assignee
	Comment 35 • 10 years ago

(In reply to Botond Ballo [:botond] from comment #34)
> (In reply to Botond Ballo [:botond] from comment #33)
> > Here's a Try push with the modified patch, to see if it fixes the reftest
> > failures (and doesn't break anything else):
> > https://tbpl.mozilla.org/?tree=Try&rev=e4db939c31d4
> 
> Doh, I didn't mean to do '-u none'. Trying again:
> https://tbpl.mozilla.org/?tree=Try&rev=8e4848936eff

Finally, a clean Try push! :)

Rebased patches incoming.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 38 • 10 years ago

Comment on attachment 8539606 [details] [diff] [review]
Part 1 - Introduce a new API for setting a resolution and scaling to it

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

Minusing based on below comments, but fundamentally this looks fine.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +391,5 @@
> +        if (attrs.scaleToResolution()) {
> +          float resolution = attrs.presShellResolution();
> +          containerLayer->SetPostScale(
> +              containerLayer->GetPostXScale() * resolution,
> +              containerLayer->GetPostYScale() * resolution);

I'm a little leery of combining this with the post-scale here. I would rather have this transaction code just call containerLayer->SetScaleToResolution and then modify the GetPostXScale/GetPostYScale and whatever other use sites there are for mPostXScale/mPostYScale to take this into account. This is mostly for clarity because it's going to be confusing if the post-scale "magically" isn't the same on the client and compositor layer trees when debugging.

::: layout/generic/nsIScrollableFrame.h
@@ +157,5 @@
> +   * Set the element resolution and specify that content should be scaled by
> +   * the amount of the resolution. This is only meaningful for root scroll
> +   * frames. See nsIDOMWindowUtils.setResolutionAndScaleTo().
> +   */
> +  virtual void SetResolutionAndScaleTo(const gfxSize& aResolution) = 0;

This doesn't appear to be called from anywhere (at least not in this patch). If it's not needed we can remove it along with a lot of the changes ins nsGfxScrollFrame.*

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 39 • 10 years ago

Comment on attachment 8539607 [details] [diff] [review]
Part 2 - Move slightly over, nontransient transform!

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

LGTM

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +722,2 @@
>        scale *= metrics.mPresShellResolution;
> +

nit: spurious blank line

	Botond Ballo [:botond] Assignee
	Comment 40 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38)
> Comment on attachment 8539606 [details] [diff] [review]
> Part 1 - Introduce a new API for setting a resolution and scaling to it
> 
> Review of attachment 8539606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing based on below comments, but fundamentally this looks fine.
> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +391,5 @@
> > +        if (attrs.scaleToResolution()) {
> > +          float resolution = attrs.presShellResolution();
> > +          containerLayer->SetPostScale(
> > +              containerLayer->GetPostXScale() * resolution,
> > +              containerLayer->GetPostYScale() * resolution);
> 
> I'm a little leery of combining this with the post-scale here. I would
> rather have this transaction code just call
> containerLayer->SetScaleToResolution and then modify the
> GetPostXScale/GetPostYScale and whatever other use sites there are for
> mPostXScale/mPostYScale to take this into account. This is mostly for
> clarity because it's going to be confusing if the post-scale "magically"
> isn't the same on the client and compositor layer trees when debugging.

Discussed with with kats on IRC and settled on the following approach:
  - make Layer::GetPost[X|Y]Scale virtual
  - override it in ContainerLayerComposite to multiply in mPresShellResolution
  - in LayerTransactionParent, just transfer mScaleToResolution and mPresShellResolution
    (don't touch the scales(
  - use Get[X|Y]PostScale instead of m[X|Y]PostScale where appropriate
    (notably, in Layer::GetTransform())
  - layer dumps continue printing m[X|Y]PostScale (this way compositor- and
    client-side dumps show the same scales), with container layers also
    printing mPresShellResolution

> ::: layout/generic/nsIScrollableFrame.h
> @@ +157,5 @@
> > +   * Set the element resolution and specify that content should be scaled by
> > +   * the amount of the resolution. This is only meaningful for root scroll
> > +   * frames. See nsIDOMWindowUtils.setResolutionAndScaleTo().
> > +   */
> > +  virtual void SetResolutionAndScaleTo(const gfxSize& aResolution) = 0;
> 
> This doesn't appear to be called from anywhere (at least not in this patch).
> If it's not needed we can remove it along with a lot of the changes ins
> nsGfxScrollFrame.*

nsDOMWindowUtils::SetResolutionAndScaleTo() was supposed to call this. It's needed to avoid the issue described in comment 12. That hunk accidentally got dropped while rebasing/folding. Thanks for catching it!

The updated patch fixes these issues.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 41 • 10 years ago

Comment on attachment 8540383 [details] [diff] [review]
Part 1 - Introduce a new API for setting a resolution and scaling to it

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +5080,4 @@
>  
>    if (mIsRoot) {
> +    nsIPresShell* presShell = mOuter->PresContext()->PresShell();
> +    if (mScaleToResolution) {

it should be safe to assert !mScaleToResolution if !mRoot, I think. Please add an assertion to that effect if you agree. (MOZ_ASSERT(mIsRoot || !mScaleToResolution) outside the if (mIsRoot) should be sufficient).

	Botond Ballo [:botond] Assignee
	Comment 42 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #41)
> it should be safe to assert !mScaleToResolution if !mRoot, I think. Please
> add an assertion to that effect if you agree. (MOZ_ASSERT(mIsRoot ||
> !mScaleToResolution) outside the if (mIsRoot) should be sufficient).

I agree. Updated patch to add assertion. Carrying r+ from kats.

Markus, could you have a look at this, either in addition to, or if you feel confident enough, instead of, Timothy? Thanks!

	Markus Stange [:mstange]
	Comment 43 • 9 years ago

Comment on attachment 8543456 [details] [diff] [review]
Part 1 - Introduce a new API for setting a resolution and scaling to it

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

I don't feel comfortable replacing tn's review here.

In what cases is scaleToResolution false, i.e. when do we still need the old setResolution methods?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3266,5 @@
> +}
> +
> +void
> +ScrollFrameHelper::SetResolutionAndScaleTo(const gfxSize& aResolution)
> +{

Can you assert mIsRoot here too?

	Botond Ballo [:botond] Assignee
	Comment 44 • 9 years ago

(In reply to Markus Stange [:mstange] from comment #43)
> In what cases is scaleToResolution false, i.e. when do we still need the old
> setResolution methods?

It doesn't appear like we use it anywhere in mozilla-central, but since it's an nsIDOMWindowUtils API, add-ons might be using it.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +3266,5 @@
> > +}
> > +
> > +void
> > +ScrollFrameHelper::SetResolutionAndScaleTo(const gfxSize& aResolution)
> > +{
> 
> Can you assert mIsRoot here too?

Yep, good idea. Updated patch to do that.

	Botond Ballo [:botond] Assignee
	Comment 45 • 9 years ago
green

Thanks!

Another Try push, with patches rebased to recent m-c: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=db2936819a76

	Botond Ballo [:botond] Assignee
	Comment 46 • 9 years ago

Looking green! Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42272b7f8e48
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c67bb878ff

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 47 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/42272b7f8e48
https://hg.mozilla.org/mozilla-central/rev/a4c67bb878ff