1139675 - Modify nsIPresShell::SetResolution() and related APIs to only take a single resolution parameter

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

nsIPresShell::SetResolution and related APIs (nsIPresShell::SetResolutionAndScaleTo, and the respective nsIDOMWindowUtils wrappers) currently accept separate resolutions for the x and y axes. 

However, no one actually passes different values for x and y, and there doesn't appear to be a legitimate use case for doing so.

In bug 1036967, I'm introducing a ScaleFactors2D class to represent cases where we legitimately need separate x and y scales. 

I don't want to needlessly use this for the pres shell resolution, but I also don't want to leave the code as-is, where it just ignores the y resolution.

Rather, I'd like to update the above-mentioned APIs to take just a single resolution.

Comment 1

[Botond Ballo [:botond]]

This dependency isn't a logical one, it just reflects the order in which I'm writing the patches (as they both touch the same code).

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1139675/botond

/r/5013 - Bug 1139675 - Add a couple of operator overloads to BaseSize. r=kats
/r/5015 - Bug 1139675 - Simplify the APIs for getting and setting the pres shell resolution. r=mstange,mattwoodrow
/r/5017 - Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in accessibility code. r=yzen
/r/5019 - Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in metro browser code. r=kats
/r/5021 - Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in Fennec code. r=kats

Pull down these commits:

hg pull review -r 6514cf4067165bb02da42a360538ffad9f56888f

Comment 3

[Botond Ballo [:botond]]

To be clear (since MozReview doesn't make this clear in Bugzilla), the review requests are:

  Kats:            Patches 1, 4, and 5
  Markus and Matt: Patch 2
  Yura:            Patch 3

Comment 4

[Botond Ballo [:botond]]

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

Comment 5

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/5015/#review4013

::: dom/interfaces/base/nsIDOMWindowUtils.idl
(Diff revision 1)
> -   * either or both dimensions.  The scale at which the content is
> +   * both dimensions.  The scale at which the content is displayed does 

whitespace at end of line

Comment 6

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

https://reviewboard.mozilla.org/r/5013/#review4017

::: gfx/2d/BaseSize.h
(Diff revision 1)
> +  friend Sub operator/(T aScale, const Sub& aSize) {

Um, this allows dividing scales by sizes and getting back a size? That doesn't seem logical.

Comment 7

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

https://reviewboard.mozilla.org/r/5019/#review4019

Comment 8

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

https://reviewboard.mozilla.org/r/5021/#review4021

Ship It!

Comment 9

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

Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

r+ on patches 1, 4, and 5 assuming you drop the operator/ that you added in BaseSize.h

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> ::: gfx/2d/BaseSize.h
> (Diff revision 1)
> > +  friend Sub operator/(T aScale, const Sub& aSize) {
> 
> Um, this allows dividing scales by sizes and getting back a size? That
> doesn't seem logical.

Heh, you're right - with the operands in this order, only multplication makes sense. Thanks for catching this!

Comment 11

[Matt Woodrow (:mattwoodrow)]

https://reviewboard.mozilla.org/r/5015/#review4033

Ship It!

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

Thanks, patch 3 looks good!

Comment 13

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/5015/#review4281

Ship It!

Comment 14

[Markus Stange [:mstange]]

Hmm, that still didn't translate into an r+. Very confusing.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

(r+, from the attachment details page, this time)

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

Marking this as r+ based on Matt's "Ship It!" in comment 11.

Comment 17

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/557224c34c6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3a0af3e57d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b23cb78d77a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/21b76c93278b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c4a96d089a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/557224c34c6a
https://hg.mozilla.org/mozilla-central/rev/fa3a0af3e57d
https://hg.mozilla.org/mozilla-central/rev/b23cb78d77a0
https://hg.mozilla.org/mozilla-central/rev/21b76c93278b
https://hg.mozilla.org/mozilla-central/rev/f4c4a96d089a

Comment 20

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1139675 - Add a couple of operator overloads to BaseSize. r=kats

Comment 21

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in accessibility code. r=yzen

Comment 22

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in metro browser code. r=kats

Comment 23

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1139675 - Update JS callers of nsIDOMWindowUtils.setResolution and getResolution in Fennec code. r=kats

Comment 24

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1139675 - Simplify the APIs for getting and setting the pres shell resolution. r=mstange,mattwoodrow