1058614 - Transition more fields of FrameMetrics to use getters/setters

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

+++ This bug was initially created as a clone of Bug #1030741 +++

We are transitioning all fields of FrameMetrics to use getters/setter.

Anyone: feel free to pick one of the public fields in FrameMetrics, transition it, and post a patch here, flagging me for review. Thanks! See bug 1030741 for an example.

Comment 1

[Abhirav Kariya]

i would like to patch this bug .. please help me work on it as im new to all this!! ty

Comment 2

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

Great! As a first step you should download the source and get Firefox building using the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Once you have a working firefox build, let me know and I'll assign this bug to you. The change needed here is a straightforward one, where we pick one of the public fields in the file at gfx/layers/FrameMetrics.h and make it private. Then we will add getter/setter methods for it and update all users of the field to use the methods.

Comment 3

[Abhirav Kariya]

ive built firefox and tried running it successfully!! please guide me through the next step.

Comment 4

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

Awesome :)

So let's look at the mMayHaveTouchCaret field in FrameMetrics [1]. You should move that field down to the bottom of the class where the other private fields are defined, and add Get/Set methods for it similar to the ones at [2].

You can use our mxr search engine (or grep/your IDE of choice) to find all uses of mMayHaveTouchCaret and replace them to use the Get/Set methods. Then make sure you can still build and run Firefox successfully. Once you have done that, please create a patch with your changes using the instructions at [3] and attach it to this bug.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=1135f5842f1f#338
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=1135f5842f1f#447
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 5

[Abhirav Kariya]

ok .. ill get started as soon as i can!

Comment 6

[Abhirav Kariya]

Attachment: done

Please check

Comment 7

[Abhirav Kariya]

Attachment: done

Comment 8

[Abhirav Kariya]

Attachment: done

Comment 9

[Abhirav Kariya]

please explain how to restore build/

Comment 10

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

Comment on attachment 8480039 [details] [diff] [review]
done

Review of attachment 8480039 [details] [diff] [review]:
-----------------------------------------------------------------

After making the changes you need to rebuild firefox by running |./mach build| (assuming you used the instructions at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build#Building). This patch by itself will not compile properly because there are other places in the code that will need to be updated as well.

Comment 11

[Abhirav Kariya]

i did not understand why it will not compile?

Comment 12

[Botond Ballo [:botond]]

(In reply to Abhirav Kariya from comment #11)
> i did not understand why it will not compile?

A search on DXR for usages of FrameMetrics::mMayHaveTouchCaret [1] shows that, in addition to gfx/layers/FrameMetrics.h, it is used in gfx/ipc/GfxMessageUtils.h, gfx/layers/apz/src/AsyncPanZoomController.cpp, and layout/base/nsDisplayList.cpp.

The uses in GfxMessageUtils.h are OK because they are inside methods of ParamTraits<FrameMetrics>, which is declared as a friend of FrameMetrics [2], meaning it can access private members of FrameMetrics. However, the uses in AsyncPanZoomController.cpp and nsDisplayList.cpp need to be converted to use the getter/setter methods.

[1] http://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3Amozilla%3A%3Alayers%3A%3AFrameMetrics%3A%3AmMayHaveTouchCaret
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h#67

Comment 13

[Mohit Tirkey]

I'm also interested to work in this and want to patch it

Comment 14

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

Hi Mohit, in general we prefer if one person works on one bug at any given time. I would suggest you search for another bug that is mentored that you could work on. You can look at http://www.joshmatthews.net/bugsahoy/ to find bugs suitable to your area of interest and ability. Thanks!

Comment 15

[Abhirav Kariya]

ok .. ill make the changes ... sorry i was inactive since 2 days

Comment 16

[Himanshu Singh]

what's the current status of this bug?

Comment 17

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

Hi Abhirav, are you still working on this bug?

Comment 18

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

Unassigning from Abhirav since it doesn't look like he's still working on it.

Himanshu, if you're interested in working on this please see comment 2 for first steps and getting a build set up, and let me know once you have that done. Thanks!

Comment 19

[Himanshu Singh]

I have build firefox and it runs.What to do next?

Comment 20

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

Comment on attachment 8480039 [details] [diff] [review]
done

Ok, great! Please see comment 4 for the next step. Take a look at the mMayHaveTouchCaret field in FrameMetrics.h which is currently public. You should make it private and instead add public Get/Set methods to manipulate it. Then you also need to update all the places in the code that use mMayHaveTouchCaret to use the new functions.

Comment 21

[Himanshu Singh]

Attachment: 1058614.patch

Comment 22

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

Comment on attachment 8509172 [details] [diff] [review]
1058614.patch

Review of attachment 8509172 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, this is a great start, but there are other pieces of code that are using mMayHaveTouchCaret which also need to be modified to use the new get/set methods. In fact the patch as you have it now shouldn't compile successfully - did you try building Firefox with this patch?

Comment 23

[Himanshu Singh]

it gives error while building.what should i do?

Comment 24

[Himanshu Singh]

I have to change the code in other files also using mMayHaveTouchCaret field?

Comment 25

[Botond Ballo [:botond]]

(In reply to Himanshu Singh from comment #24)
> I have to change the code in other files also using mMayHaveTouchCaret field?

Yeah. I mention in comment 12 what some of these other files are.

Comment 26

[Himanshu Singh]

Attachment: 1058614.patch

Comment 27

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

Comment on attachment 8509240 [details] [diff] [review]
1058614.patch

Review of attachment 8509240 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking much better, thanks! I have one comment on the style below that needs to be fixed. Please upload a new patch with that addressed. Also, in the new patch, please include a proper commit message - generally our commit messages follow the syntax "Bug <number> - <message>. r=<reviewer>". So for this bug you'd have something like "Bug 1058614 - Transition mMayHaveTouchCaret to be private. r=kats" or something similar. With that done this should be ready to land.

::: gfx/layers/FrameMetrics.h
@@ +529,5 @@
>  
> +  bool GetMayHaveTouchCaret() const
> +  {
> +  	return mMayHaveTouchCaret;
> +  }

Please use two-space indents (spaces, not tab characters) in these functions.

@@ +540,5 @@
>  private:
>    // New fields from now on should be made private and old fields should
>    // be refactored to be private.
>  
> +  // Whether or not this frame may have touch caret.

may have *a* touch caret

Comment 28

[Himanshu Singh]

How to add commit message.Should i edit the .patch file and write in it or any mercurial command is there to use?

Comment 29

[Himanshu Singh]

Attachment: 1058614.patch

Comment 30

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

Comment on attachment 8509641 [details] [diff] [review]
1058614.patch

Review of attachment 8509641 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! Note that you don't need the quote marks around the commit message, but that's fine. I'll remove them when I land the patch.

Comment 31

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

Landed on b2g-inbound:

https://hg.mozilla.org/integration/b2g-inbound/rev/2f41349285a9

Once that gets merged to mozilla-central (usually about once a day) then this bug will be marked fixed automatically.

Comment 32

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

I also cloned this bug to bug 1087478 for transitioning additional fields. Himanshu, if you're interested doing another patch similar to this one to get more practice, feel free to take that bug. If you want to try for something a little more challenging you can look at http://www.joshmatthews.net/bugsahoy/ to find other mentored bugs.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f41349285a9