1001582 - Transition remaining fields of FrameMetrics to use getters/setters (mozilla32/mozilla33 edition)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

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

Bug 980493 used to track this work, but it had patches spanning different trains, and for organizational reasons it's better to have all the patches in a bug land in a single train.

So, I'm opening this bug to track all the fields that are transitioned in the 32 train.

Anyone: feel free to pick one or more fields, transition them, and post a patch here, flagging me for review. Thanks!

Comment 1

[Debbie Heisler]

Attachment: mPresShellId from public to private patch

Bug 1001582 -- Transitioned mPresShellId from public to private member, wrote GetPresShellId and SetPresShellId, and replaced setter/getter where the public member was used (did not modify friends of the struct).


I ran reftest on this patch with my changes and without.  It failed and succeeded the same number of times for both.  A cursory look at the failures looked like the same failures.

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8424083 [details] [diff] [review]
mPresShellId from public to private patch

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

Looks good! There's just one small mistake - if you could upload a new patch with that fixed, that would be great.

Thanks for helping out with this!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +68,5 @@
>    APZC_LOG(prefix ":" \
>             " i=(%ld %lld) cb=(%d %d %d %d) rcs=(%.3f %.3f) dp=(%.3f %.3f %.3f %.3f) dpm=(%.3f %.3f %.3f %.3f) um=%d " \
>             "v=(%.3f %.3f %.3f %.3f) s=(%.3f %.3f) sr=(%.3f %.3f %.3f %.3f) z(ld=%.3f r=%.3f cr=%.3f z=%.3f ts=%.3f) u=(%d %lu)\n", \
>             __VA_ARGS__, \
> +           fm.GetPresShellId, fm.GetScrollId(), \

You're missing the parentheses here.

Comment 3

[Debbie Heisler]

Attachment: updated mPresShellId patch from public to private member

Bug 1001582 -- Transitioned mPresShellId from public to private member, wrote GetPresShellId and SetPresShellId, and replaced setter/getter where the public member was used (did not modify friends of the struct).

I ran reftest on this patch with my changes and without.  It failed and succeeded the same number of times for both.  A cursory look at the failures looked like the same failures.

Fixed missing parenthesis in macro.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8424161 [details] [diff] [review]
updated mPresShellId patch from public to private member

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

Thanks!

Comment 5

[Botond Ballo [:botond]]

I did a build-only Try push for this patch to be sure we didn't miss any uses: https://tbpl.mozilla.org/?tree=Try&rev=fcb5ca8f7482

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> I did a build-only Try push for this patch to be sure we didn't miss any
> uses: https://tbpl.mozilla.org/?tree=Try&rev=fcb5ca8f7482

All green! Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/89964e80a627

Debbie, thanks again for your patch! If you'd like to transition another field, or work on another bug, please feel free (let me know if you'd like me to recommend a next bug).

Comment 7

[Botond Ballo [:botond]]

Marking as leave-open so we can reuse this bug for other fields that are transitioned in the 32 train.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/89964e80a627

Comment 9

[Emma Benoit]

Attachment: HasScrollgrab transitioned from private to public member, getter/setter added

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8446122 [details] [diff] [review]
HasScrollgrab transitioned from private to public member, getter/setter added

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

Thanks for the patch! Redirecting review to Kats as I'm on vacation right now.

Comment 11

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

Comment on attachment 8446122 [details] [diff] [review]
HasScrollgrab transitioned from private to public member, getter/setter added

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

Looks good, thanks! I can land this patch for you once the tree reopens.

Comment 12

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

Comment on attachment 8424161 [details] [diff] [review]
updated mPresShellId patch from public to private member

This patch has already landed, flagging it as such.

Comment 13

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

And since trunk is now mozilla33 we should close this bug after this patch lands.

Comment 14

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

Landed on b2g-inbound since I'm not sure when inbound will reopen.

https://hg.mozilla.org/integration/b2g-inbound/rev/d761481accd4

Comment 15

[Emma Benoit]

@kats:
Thanks for landing it! If I want to make more patches for this bug, should I file a new bug for the mozilla33 edition ? And is it better to do 1 patch per field or a mammoth patch ?

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d761481accd4

Comment 17

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

(In reply to Emma Benoit from comment #15)
> @kats:
> Thanks for landing it! If I want to make more patches for this bug, should I
> file a new bug for the mozilla33 edition ? And is it better to do 1 patch
> per field or a mammoth patch ?

I filed bug 1030741 as the next bug in this transition series and assigned it to you. Feel free to convert more fields - please do 1 patch per field, but convert as many as you like. Attach them all to the bug and we can land them all together. If you don't want to convert all of them, that's perfectly fine and I'll file another follow-up bug to do the rest after that bug has landed. Sound good?