Closed Bug 1076253 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1076241

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
I'm going to start looking at this as part of stage 2 of apz-css-transforms.
Assignee: nobody → botond
Attached patch bug1076253.patch (obsolete) — Splinter Review
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).
Attachment #8508358 - Flags: feedback?(tnikkel)
Comment on attachment 8508358 [details] [diff] [review]
bug1076253.patch

Looks good to me. I think you'll need an iid bump on nsIPresShell.
Attachment #8508358 - Flags: feedback?(tnikkel) → feedback+
Attached patch bug1076253.patch (obsolete) — Splinter Review
Updated patch to bump nsIPresShell's IID.
Attachment #8508358 - Attachment is obsolete: true
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.
Attachment #8511190 - Flags: review?(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 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?
Attachment #8511190 - Flags: feedback?(bugmail.mozilla)
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?
Attachment #8511190 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8511190 - Flags: review?(tnikkel) → review+
Attached patch bug1076253.patch (obsolete) — Splinter Review
Updated to address review comments. Carrying r+.
Attachment #8511190 - Attachment is obsolete: true
Attachment #8527031 - Flags: review+
Attached patch bug1076253.patchSplinter Review
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.
Attachment #8527031 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: begone-nontransient
You need to log in before you can comment on or make changes to this bug.