1677643 - Replace nsIScrollableFrame::{HORIZONTAL, VERTICAL} with ScrollDirections

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Botond Ballo [:botond]]

In bug 1507189, we replaced the ScrollableDirection enum (which had separate values for "horizontal", "vertical", and "either") with ScrollDirection (which has just "horizontal" and "vertical", but there's also an EnumSet used to represent combinations).

There are also HORIZONTAL and VERTICAL enumerators in an anonymous enumeration in nsIScrollableFrame could be similarly replaced with ScrollDirection.

Comment 1

[Nirmay Dhruv [:nirmay]]

(In reply to Botond Ballo [:botond] [away until Jan 4] from comment #0)

In bug 1507189, we replaced the ScrollableDirection enum (which had separate values for "horizontal", "vertical", and "either") with ScrollDirection (which has just "horizontal" and "vertical", but there's also an EnumSet used to represent combinations).

There are also HORIZONTAL and VERTICAL enumerators in an anonymous enumeration in nsIScrollableFrame could be similarly replaced with ScrollDirection.

Hello,
Can I work upon this bug if no one has been assigned? It would be my first time working on a bug and would to work upon it.

Comment 2

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

Yes, feel free to work on this bug. It will automatically be assigned to you when you submit a patch. To get started you will need to follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions to build Firefox if you haven't already. Once you have it successfully built, you can make the necessary code changes and ensure everything still builds. Definitely ask questions if you get stuck!

Comment 3

[Nirmay Dhruv [:nirmay]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)

Yes, feel free to work on this bug. It will automatically be assigned to you when you submit a patch. To get started you will need to follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions to build Firefox if you haven't already. Once you have it successfully built, you can make the necessary code changes and ensure everything still builds. Definitely ask questions if you get stuck!

Thank you!
Yes, I have already built Firefox successfully.
I saw nslScrollableFrame line no. 94, bug 1507189(already fixed) and https://hg.mozilla.org/integration/autoland/rev/9cf73428357c for more reference. So, I just need to replace the words HORIZONTAL and VERTICAL with some other. But I was confused, do I need to replace HORIZONTAL with HorizontalScrollDirection or ScrollDirection::eHorizontal, similarly for VERTICAL with VerticalScrollDirection or ScrollDirection::eVertical?
Please help me progress over it.

Comment 4

[Botond Ballo [:botond]]

(In reply to nirmay.dhruv from comment #3)

So, I just need to replace the words HORIZONTAL and VERTICAL with some other. But I was confused, do I need to replace HORIZONTAL with HorizontalScrollDirection or ScrollDirection::eHorizontal, similarly for VERTICAL with VerticalScrollDirection or ScrollDirection::eVertical?

We'll also want to change the type of variables that currently store the HORIZONTAL and VERTICAL flags (for example, this one), from uint32_t to ScrollDirections.

The usages of HORIZONTAL and VERTICAL will then need to be replaced with equivalents using the EnumSet API. For example, an expression like directions & nsIScrollableFrame::VERTICAL would now be written directions.contains(ScrollDirection::eVertical).

Comment 5

[Nirmay Dhruv [:nirmay]]

(In reply to Botond Ballo [:botond] [away until Jan 4] from comment #4)

(In reply to nirmay.dhruv from comment #3)

So, I just need to replace the words HORIZONTAL and VERTICAL with some other. But I was confused, do I need to replace HORIZONTAL with HorizontalScrollDirection or ScrollDirection::eHorizontal, similarly for VERTICAL with VerticalScrollDirection or ScrollDirection::eVertical?

We'll also want to change the type of variables that currently store the HORIZONTAL and VERTICAL flags (for example, this one), from uint32_t to ScrollDirections.

The usages of HORIZONTAL and VERTICAL will then need to be replaced with equivalents using the EnumSet API. For example, an expression like directions & nsIScrollableFrame::VERTICAL would now be written directions.contains(ScrollDirection::eVertical).
I looked over the files and links you suggested.
So in changing the type of variables that currently store the HORIZONTAL and VERTICAL flags, I think we just need to change uint32_t to ScrollDirections here https://searchfox.org/mozilla-central/rev/0eebf69d04cc0cdd252b135dda2fa9b7c2fa2bfb/layout/base/PresShell.cpp#3450 (the example you specified) at this one place only.
And in the second case for the conditions like directions & nsIScrollableFrame::VERTICAL, I would find each occurrence of nsIScrollableFrame::VERTICAL and nsIScrollableFrame::HORIZONTAL like this https://searchfox.org/mozilla-central/search?q=nsIScrollableFrame%3A%3AHORIZONTAL&path=&case=true&regexp=false and replace them as you told me. Right?
But also what do I replace just HORIZONTAL and VERTICAL or nsIScrollableFrame::HORIZONTAL and nsIScrollableFrame::VERTICAL occurrences with?
Thank you for your guidance.

Comment 6

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

That sounds like a good place to start. As you work on the patch the compiler may point out other occurrences that you need to replace.

(In reply to nirmay.dhruv from comment #5)

But also what do I replace just HORIZONTAL and VERTICAL or nsIScrollableFrame::HORIZONTAL and nsIScrollableFrame::VERTICAL occurrences with?

You'd replace them with ScrollDirection::eHorizontal or ScrollDirection::eVertical.

Comment 7

[Nirmay Dhruv [:nirmay]]

Alright, got it. I will give it a try, figure it out and would let you know what happened or ask for any help if required in the process.
Thank you.

Comment 8

[Nirmay Dhruv [:nirmay]]

Sorry I was not able to work much for a few days. I did try a fews changes as told but am doing some wrong. It is not building properly. So I am having a few queries. Here I am searching every occurrence of uint32_t directions and choose the ones that store the HORIZONTAL and VERTICAL flags. Also here I am finding occurrences of HORIZONTAL or VERTICAL(similarly). I am confused with that HORIZONTAL and nsIScrollableFrame::HORIZONTAL are different so they would have different replacements. So ScrollDirection::eHorizontal as replacement would be for HORIZONTAL or nsIScrollableFrame::HORIZONTAL and what would be for the other one.

I tried a few thing by myself but didn't seem to work, please help me move forward.

Comment 9

[Botond Ballo [:botond]]

(In reply to nirmay.dhruv from comment #8)

Here I am searching every occurrence of uint32_t directions and choose the ones that store the HORIZONTAL and VERTICAL flags. Also here I am finding occurrences of HORIZONTAL or VERTICAL(similarly).

I would suggest starting from the occurrences of HORIZONTAL and VERTICAL. For each one, change to the corresponding ScrollDirection, and then propagate the change to affected variables and functions.

Note that Searchfox supports semantic search. So, rather than doing a textual search for HORIZONTAL which turns up all textual occurrences of that word, many of which refer to other variables named HORIZONTAL, you can visit the definition of nsIScrollableFrame::HORIZONTAL, click on it, and select "Search for enum constant nsIScrollableFrame::HORIZONTAL" from the context menu. This will give you a list of uses only of nsIScrollableFrame::HORIZONTAL, and not other variables named HORIZONTAL.

Let's go through an example. The first result of the mentioned semantic search is here.

• Observe that it's part of an expression directions & nsIScrollableFrame::HORIZONTAL. To convert this expression to use ScrollDirections:
  • The constant needs to change from nsIScrollableFrame::HORIZONTAL to ScrollDirection::eHorizontal.
  • The type of the directions variable needs to change from uint32_t to ScrollDirections.
  • The form of the expression needs to change from a & b to a.contains(b).
    • Other types of expression may have other equivalents in the EnumSet API. I encourage you to look through the EnumSet class to determine what these are, and ask here if you're unsure.
• The change of type to directions needs to be propagated to its other uses.
  • It's currently initialized to scrollableFrame->GetAvailableScrollingDirectionsForUserInputEvents(). We can use Searchfox to find the definition of this function here. The function's signature will need to change to return ScrollDirections instead of uint32_t.
    • This change to the function's signature will in turn require adjustments to the function's definition, and to its other callers (which can be found using Searchfox).
    • In this case, it's a virtual function, so overriding functions need a corresponding signature change. In this case, searching for the identifier shows there are two overriding functions here and here.
  • The remaining uses are also expressions of them form a & b which can be converted as above.

It may sometimes seem that the dependency chain goes very deep, but soon enough you'll start coming across things you've already converted. For example, once you're done with the function GetAvailableScrollingDirectionsForUserInputEvents(), you'll find it comes up for many of the other uses of HORIZONTAL and VERTICAL, which will then only requires local changes to convert.

I am confused with that HORIZONTAL and nsIScrollableFrame::HORIZONTAL are different so they would have different replacements. So ScrollDirection::eHorizontal as replacement would be for HORIZONTAL or nsIScrollableFrame::HORIZONTAL and what would be for the other one.

They refer to the same thing. In the first case, the use is inside a method of nsIScrollableFrame, so name lookup searches the class scope of nsIScrollableFrame, and the member HORIZONTAL is found without explicit qualification.

Comment 10

[Nirmay Dhruv [:nirmay]]

(In reply to Botond Ballo [:botond] from comment #9)

I would suggest starting from the occurrences of HORIZONTAL and VERTICAL. For each one, change to the corresponding ScrollDirection, and then propagate the change to affected variables and functions.

Note that Searchfox supports semantic search. So, rather than doing a textual search for HORIZONTAL which turns up all textual occurrences of that word, many of which refer to other variables named HORIZONTAL, you can visit the definition of nsIScrollableFrame::HORIZONTAL, click on it, and select "Search for enum constant nsIScrollableFrame::HORIZONTAL" from the context menu. This will give you a list of uses only of nsIScrollableFrame::HORIZONTAL, and not other variables named HORIZONTAL.

Yes, I did search for specifically nsIScrollableFrame::HORIZONTAL after searching for HORIZONTAL and got 6 files where I would replace as mentioned. I read the rest of your instructions thoroughly

Let's go through an example. The first result of the mentioned semantic search is here.

• Observe that it's part of an expression directions & nsIScrollableFrame::HORIZONTAL. To convert this expression to use ScrollDirections:
  • The constant needs to change from nsIScrollableFrame::HORIZONTAL to ScrollDirection::eHorizontal.
  • The type of the directions variable needs to change from uint32_t to ScrollDirections.
  • The form of the expression needs to change from a & b to a.contains(b).
    • Other types of expression may have other equivalents in the EnumSet API. I encourage you to look through the EnumSet class to determine what these are, and ask here if you're unsure.

I did make the changes regarding this as mentioned. But will ask if the error is faced.

• The change of type to directions needs to be propagated to its other uses.
  • It's currently initialized to scrollableFrame->GetAvailableScrollingDirectionsForUserInputEvents(). We can use Searchfox to find the definition of this function here. The function's signature will need to change to return ScrollDirections instead of uint32_t.
    • This change to the function's signature will in turn require adjustments to the function's definition, and to its other callers (which can be found using Searchfox).
    • In this case, it's a virtual function, so overriding functions need a corresponding signature change. In this case, searching for the identifier shows there are two overriding functions here and here.
  • The remaining uses are also expressions of them form a & b which can be converted as above.

It may sometimes seem that the dependency chain goes very deep, but soon enough you'll start coming across things you've already converted. For example, once you're done with the function GetAvailableScrollingDirectionsForUserInputEvents(), you'll find it comes up for many of the other uses of HORIZONTAL and VERTICAL, which will then only requires local changes to convert.

Ok got it. Will surely ask if there would be any issue.

I am confused with that HORIZONTAL and nsIScrollableFrame::HORIZONTAL are different so they would have different replacements. So ScrollDirection::eHorizontal as replacement would be for HORIZONTAL or nsIScrollableFrame::HORIZONTAL and what would be for the other one.

They refer to the same thing. In the first case, the use is inside a method of nsIScrollableFrame, so name lookup searches the class scope of nsIScrollableFrame, and the member HORIZONTAL is found without explicit qualification.

Oh ok, I see it. I didn't see in the first case it was used inside a method of nsIScrollableFrame.
Thank you, it was really helpful.

Comment 11

[Nirmay Dhruv [:nirmay]]

Hello [:botond], I did try this bug but am doing something wrong, so to get to know what am I doing wrong can I submit a patch? Also, can I tag you as a reviewer or anyone else?

Comment 12

[Botond Ballo [:botond]]

Yes, please feel free to submit a patch and tag me for review. You can find instructions for submitting a patch here.

Comment 13

[Nirmay Dhruv [:nirmay]]

Okay, Thank you!

Comment 14

[Nirmay Dhruv [:nirmay]]

Attachment: Bug 1677643 - Replace nsIScrollableFrame::{HORIZONTAL, VERTICAL} with ScrollDirections. r?botond

Comment 15

[Nirmay Dhruv [:nirmay]]

Hello [:botond], in the file nsGfxScrollFrame.cpp here a uint32_t variable is initialized to '0' but when uint32_t is converted to ScrollDirections it can't be initialised with '0' else no viable conversion from 'int' to 'mozilla::layers::ScrollDirections' error is shown, what should I do about that?
Also as we replace a & b types with a.contains(b), what will I replace (|=) or (a | b types) operations as performed here with, as previously I was getting error no viable overloaded '|='?

Comment 16

[Botond Ballo [:botond]]

(In reply to Nirmay Dhruv [:nirmay] from comment #15)

Hello [:botond], in the file nsGfxScrollFrame.cpp here a uint32_t variable is initialized to '0' but when uint32_t is converted to ScrollDirections it can't be initialised with '0' else no viable conversion from 'int' to 'mozilla::layers::ScrollDirections' error is shown, what should I do about that?

You can leave it default-initialized, the EnumSet default constructor initializes the internally stored value to 0.

Also as we replace a & b types with a.contains(b), what will I replace (|=) or (a | b types) operations as performed here with, as previously I was getting error no viable overloaded '|='?

It looks like EnumSet uses +=.

Comment 17

[Nirmay Dhruv [:nirmay]]

Thank you!
I understood and have updated the patch. Can you review it?
It is having 2 errors and 2 warnings, rest is alright I guess. I tried figuring out a way to remove these but didn't work.

Comment 18

[Botond Ballo [:botond]]

Thanks for the update! Answered on Phabricator.

Comment 19

[Nirmay Dhruv [:nirmay]]

(In reply to Botond Ballo [:botond] from comment #18)

Thanks for the update! Answered on Phabricator.

Thank you for the corrections, I have made the changes and submitted them! :)

Comment 20

[Nirmay Dhruv [:nirmay]]

Hello [:botond],
I have made the changes you asked for. Let me know if there's anything else left to do :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/572a833231a1
Replace nsIScrollableFrame::{HORIZONTAL, VERTICAL} with ScrollDirections. r=botond

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/572a833231a1

Comment 23

[Botond Ballo [:botond]]

Thank you Nirmay for your contribution!

Comment 24

[Nirmay Dhruv [:nirmay]]

(In reply to Botond Ballo [:botond] from comment #23)

Thank you Nirmay for your contribution!

Thank you [:botond] for your guidance!