Closed Bug 1139675 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(5 files, 1 obsolete file)

39 bytes, text/x-review-board-request
kats
: review+
mstange
: review+
botond
: review+
yzen
: review+
Details
39 bytes, text/x-review-board-request
yzen
: review+
mstange
: review+
kats
: review+
botond
: review+
Details
39 bytes, text/x-review-board-request
kats
: review+
mstange
: review+
botond
: review+
yzen
: review+
Details
39 bytes, text/x-review-board-request
kats
: review+
mstange
: review+
botond
: review+
yzen
: review+
Details
39 bytes, text/x-review-board-request
mstange
: review+
kats
: review+
botond
: review+
yzen
: review+
Details
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.
Assignee: nobody → 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).
Attached file MozReview Request: bz://1139675/botond (obsolete) —
/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
Attachment #8574209 - Flags: review?(yzenevich)
Attachment #8574209 - Flags: review?(mstange)
Attachment #8574209 - Flags: review?(matt.woodrow)
Attachment #8574209 - Flags: review?(bugmail.mozilla)
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
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
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 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
Attachment #8574209 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

Thanks, patch 3 looks good!
Attachment #8574209 - Flags: review?(yzenevich) → review+
Hmm, that still didn't translate into an r+. Very confusing.
Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

(r+, from the attachment details page, this time)
Attachment #8574209 - Flags: review?(mstange) → review+
Comment on attachment 8574209 [details]
MozReview Request: bz://1139675/botond

Marking this as r+ based on Matt's "Ship It!" in comment 11.
Attachment #8574209 - Flags: review?(matt.woodrow) → review+
Attachment #8574209 - Attachment is obsolete: true
Attachment #8619659 - Flags: review+
Attachment #8619660 - Flags: review+
Attachment #8619661 - Flags: review+
Attachment #8619662 - Flags: review+
Attachment #8619663 - Flags: review+
You need to log in before you can comment on or make changes to this bug.