1112830 - 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 #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.

Comment 1

[Prabhjyot Sodhi [:psd]]

I'll start working on it. :)

Comment 2

[Prabhjyot Sodhi [:psd]]

Attachment: 1112830.patch

Comment 3

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

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.

Comment 4

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

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6379ea8da0

Comment 5

[Prabhjyot Sodhi [:psd]]

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 :)

Comment 6

[Botond Ballo [:botond]]

(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

Comment 7

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

(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!

Comment 8

[Prabhjyot Sodhi [:psd]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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 ?

Comment 9

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

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b6379ea8da0

Comment 11

[Prabhjyot Sodhi [:psd]]

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.

Comment 12

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

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.

Comment 13

[Prabhjyot Sodhi [:psd]]

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?

Comment 14

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

VMware should work just fine. Let me know once you have the build working and i will assign the bug to you. Thanks!

Comment 15

[Prabhjyot Sodhi [:psd]]

Hey Kats, 
Could you please help me with the mozconfig file.
I am not sure how should I go about it

Comment 16

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

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?

Comment 17

[Prabhjyot Sodhi [:psd]]

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 :)

Comment 18

[Prabhjyot Sodhi [:psd]]

I initially wrote:

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

Is this correct ?

Comment 19

[Prabhjyot Sodhi [:psd]]

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

Comment 20

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

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

Comment 21

[Prabhjyot Sodhi [:psd]]

Hey!
I finally have the build running :)