1730615 - Change TestWRScrollData::Create() to use LayerIntRegion instead of nsIntRegion

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P3)

Description

[Botond Ballo [:botond]]

During the review of https://phabricator.services.mozilla.com/D124288, Hiro suggested that TestWRScrollData::Create() and its callers should use the strongly-typed LayerIntRegion instead of the untyped nsIntRegion.

This bug tracks making that change as a follow-up.

Comment 1

[Botond Ballo [:botond]]

I'm going to make this a mentored bug.

Comment 2

[vishal]

Hello, Sir Can I be the assignee for this bug?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Sure, thanks!

Comment 4

[vishal]

Hello, I am searching the certain path. But unable to find the file "gfx/layers/apz/test/gtest/TestWRScrollData.cpp"

Comment 5

[Hiroyuki Ikezoe (:hiro)]

There should be.
https://searchfox.org/mozilla-central/source/gfx/layers/apz/test/gtest/TestWRScrollData.cpp

I guess you didn't clone the source repo properly? If you did "hg clone https://hg.mozilla.org/mozilla-central/" then the path should be there.

Comment 6

[vishal]

Attachment: Bug 1730615 change nsIntRegion to LayerIntRegion r?botond

Comment 7

[vishal]

Please review

Comment 8

[Botond Ballo [:botond]]

(In reply to vishal from comment #2)

Can I be the assignee for this bug?

I've actually had someone reach out to me over chat a few days ago, and helped them get started on this bug. My bad for failing to note that with a comment on the bug.

I've had a look at your patch, and it's on the right track, but there is more work to be done. The function's call sites will need to be updated, as well as the function's implementation where it uses the parameters.

Generally speaking, the first thing to do before starting on a bug is to build Firefox, and then make sure it continues to build successfully with your changes.

Vishal, given that someone else has started to work on this and your patch is at an early stage, would you be OK with choosing a different bug to work on? There are lots to choose from, you can search using https://codetribute.mozilla.org/.

Comment 9

[vishal]

Actually I build firefox. It is using artifacts. How can I build it from source code?
Thanks

Comment 10

[Botond Ballo [:botond]]

(In reply to vishal from comment #9)

Actually I build firefox. It is using artifacts. How can I build it from source code?

There is a step during the bootstrap process, where you are given this prompt:

Please choose the version of Firefox you want to build:
  1. Firefox for Desktop Artifact Mode [default]
  2. Firefox for Desktop
  3. GeckoView/Firefox for Android Artifact Mode
  4. GeckoView/Firefox for Android

Choose option 2 there.

Comment 11

[vishal]

Attachment: WIP: Bug 1730615 Revert the changes

Depends on D129854

Comment 12

[Botond Ballo [:botond]]

(In reply to vishal from comment #11)

Created attachment 9248429 [details]
WIP: Bug 1730615 Revert the changes

There is no need to revert any changes, since no changes were merged in the first place.

Comment 13

[Joe Peterson]

Hey Vishal, I'm who botond referred to having reached out to him about the task. I guess it got assigned to you though. I'll go ahead and find another task. Lesson learned to comment on the board to get assigned :)

Comment 14

[Botond Ballo [:botond]]

It's my bad, I should have kept the bug more up to date.

Vishal, based on comment 10, you're welcome to continue working on this, if you'd like. Let me know if you're interested!

Comment 15

[vishal]

Ok,
But, I am facing some problems in managing the source code. I don't have knowledge about how to handle a patch like delete, update, merge with another. Please refer me something which can help me to handle patch.
Thanks

Comment 16

[Botond Ballo [:botond]]

(In reply to vishal from comment #15)

But, I am facing some problems in managing the source code. I don't have knowledge about how to handle a patch like delete, update, merge with another. Please refer me something which can help me to handle patch.

Some useful documentation pages are:

Contributor quick reference
Phabricator user guide (for using the Phabricator code review system)
Mercurial docs (if you're using Mercurial (hg) for version control; ignore this if you're using Git)

I'm happy to try and help in more detail, if you let me know:

• which version control system you are using (hg or git)
• what is your current state (output of hg status or git status, and first few entries in hg log or git log are helpful)
• what you're trying to do

You can also ask your question on Matrix, in the #apz room, you might get a faster answer there.

Comment 17

[vishal]

Can I make some changes in this bug?

Comment 18

[Botond Ballo [:botond]]

(In reply to vishal from comment #17)

Can I make some changes in this bug?

I assume you mean, make some changes to the patch attached to this bug? Yes, if you don't already have the patch applied locally you can pull it down using moz-phab patch D129803 --apply-to here, then make changes, amend them into the patch, and resubmit. Please remember to build your changes using ./mach build.

Comment 19

[Botond Ballo [:botond]]

Hey Vishal, just wanted to check in and see how this is going. If you need some help working through compiler errors, please don't hesitate to ask!

Comment 20

[Botond Ballo [:botond]]

Hi Vishal, could you let us know if you're planning to continue working on this?

Comment 21

[Botond Ballo [:botond]]

As we haven't heard from Vishal in a while, I'm going to return this bug to the pool of available bugs for interested contributors to take on.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 23

[Razvan Cojocaru]

Attachment: Bug 1730615 - Change TestWRScrollData::Create() to use LayerIntRegion instead of nsIntRegion. r?botond

TestWRScrollData::Create() and its callers now use the
strongly-typed LayerIntRegion instead of the untyped nsIntRegion.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b01e2e315393
Change TestWRScrollData::Create() to use LayerIntRegion instead of nsIntRegion. r=botond

Comment 25

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b01e2e315393

Comment 26

[Botond Ballo [:botond]]

Thank you for your contribution here, Razvan! Let me know if you're interested in suggestions for other mentored bugs to work on.

Comment 27

[Razvan Cojocaru]

No problem! I'm waiting for the dust to settle on bug 1142667 right now, and then at some point I'll probably attack bug 1676299 (which you appear to be involved in).

Of course if you have other suggestions they're welcome.

Comment 28

[Razvan Cojocaru]

Oh, sorry, I see somebody's just been assigned bug 1676299 5 hours ago. Nevermind. :)

Comment 29

[Botond Ballo [:botond]]

Yeah Dan beat you to that particular one, but here is a list of other bugs I'm mentoring, please feel free to take any one of them that's not already assigned.

Comment 30

[Razvan Cojocaru]

Thanks, will take a look and see where I can help.