Transition more fields of FrameMetrics to use getters/setters

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: psd, Mentored)

Tracking

Trunk
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

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

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

Anyone: feel free to pick one of the public fields in FrameMetrics (mCumulativeResolution is a good one to take), transition it, and post a patch here, flagging me for review. Thanks! See bug 1058614 for an example.
(Assignee)

Comment 1

4 years ago
I'll start working on it. :)
Assignee: nobody → prabhjyotsingh95
(Assignee)

Comment 2

4 years ago
Created attachment 8538714 [details] [diff] [review]
1112830.patch
Attachment #8538714 - Flags: review?(bugmail.mozilla)
Comment on attachment 8538714 [details] [diff] [review]
1112830.patch

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

Nice work, this looks good!

::: gfx/layers/FrameMetrics.h
@@ +307,5 @@
> +  void SetCumulativeResolution(const LayoutDeviceToLayerScale& aCumulativeResolution)
> +  {
> +    mCumulativeResolution = aCumulativeResolution;
> +  }
> +  

small nit: there's some extra whitespace on this line. This usually isn't worth uploading a new patch for so I'll just fix it when I land the patch.
Attachment #8538714 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 5

4 years ago
There must be one line gap between two function definition right ? 
Is there a quick-guide to mozilla's coding standards?
Thanks a lot again :)
(In reply to Prabhjyot Sodhi from comment #5)
> Is there a quick-guide to mozilla's coding standards?

Yep, here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
(In reply to Prabhjyot Sodhi from comment #5)
> There must be one line gap between two function definition right ? 

Yeah, the extra line was fine, but the line had a couple of space characters which it shouldn't have.

And thank *you* for the patch!
(Assignee)

Comment 8

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) 
> Yeah, the extra line was fine, but the line had a couple of space characters
> which it shouldn't have.
> 
> And thank *you* for the patch!
Alrighty :)
Do you suggest any other unresolved bug for me to try ?
I filed bug 1113774 for the next one in this series, feel free to start working on that. If you're looking for something else that's more challenging let me know and I can try to find something. Most of the code I work on is specific to Firefox OS and Firefox for Android so you would need a phone that can run that to work on those bugs. But I can definitely help you find some desktop bugs if that's what you're interested in.
https://hg.mozilla.org/mozilla-central/rev/4b6379ea8da0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 11

4 years ago
I have no problem working for the bug 1113774, but I want to leave it to some other new contributors. It is a great first bug, provides the feel of open source immediately.
Still if you don't find any one interested in , say a couple of days, feel free to ask me to do it or directly just assign it to me :)
I have coded in C/C++, java before and I have recently started learning python. How should I proceed further, What would you suggest?
btw, I have an android phone running Kitkat and another old phone running gingerbread.
Great! If you know java and have an android phone then there's a bunch of bugs in Firefox for android that would be good to take. Bug 721421 is relatively straightforward one if that sounds interesting to you. Bug 1106905 is also a good one. Do either of those sound interesting to you?

To get started with either of them you will need to set up the build for Firefox for android which is a little more involved than for desktop Firefox. There are instructions on how to do that at https://wiki.mozilla.org/Mobile/Fennec/Android. The only thing I'm worried about is that IIRC you are working on windows and so would need to set up a Linux VM to build. You can use virtual box for free but if you have a slow computer this might be painful. Let me know if you want to try or if i should look for some other desktop bugs for you.
(Assignee)

Comment 13

4 years ago
I will try bug 721421. I will try and get the build running on a virtual machine.
I use vmware. That won't be a problem, will it?
VMware should work just fine. Let me know once you have the build working and i will assign the bug to you. Thanks!
(Assignee)

Comment 15

4 years ago
Hey Kats, 
Could you please help me with the mozconfig file.
I am not sure how should I go about it
The mozconfig file is just a plain-text file that you need to create in your src folder with the specified contents. You can use any text editor (emacs, vim, nano, etc) to create this file. Are you having trouble creating the file or with what to put inside it?
(Assignee)

Comment 17

4 years ago
I am not sure what to put inside.
I am facing a problem with the --with-android = path 
the terminal output on running |mach build|  points out to that error.
Can you, maybe , attach a sample mozconfig file.
I believe, that will help me understand better.

Thanks :)
(Assignee)

Comment 18

4 years ago
I initially wrote:

--with-android = "~/android-ndk-r8e"

Is this correct ?
(Assignee)

Comment 19

4 years ago
This is the terminal output on trying to build:
https://pastebin.mozilla.org/8099643

and this is my .mozconfig file :
https://pastebin.mozilla.org/8099644
In general you should use $HOME rather than ~ because ~ is a shell expansion and it won't get expanded properly in the context of a mozconfig. Here's an example mozconfig that I use on linux: https://github.com/staktrace/moz-scripts/blob/master/mozconfig.Linux-android-debug
(Assignee)

Comment 21

4 years ago
Hey!
I finally have the build running :)
You need to log in before you can comment on or make changes to this bug.