Closed Bug 1087478 Opened 5 years ago Closed 5 years ago

Transition more fields of FrameMetrics to use getters/setters

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kats, Assigned: himanshusingh.singh06, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

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

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

Anyone: feel free to pick one of the public fields in FrameMetrics (mMayHaveTouchListeners is an easy one), transition it, and post a patch here, flagging me for review. Thanks! See bug 1058614 for an example.
i want to work on this bug.
Great! It's very similar to the last one you worked on. I suggest transitioning the mMayHaveTouchListeners field. This time we'll leave it up to you to find all the places in the code the field is used - you can use local text-search tools or you can use our online code indexing tools at http://mxr.mozilla.org/mozilla-central/ or http://dxr.mozilla.org/mozilla-central/source/

Feel free to ask any questions here or on IRC.
Assignee: nobody → himanshusingh.singh06
Attached patch 1087478.patch (obsolete) — Splinter Review
Attachment #8509702 - Flags: review?(bugmail.mozilla)
Comment on attachment 8509702 [details] [diff] [review]
1087478.patch

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

Almost correct, just one little problem... please upload a new patch with the comment below addressed. Thanks!

Also, as you work on additional patches in the mozilla codebase you'll learn how to run some of the tests suites to exercise your code and make sure nothing broke. In this case, running the gtest code would have caught this error. You can run "mach gtest" (without the quotes) to run this code. It would be a good exercise for you to take this patch as-is, run the gtests, and observe the test failures. Then correct the patch and rebuild firefox, and re-run the tests and observe them passing. (This is optional, since I've already done it, but highly recommended if you care to learn more about our code development process).

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

This is a copy-paste error
Attachment #8509702 - Flags: review?(bugmail.mozilla) → review-
Attached patch 1087478.patchSplinter Review
Attachment #8509702 - Attachment is obsolete: true
Attachment #8510009 - Flags: review?(bugmail.mozilla)
Comment on attachment 8510009 [details] [diff] [review]
1087478.patch

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

Perfect, thanks!
Attachment #8510009 - Flags: review?(bugmail.mozilla) → review+
Although there are more transitions to be done I'm going to hold off on filing another bug for it until bug 1076163 is landed, because that changes a lot of code in FrameMetrics and is going to be annoying to rebase if we keep changing fields from under it.

Himanshu, if you're interested in working on additional bugs I recommend taking a look at http://www.joshmatthews.net/bugsahoy/ to find some, or if you want I can try to look for a couple that might be suitable for you. Let me know!
Please Kartikaya if you find any bug let me know.And thanks for help
Himanshu, bug 1020184 looks like a good one (somebody seems to have started with it but hasn't touched it in months, so you should be able to pick it up). I can also CC you when I file a new bug for more FrameMetrics field transitions.
https://hg.mozilla.org/mozilla-central/rev/126479bbdc25
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.