Closed Bug 1101620 Opened 7 years ago Closed 7 years ago

Transition more fields of FrameMetrics to use getters/setters

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: kats, Assigned: psd, Mentored)

Details

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

Attachments

(2 files)

+++ 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 (mDevPixelsPerCSSPixel is a good one to take), transition it, and post a patch here, flagging me for review. Thanks! See bug 1058614 for an example.
Hi @Kartikaya
I would like to work on this bug, could you please assign it to me 
and help me out?

Thanks
Hi gokul,

The first step would be to get a build of Firefox up and running. You can find instructions to do this at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions. Once you have a local build of Firefox working let me know and I can assign this bug to you and provide further guidance.

Thanks!
Hi Karitkaya,
I have done all that :)
I have a local build of firefox nightly
Great! Ok, so the thing you need to change is in gfx/layers/FrameMetrics.h. Take the field at [1] and move it down to the top of the private section at [2]. Then, add getter/setter methods for the field like the ones just above [2]. Finally, you will need to find all uses of that field and modify them so that they use the getter/setter functions. You can use the mxr tool for this (or your local IDE/text search if you want). [3] is a search that identifies most if not all of the places this field is used. Once you do that you will need to make sure that you can still build Firefox with your changes. Finally, attach a patch with your changes to this bug, using the instructions at [4].

Feel free to post questions here if you run into trouble, or ask on our IRC server (#gfx or #introduction would be appropriate channels).

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=50952288e4a4#315
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=50952288e4a4#529
[3] http://mxr.mozilla.org/mozilla-central/ident?i=mDevPixelsPerCSSPixel
[4] https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Assignee: nobody → gokul.uf
I'm on it.

BTW I don't have to change 
gfx/layers/FrameMetrics.h 
   LHS of line 86 -- mDevPixelsPerCSSPixel == aOther.mDevPixelsPerCSSPixel &&

    line 139 -- return mCumulativeResolution * mDevPixelsPerCSSPixel;

    line 319 -- CSSToLayoutDeviceScale mDevPixelsPerCSSPixel; 
because they are all within the class.
Is this right?
Correct. You also don't need to touch the ones in gfx/ipc/GfxMessageUtils.h as that class is declared as a friend of FrameMetrics.
Hi,
I'm working on it now. Some of my changes caused the build to break.
Can you help me with how to read the mach build 's output?
Sure. Most of the output is just the compiler output, but since it builds multiple source files in parallel the output might be interleaved which can make it look confusing. I find the easiest thing to do us just look for occurrences of the string "error:" as all the actual compiler errors will be prefixed with this. You may need to redirect the mach output to a file and then search in the file to get everything, if you find that your terminal scrollback is not big enough to keep all the output.

If you are having trouble figuring out how to fix a specific error, feel free to paste the changes you have so far and i can suggest something.
Hi,
here's the output of mach build

http://pastebin.com/aKxuWv59

Thanks
The most common error in that file is about const-correctness, you need to make the getter method const.
Hey Gokul, are you still working on this?
Flags: needinfo?(gokul.uf)
Hi,
yes, I am working on it but I'm not able to devote enough time.
I'll done by tomorrow night, is it okay with you?
Flags: needinfo?(gokul.uf)
Sure, no problem!
Hi,
I've done the following changes in gfx/layers/FrameMetrics.h

http://pastebin.com/wwd73N5X

The getter function gives me errors which break the build.
Must've done something really stupid

Relevant mach output snippet
http://pastebin.com/duchGXAk
As I mentioned in comment 6 you shouldn't need to change the code in the GfxMessageUtils file. So calling the getter method is not necessary (and calling it as part of the ReadParam, which expects a reference, is what causes the compiler error).
what's the current status of this bug?
If no one is working on it then I would love to. Thanks :)
Absolutely, it doesn't look like gokul is still working on it. Do you have a local build of Firefox up and running? Please see comment 2 for instructions on how to get that set up, and let me know once you have that done. Then I can assign the bug to you and you can get started with it.
Hi Kartikaya,
I have the build up and running.
Great! I'm assigning the bug to you. The instructions for how to tackle this bug are in comment 4, so take a look through there and let me know if you have any questions!
Assignee: gokul.uf → prabhjyotsingh95
I have made some changes and have introduced getters and setters.
I am getting the following build error :

http://pastebin.com/CuqeSx3r

I get the error in the modified file : mozilla-central/gfx/layers/apz/src/AsyncPanZoomController.cpp
http://pastebin.com/rggYtUFq
The build error you are getting is this:

10:08.53 c:\mozilla-source\mozilla-central\gfx\layers\apz/src/AsyncPanZoomController.cpp(2777) : error C2106: '=' : left operand must be l-value

There is an error on line 2777 of AsyncPanZoomController.cpp. Have you C++ programming before?
Yes I have, 
In the file : AsyncPanZoomController.cpp
I have tried to change :
I have tried to change 
mFrameMetrics.mDevPixelsPerCSSPixel.scale = aLayerMetrics.mDevPixelsPerCSSPixel.scale; // original code

to 

mFrameMetrics.GetMDevPixelsPerCSSPixel().scale = aLayerMetrics.GetMDevPixelsPerCSSPixel().scale;  // after introducing getters and setters 

This maybe a problem if the scale variable is declared const, am I right ? 
or is this problem because of the getter function of mDevPixelsPerCSSPixel which I have declared to be returning const value?
Any suggestions ?
(In reply to prabhjyotsingh95 from comment #24)
> I have tried to change 
> mFrameMetrics.mDevPixelsPerCSSPixel.scale =
> aLayerMetrics.mDevPixelsPerCSSPixel.scale; // original code
> 
> to 
> 
> mFrameMetrics.GetMDevPixelsPerCSSPixel().scale =
> aLayerMetrics.GetMDevPixelsPerCSSPixel().scale;  // after introducing
> getters and setters 
> 
> This maybe a problem if the scale variable is declared const, am I right ? 
> or is this problem because of the getter function of mDevPixelsPerCSSPixel
> which I have declared to be returning const value?

Yeah, you can't change a field accessed through the getter. You need to use the setter to set a new value instead.

In this case, 'scale' is the only field of ScaleFactor, so the original code might more simply have assigned the whole structure:

  mFrameMetrics.mDevPixelsPerCSSPixel = aLayerMetrics.mDevPixelsPerCSSPixel;

which with getter/setters becomes:

  mFrameMetrics.SetDevPixelsPerCSSPixel(aLayerMetrics.GetDevPixelsPerCSSPixel());
(In reply to Botond Ballo [:botond] from comment #25)
> (In reply to prabhjyotsingh95 from comment #24)
> > I have tried to change 
> > mFrameMetrics.mDevPixelsPerCSSPixel.scale =
> > aLayerMetrics.mDevPixelsPerCSSPixel.scale; // original code
> > 
> > to 
> > 
> > mFrameMetrics.GetMDevPixelsPerCSSPixel().scale =
> > aLayerMetrics.GetMDevPixelsPerCSSPixel().scale;  // after introducing
> > getters and setters 
> > 
> > This maybe a problem if the scale variable is declared const, am I right ? 
> > or is this problem because of the getter function of mDevPixelsPerCSSPixel
> > which I have declared to be returning const value?
> 
> Yeah, you can't change a field accessed through the getter. You need to use
> the setter to set a new value instead.
> 
> In this case, 'scale' is the only field of ScaleFactor, so the original code
> might more simply have assigned the whole structure:
> 
>   mFrameMetrics.mDevPixelsPerCSSPixel = aLayerMetrics.mDevPixelsPerCSSPixel;
> 
> which with getter/setters becomes:
> 
>  
> mFrameMetrics.SetDevPixelsPerCSSPixel(aLayerMetrics.
> GetDevPixelsPerCSSPixel());

Thanks a lot. I have made the changes. waiting for the build :)
Now the build is running again.
I have changed all the files using the variable mDevPixelsPerCSSPixel using getters and setter.
Now, I make the patch right ?
Attached patch 1101620.patchSplinter Review
Comment on attachment 8537629 [details] [diff] [review]
1101620.patch

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

Thanks, this patch looks great! Looks like you got all the use sites of the variable. There's just a few small issues to correct that I've noted below - if you can make those changes and upload an updated version of the patch we should be able to get it landed. Also kudos on the commit message you added to the patch - most first-time contributors either don't have a commit message or just restate the bug title but you have a good commit message that's formatted correctly as well. Awesome!

::: gfx/layers/FrameMetrics.h
@@ +308,5 @@
>    // containing this scroll frame and its ancestors, and any css-driven
>    // resolution. This information is provided by Gecko at layout/paint time.
>    LayoutDeviceToLayerScale mCumulativeResolution;
>  
> +  

Please remove the whitespace on this line

@@ +313,4 @@
>  
>  public:
> +
> +  void SetMDevPixelsPerCSSPixel(CSSToLayoutDeviceScale aMDevPixelsPerCSSPixel)

This function should be called SetDevPixelsPerCSSPixel (no "M" in the name). The "m" prefix on member variables is simply to indicate that it is a member variable, and logically is not part of the name. So the function shouldn't have the M in it. The getter method likewise should not have the M.

Also, please change the parameter to this function to be a "const CSSToLayoutDeviceScale&" rather than just CSSToLayoutDeviceScale for consistency with the other similar functions.
Attachment #8537629 - Flags: feedback+
Attached patch 1101620.patchSplinter Review
I have made the changes :)
Comment on attachment 8537792 [details] [diff] [review]
1101620.patch

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

Looks perfect, thanks! I'll land this into our codebase shortly. For future reference, after you attach a patch you can flag it for review by going into the patch details and change the "review" option to "?" and putting the reviewer in the textbox that shows up. Doing that is the step that indicates you think the patch is ready to be reviewed.
Attachment #8537792 - Flags: review+
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/56f10b2aa4a8

Once this gets merged to mozilla-central (usually about once a day) this bug will be marked fixed. Thank you for the patch!

I'll file a follow-up bug shortly for converting another field in FrameMetrics, so if you're interested in doing another one you can take that, or if you want to find something more challenging the bugs ahoy website [1] is a great place to look for other mentored bugs you can work on.

[1] http://www.joshmatthews.net/bugsahoy/
https://hg.mozilla.org/mozilla-central/rev/56f10b2aa4a8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Great. Thanks a lot for your help. :)
You need to log in before you can comment on or make changes to this bug.