Closed Bug 1482665 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
Attachment #8999406 - Attachment description: Bug 1482665 Part 2 - Remove nsPresContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel(). Depends on D3154 → Bug 1482665 Part 2 - Remove nsPresContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().
Comment on attachment 8999405 [details]
Bug 1482665 Part 1 - Add constexpr to mozilla::AppUnitsPerCSSPixel() and mozilla::AppUnitsPerCSSInch().

Daniel Holbert [:dholbert] has approved the revision.
Attachment #8999405 - Flags: review+
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.
Attachment #8999406 - Flags: review+
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 on attachment 8999407 [details]
Bug 1482665 Part 3 - Remove nsPresContext::AppUnitsPerCSSInch() and replace it by mozilla::AppUnitsPerCSSInch().

Daniel Holbert [:dholbert] has approved the revision.
Attachment #8999407 - Flags: review+
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.
Attachment #8999409 - Flags: review+
(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.
(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 on attachment 8999408 [details]
Bug 1482665 Part 4 - Remove nsDeviceContext::AppUnitsPerCSSPixel() and replace it by mozilla::AppUnitsPerCSSPixel().

Daniel Holbert [:dholbert] has approved the revision.
Attachment #8999408 - Flags: review+
(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.)
(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.
Pushed by aethanyc@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.