Transition more fields of FrameMetrics to use getters/setters

RESOLVED FIXED in mozilla36

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: Himanshu Singh, Mentored)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++] [good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

+++ 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

3 years ago
i would like to patch this bug .. please help me work on it as im new to all this!! ty
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

3 years ago
ive built firefox and tried running it successfully!! please guide me through the next step.
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
Assignee: nobody → abhirav.kariya

Comment 5

3 years ago
ok .. ill get started as soon as i can!

Comment 6

3 years ago
Created attachment 8480035 [details] [diff] [review]
done

Please check
Attachment #8480035 - Flags: review?(bugmail.mozilla)

Comment 7

3 years ago
Created attachment 8480036 [details] [diff] [review]
done
Attachment #8480035 - Attachment is obsolete: true
Attachment #8480035 - Flags: review?(bugmail.mozilla)
Attachment #8480036 - Flags: review?(bugmail.mozilla)

Comment 8

3 years ago
Created attachment 8480039 [details] [diff] [review]
done
Attachment #8480036 - Attachment is obsolete: true
Attachment #8480036 - Flags: review?(bugmail.mozilla)
Attachment #8480039 - Flags: review?(bugmail.mozilla)

Comment 9

3 years ago
please explain how to restore build/
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.
Attachment #8480039 - Flags: review?(bugmail.mozilla) → review-

Comment 11

3 years ago
i did not understand why it will not compile?
(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

3 years ago
I'm also interested to work in this and want to patch it
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

3 years ago
ok .. ill make the changes ... sorry i was inactive since 2 days
(Assignee)

Comment 16

3 years ago
what's the current status of this bug?
Hi Abhirav, are you still working on this bug?
Flags: needinfo?(abhirav.kariya)
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!
Assignee: abhirav.kariya → nobody
Flags: needinfo?(abhirav.kariya)
(Assignee)

Comment 19

3 years ago
I have build firefox and it runs.What to do next?
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.
Attachment #8480039 - Attachment is obsolete: true
Assignee: nobody → himanshusingh.singh06
(Assignee)

Comment 21

3 years ago
Created attachment 8509172 [details] [diff] [review]
1058614.patch
Attachment #8509172 - Flags: review?(bugmail.mozilla)
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?
Attachment #8509172 - Flags: review?(bugmail.mozilla) → feedback+
(Assignee)

Comment 23

3 years ago
it gives error while building.what should i do?
(Assignee)

Comment 24

3 years ago
I have to change the code in other files also using mMayHaveTouchCaret field?
(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.
(Assignee)

Comment 26

3 years ago
Created attachment 8509240 [details] [diff] [review]
1058614.patch
Attachment #8509172 - Attachment is obsolete: true
Attachment #8509240 - Flags: review?(bugmail.mozilla)
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
Attachment #8509240 - Flags: review?(bugmail.mozilla) → feedback+
(Assignee)

Comment 28

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

Comment 29

3 years ago
Created attachment 8509641 [details] [diff] [review]
1058614.patch
Attachment #8509240 - Attachment is obsolete: true
Attachment #8509641 - Flags: review?(bugmail.mozilla)
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.
Attachment #8509641 - Flags: review?(bugmail.mozilla) → review+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/2f41349285a9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.