1116008 - 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 #1115327 +++

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

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

Comment 1

[Kushan Joshi [:kepta]]

Can I work on this ?

Comment 2

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

Absolutely! The first step is to get Firefox building locally and running. There are instructions on how to do this at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions. Once you have that up and running let me know and I can assign this bug to you and walk you through what needs to be done.

Comment 3

[Kushan Joshi [:kepta]]

I have run the first build Kartikaya, What should I do next ?

Comment 4

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

Great! The next step is to edit the file at gfx/layers/FrameMetrics.h in the source tree so that the mCriticalDisplayPort field is moved down into the private fields section of the file, and add public getter/setter methods to allow code to access and modify the field. See https://hg.mozilla.org/mozilla-central/diff/3f980229dfc1/gfx/layers/FrameMetrics.h for an example of how this was done for the mScrollableRect field.

You will then need to find all the places in the code that the mCriticalDisplayPort field is used, and update those to use the getter/setter methods instead. You can use the search tool at mxr.mozilla.org to find the various places the field is referenced, by doing an identifier search on the mozilla-central repository. It should give you results that look like this: http://mxr.mozilla.org/mozilla-central/ident?i=mCriticalDisplayPort&filter=. You'll have to go through each of those and if applicable, convert them to use the getter/setter as appropriate. You can also build the code with your changes periodically to let the compiler catch any errors or omissions.

Finally you'll need to put together a patch with your changes using the instructions at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and attach it to this bug, flagging me for review. This probably all sounds pretty complicated but after doing it once it'll be pretty easy. Feel free to ask here if you have any questions, or jump on IRC and ask there. #introduction is probably the best channel to ask questions in, or you can find me in #gfx during work hours in the EST time zone.

Comment 5

[Kushan Joshi [:kepta]]

I have a few doubts:

1. In the file GfxMessageUtils.h line 739 ,781, should I be implementing getter method there. And why?
2. In the file TiledContentClient.h line 315 , what should I do there?
3. In the file ClientTiledPaintedLayer.cpp line 115 , how do i set it empty?
please see my diff http://pastebin.mozilla.org/8144780.

Thanks

Comment 6

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

Excellent questions!

(In reply to 0o3ko0 from comment #5)
> I have a few doubts:
> 
> 1. In the file GfxMessageUtils.h line 739 ,781, should I be implementing
> getter method there. And why?

For the uses in GfxMessageUtils.h you don't need to use the getter method. The code in GfxMessageUtils is used by our IPC code and is declared as a "friend" of the FrameMetrics class at [1]. Therefore that code can directly access the private members and doesn't need to use the getter. Conceptually also it is better to use the private member directly there so that if the getter were later changed to return something else the IPC code would still send across a direct copy of the actual data stored in the class.

> 2. In the file TiledContentClient.h line 315 , what should I do there?

That is a different data structure which also has its own mCriticalDisplayPort member. You don't need to change that one. Similarly you will probably find pieces of code that reference the mCriticalDisplayPort member in BasicTiledLayerPaintData, and you don't need to change those either.

> 3. In the file ClientTiledPaintedLayer.cpp line 115 , how do i set it empty?

This is one of the places that is using the mCriticalDisplayPort member from BasicTiledLayerPaintData, so you don't need to make any changes to that.

> please see my diff http://pastebin.mozilla.org/8144780.

This is looking pretty good, but there's a few places where you changed things like "mPaintData.mCriticalDisplayPort" and these all refer to the BasicTiledLayerPaintData structure's mCriticalDisplayPort member. These do not need to be changed, and will probably give you a compiler error. It's only the FrameMetrics structure's mCriticalDisplayPort that needs to be changed.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=3f980229dfc1#32

Comment 7

[Kushan Joshi [:kepta]]

Thanks for quick replies.

I corrected my work, please have a look 
http://pastebin.mozilla.org/8144923

Comment 8

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

Thanks, please attach it as a patch to this bug. It's much easier for me to review that way. It doesn't matter if it's not complete, you can always upload a corrected/completed version afterwards.

Comment 9

[Kushan Joshi [:kepta]]

Attachment: bug.patch

Comment 10

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

Comment on attachment 8542206 [details] [diff] [review]
bug.patch

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

This looks pretty good. I just have a couple of comments below. If you can make the suggested changes and upload a new patch I'll look it over again and assuming all is well, get it landed. Thanks!

1. Please add a commit message to the patch. Usually this is of the form "Bug <n> - <description>. In this case something like "Bug 1116008 - Make FrameMetrics::mCriticalDisplayPort private." would be appropriate.

2. Since the order of member fields in FrameMetrics is changing, you will need to change the initializer list in FrameMetrics as well. The mCriticalDisplayPort initializer at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h?rev=3f980229dfc1#43 will need to move down a couple of lines so that it is after the mDisplayPort initializer. That way it will maintain the same order of fields (some of our platforms will fail to build if this isn't done).

::: gfx/layers/FrameMetrics.h
@@ +541,5 @@
> +  
> +  // ---------------------------------------------------------------------------
> +  // The following metrics are all in CSS pixels. They are not in any uniform
> +  // space, so each is explained separately.
> +  //

3. You can remove this comment as it's not needed any more.

Comment 11

[Kushan Joshi [:kepta]]

Attachment: bug1116008.patch

I made the changes you suggested. Please review.
Thanks :D

Comment 12

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

Comment on attachment 8542276 [details] [diff] [review]
bug1116008.patch

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

Looks pretty good. I push this patch to our "try" server to check if it builds across all platforms. Unfortunately it failed because of a typo, in the SetCriticalDisplayPort function you wrote "cont" instead of "const".

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a086017dcbf0

Other than that there's just one whitespace nit (see below). Sorry for the runaround but would you mind correcting these two issues and putting up a new patch? Thanks!

::: gfx/layers/FrameMetrics.h
@@ +283,5 @@
> +  CSSRect GetCriticalDisplayPort() const
> +  {
> +    return mCriticalDisplayPort;
> +  }
> +    

nit: there's some trailing whitespace on this line (it shows up in pink if you use the "review" link on the attachment) which should be removed.

Comment 13

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

Actually there's at least a couple more usages of mCriticalDisplayPort that need to be converted - there's one at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp?rev=a90e378dcfc6#155 and one at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp?rev=a90e378dcfc6#192 for example.

Were you able to build this patch successfully on your local machine? I would have thought that these two would have failed your local build (along with the const problem, but I thought that may have been a typo after you finished your build).

Comment 14

[Kushan Joshi [:kepta]]

Attachment: 3rd revision of the patch

I think I messed up my previous patch. I tried building this one and it works.

Comment 15

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

Comment on attachment 8542606 [details] [diff] [review]
3rd revision of the patch

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

Awesome, thank you! This looks great, I'll land it shortly.

Comment 16

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

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8d66893f5a

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

If you're interested in working on other bugs feel free to look through http://www.joshmatthews.net/bugsahoy/ to find something that interests you, or if you let me know what kind of bugs you're interested in I can help you find some.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b8d66893f5a