1076253 - Introduce an API for setting the pres shell resolution for the purpose of zooming

Status: RESOLVED DUPLICATE of bug 1076241

(Core :: Layout, defect)

Description

[Botond Ballo [:botond]]

Currently, when we set a pres shell resolution, Layout sets a compensating post-scale on the root layer, so that the size of things on the screen doesn't actually change.

Callers that want the size of things on the screen to change need to make that happen themselves, e.g. by setting an additional transform. For example, APZ, which increases the pres shell resolution when pinch-zooming in, and wants things to appear correspondingly bigger on the screen, sets a "non-transient async transform" to make this happen.

The problem is that such callers need to know what layer the post-scale is set on, so they know to set the transform on the same layer.

It would be simpler if Layout had an API for increasing the pres shell resolution and *not* setting a compensating post-scale. Callers in the above category could then just use this new API, and not have to compensate themselves.

Comment 1

[Botond Ballo [:botond]]

I'm going to start looking at this as part of stage 2 of apz-css-transforms.

Comment 2

[Botond Ballo [:botond]]

Attachment: bug1076253.patch

Here's my initial attempt at this.

Asking for feedback on the approach; will wait with posting for review until code that successfully exercises the  API is written (bug 1076241).

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8508358 [details] [diff] [review]
bug1076253.patch

Looks good to me. I think you'll need an iid bump on nsIPresShell.

Comment 4

[Botond Ballo [:botond]]

Attachment: bug1076253.patch

Updated patch to bump nsIPresShell's IID.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8511190 [details] [diff] [review]
bug1076253.patch

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

I have successfully used the new API in bug 1076241, and haven't run into any problems.

Flagging patch for review. Timothy: naturally, the bug 1076163 review is higher priority than this, as it's earlier in the dependency chain.

Comment 6

[Timothy Nikkel (:tnikkel)]

I'm pretty sure I'm okay with this from when I feedback+'d it, but we should probably get at least one other person to agree with the api change.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8511190 [details] [diff] [review]
bug1076253.patch

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

Kats, do you agree with the API change as per comment 6?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8511190 [details] [diff] [review]
bug1076253.patch

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

API looks good to me!

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +201,5 @@
>     * Setting a new resolution does *not* trigger reflow.  This API is
>     * entirely separate from textZoom and fullZoom; a resolution scale
>     * can be applied together with both textZoom and fullZoom.
>     *
>     * The effect of is API for gfx code to allocate more or fewer

englishing needs fixing

::: layout/base/nsDisplayList.cpp
@@ +1370,5 @@
>      layerManager->SetUserData(&gLayerManagerLayerBuilder, oldBuilder);
>      return;
>    }
>    // Root is being scaled up by the X/Y resolution. Scale it back down.
> +  if (!presShell->ScaleToResolution()) {

Move comment inside the if, or append "unless the ScaleToResolution predicate is true"

::: layout/base/nsIPresShell.h
@@ +1391,5 @@
> +
> +  /**
> +   * Return whether we are scaling to the set resolution.
> +   * This is initially false; it's set to true by a call to
> +   * SetResolutionAndScaleTo(), and set t false by a call to SetResolution().

s/ t / to /

::: layout/base/nsPresShell.cpp
@@ +729,5 @@
>  }
>  
>  PresShell::PresShell()
> +  : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE),
> +    mScaleToResolution(false)

How come all the other bitflags aren't initialized here?

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> >  }
> >  
> >  PresShell::PresShell()
> > +  : mMouseLocation(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE),
> > +    mScaleToResolution(false)
> 
> How come all the other bitflags aren't initialized here?

PresShell uses a zeroing new
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.h#68
so you don't need to initialize members if zeroes are what you want.

Comment 10

[Botond Ballo [:botond]]

Attachment: bug1076253.patch

Updated to address review comments. Carrying r+.

Comment 11

[Botond Ballo [:botond]]

Attachment: bug1076253.patch

While exercising the new API in bug 1076241, we discovered that the implementation was incomplete, because the scale-to-resolution flag did not survive a SaveState/RestoreState cycle. We need to store the flag on the ScrollFrameHelper and in the nsPresState as well.

I'll wait for some Try results before posting the updated patch for review.

Comment 12

[Botond Ballo [:botond]]

I'm going to close this bug as a duplicate of bug 1076241. The patch here and the one in bug 1076241 are pretty closely related and often change in tandem, so it makes more sense to just handle them as two patches in the same bug.