1482665 - Remove AppUnitsPerCSSPixel() and AppUnitsPerCSSInch() in nsPresContext and nsDeviceContext

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P4)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Spin-off from bug 1482514 comment 3:

we can gid of nsPresContext::AppUnitsPerCSSPixel(), nsPresContext::AppUnitsPerCSSInch(), nsDeviceContext::AppUnitsPerCSSPixel(), and nsDeviceContext()::AppUnitsPerAppUnitsPerCSSInch() because they're equivalent to those defined in gfx/src/AppUnits.h.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1482665 Part 1 - Add constexpr to mozilla::AppUnitsPerCSSPixel() and mozilla::AppUnitsPerCSSInch().

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1482665 Part 2 - Remove nsPresContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1482665 Part 3 - Remove nsPresContext::AppUnitsPerCSSInch() and replace it by mozilla::AppUnitsPerCSSInch().

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1482665 Part 4 - Remove nsDeviceContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1482665 Part 5 - Remove nsDeviceContext::AppUnitsPerCSSInch() and replace it by mozilla::AppUnitsPerCSSInch().

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8999405 [details]
Bug 1482665 Part 1 - Add constexpr to mozilla::AppUnitsPerCSSPixel() and mozilla::AppUnitsPerCSSInch().

Daniel Holbert [:dholbert] has approved the revision.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8999406 [details]
Bug 1482665 Part 2 - Remove nsPresContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().

Daniel Holbert [:dholbert] has approved the revision.

Comment 8

[Daniel Holbert [:dholbert]]

Commit-message nit that applies to patches 2-5:

s/by/with/

("replace it by" isn't quite grammatically correct. You mean "replace it with")

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8999407 [details]
Bug 1482665 Part 3 - Remove nsPresContext::AppUnitsPerCSSInch() and replace it by mozilla::AppUnitsPerCSSInch().

Daniel Holbert [:dholbert] has approved the revision.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8999409 [details]
Bug 1482665 Part 5 - Remove nsDeviceContext::AppUnitsPerCSSInch() and replace it by mozilla::AppUnitsPerCSSInch().

Daniel Holbert [:dholbert] has approved the revision.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Commit-message nit that applies to patches 2-5:
> 
> s/by/with/
> 
> ("replace it by" isn't quite grammatically correct. You mean "replace it
> with")

Fixed the comment message in patches 2-5. I don't fix the description of the revisions on Differential UI, though.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #11)
> Fixed the comment message in patches 2-5. I don't fix the description of the
> revisions on Differential UI, though.

I think the description on Differential UI is what actually ends up in the Lando-generated commit message, if I remember correctly...

i.e. the local commit message is what is used to generate the initial Differential UI description, but then from that point on, the Differential UI is the source of truth, at least if you're using Lando.

It doesn't matter a whole lot in this case, because the typo isn't severe and it'd be fine if it landed unfixed, but might be worth keeping an eye on when landing if you're curious (and updating the description if needed).

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8999408 [details]
Bug 1482665 Part 4 - Remove nsDeviceContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().

Daniel Holbert [:dholbert] has approved the revision.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> I think the description on Differential UI is what actually ends up in the
> Lando-generated commit message, if I remember correctly...

(and I think this comes from the fact that arc/phabricator expects you to be making multiple commits that get squashed together in a single Revision [including commits to address review comments], and it wants to let you have meaningful local commit messages for your fixes, without having to worry about those messages showing up in the final polished patch.)

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> (In reply to Daniel Holbert [:dholbert] from comment #12)
> > I think the description on Differential UI is what actually ends up in the
> > Lando-generated commit message, if I remember correctly...
> 
> (and I think this comes from the fact that arc/phabricator expects you to be
> making multiple commits that get squashed together in a single Revision
> [including commits to address review comments], and it wants to let you have
> meaningful local commit messages for your fixes, without having to worry
> about those messages showing up in the final polished patch.)

That makes sense. I see Lando uses the message on the Differential UI as the final commit message. However, since the support for landing multiple patches is still ongoing (bug 1457525), I'd better push them to inbound myself so that they are in one push rather than 5 on autoland.

Comment 16

[Daniel Holbert [:dholbert]]

Makes sense. Thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/975fd6febbea
Part 1 - Add constexpr to mozilla::AppUnitsPerCSSPixel() and mozilla::AppUnitsPerCSSInch(). r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61b7e1a0216
Part 2 - Remove nsPresContext::AppUnitsPerCSSPixel() and replace it with mozilla::AppUnitsPerCSSPixel(). r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/67c05f411b48
Part 3 - Remove nsPresContext::AppUnitsPerCSSInch() and replace it with mozilla::AppUnitsPerCSSInch(). r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed67e5b2479a
Part 4 - Remove nsDeviceContext::AppUnitsPerCSSPixel() and replace it with mozilla::AppUnitsPerCSSPixel(). r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bbf7aa0ab2e
Part 5 - Remove nsDeviceContext::AppUnitsPerCSSInch() and replace it with mozilla::AppUnitsPerCSSInch(). r=dholbert

Comment 18

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/975fd6febbea
https://hg.mozilla.org/mozilla-central/rev/f61b7e1a0216
https://hg.mozilla.org/mozilla-central/rev/67c05f411b48
https://hg.mozilla.org/mozilla-central/rev/ed67e5b2479a
https://hg.mozilla.org/mozilla-central/rev/0bbf7aa0ab2e