1837832 - Change the parameter types of BasePoint::MoveTo() to Coord

Status: RESOLVED FIXED

(Core :: Graphics, task, P3)

Description

[Botond Ballo [:botond]]

As a next step in the effort to use strongly-typed coordinates where appropriate in the interface of types deriving from BasePoint, let's convert the parameter types of BasePoint::MoveTo() from T to Coord.

Comment 1

[thiagoms.15]

Hi, I'd like to work on it. This is my first bug. Is it only to update the interfaces BasePoint::MoveTo() and BasePoint::MoveBy()?

Comment 2

[Botond Ballo [:botond]]

(In reply to thiagoms.15 from comment #1)

Hi, I'd like to work on it. This is my first bug.

Hi, thanks for your interest!

The first step is to check out the Firefox source code and build it locally. You can find instructions at https://firefox-source-docs.mozilla.org/setup/index.html (choose the link corresponding your operating system under "Setting Up Your Machine"). For this bug, since we'll be modifying C++ code, we want a non-Artifact Build, so when the bootstrap script asks you to select the type of build, please be sure to select "2. Firefox for Desktop".

Once you have a successful build, please post back here and I'll provide further details. If you run into any errors, feel free to ask about them here, or on Matrix (you can find me in the #apz channel).

Comment 3

[thiagoms.15]

ok, I have a build with success. I did the following change:

diff --git a/gfx/2d/BasePoint.h b/gfx/2d/BasePoint.h
--- a/gfx/2d/BasePoint.h
+++ b/gfx/2d/BasePoint.h
@@ -37,11 +37,11 @@ struct BasePoint {
   MOZ_ALWAYS_INLINE Coord X() const { return x; }
   MOZ_ALWAYS_INLINE Coord Y() const { return y; }
 
-  void MoveTo(T aX, T aY) {
+  void MoveTo(Coord aX, Coord aY) {
     x = aX;
     y = aY;
   }
-  void MoveBy(T aDx, T aDy) {
+  void MoveBy(Coord aDx, Coord aDy) {
     x += aDx;
     y += aDy;
   }

I ran only the gtest:

$ ./mach gtest 
$ ./mach gtest " Moz2D.*"

do you suggest more tests to run regarding the Graph?

Comment 4

[Botond Ballo [:botond]]

The main things to do would be:

1. Check that code builds successfully with mach build after the changes.
2. Use our code search tool, Searchfox, to find call sites of these functions in the codebase (BasePoint::MoveTo, BasePoint::MoveBy). Often places that need adjustment will give a compiler error, but other times an adjustment is appropriate even if it the code would compile (for example, if a call site was unwrapping a CSSIntCoord variable coord to its contained int32_t value by passing coord.value, that would continue to compile, but it's better to pass coord without unwrapping after this change). (Eyeballing the call sites, it looks like neither of these may be applicable, just mentioning for future reference.)

Once you've done that, you can go ahead and submit the patch for review (instructions for that can be found at https://firefox-source-docs.mozilla.org/setup/contributing_code.html#getting-your-code-reviewed).

Regarding tests: once you've posted the patch for review, I'll kick off an automated test run for it which will run a bunch of test suites in our CI.

Comment 5

[thiagoms.15]

Attachment: Bug 1837832 - Change parameter types from T to Coord where appropriate in the remainder of the BasePoint interface r=botond

Comment 6

[thiagoms.15]

Thanks for the tip of Search, this is amazing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/905a6163df1f
Change parameter types from T to Coord where appropriate in the remainder of the BasePoint interface r=botond

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/905a6163df1f