980493 - Transition other fields of FrameMetrics to use getters/setters

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

My immediate motivation for this is to make debugging easier, since you can set a single breakpoint (or add a single print statement) every time a particular field is set.

This also maintains flexibility for the longer term (if we later want to do something nontrivial in a getter/setter).

Comment 1

[Botond Ballo [:botond]]

Attachment: Transition FrameMetrics::mScrollOffset to use a getter/setter

Comment 2

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

Comment on attachment 8389848 [details] [diff] [review]
Transition FrameMetrics::mScrollOffset to use a getter/setter

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

Seems pretty mechanical; rubber-stamping.

Comment 3

[Botond Ballo [:botond]]

Attachment: Transition FrameMetrics::mZoom to use a getter/setter

Note that in addition to the mechanical changes, I've added FrameMetrics::ZoomBy(float). The motivation for this was that I thought it unfortunate to have to rewrite the fairly common

  metrics.mZoom.scale *= someFloat;

as

  metrics.SetZoom(CSSToScreenScale(metrics.GetZoom().scale * someFloat));

Now instead we cam write

  metrics.ZoomBy(someFloat);

Note that the Part 1 patch rewrites

  metrics.mScrollOffset += somePoint;

as

  metrics.SetScrollOffset(metrics.GetScrollOffset() + somePoint);

If you think we should add a ScrollBy() to make this a bit nicer, I can change that.

Comment 4

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

(In reply to Botond Ballo [:botond] from comment #3)
> If you think we should add a ScrollBy() to make this a bit nicer, I can
> change that.

It would be a little nicer but I don't care too much either way - your call.

Comment 5

[Botond Ballo [:botond]]

Attachment: Transition FrameMetrics::mScrollOffset to use a getter/setter

Updated Part 1 patch to add FrameMetrics::ScrollBy(). Carrying r+.

Try push for the two patches together: https://tbpl.mozilla.org/?tree=Try&rev=66d94f15848c

Comment 6

[Botond Ballo [:botond]]

Attachment: Transition FrameMetrics::mScrollOffset to use a getter/setter

Missed some uses of mScrollOffset in android-only code. Updated patch to fix these, carrying r+.

ReTrying for android only: https://tbpl.mozilla.org/?tree=Try&rev=5c0f6c8be526

Comment 7

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/509090e51f4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/27737607943d

Comment 8

[Botond Ballo [:botond]]

Marking 'leave-open' as we will likely want to transition other fields as well.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/509090e51f4c
https://hg.mozilla.org/mozilla-central/rev/27737607943d

Comment 10

[Botond Ballo [:botond]]

There is no rush for transitioning the other fields, but we should do it at some point. Sounds like a good candidate for a mentored bug, and simple enough to be a first bug.

Comment 11

[Botond Ballo [:botond]]

Attachment: Update uses of mScrollOffset and mZoom in APZC_LOG calls

I missed the uses of mScrollOffset and mZoom in APZC_LOG calls, which are only compiled when APZC_LOG is defined to do something.

Comment 12

[Chadd Williams]

Attachment: This is a patch to transition FrameMetrics::mScrollId to use a getter/setter

Transition FrameMetrics::mScrollId to use a getter/setter

Comment 13

[Botond Ballo [:botond]]

Landed the patch from comment 11: https://hg.mozilla.org/integration/mozilla-inbound/rev/b10649cebf55.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8394364 [details] [diff] [review]
This is a patch to transition FrameMetrics::mScrollId to use a getter/setter

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

Thanks for this patch!

I have one small comment about the parameter type of SetScrollId().

::: gfx/layers/FrameMetrics.h
@@ +371,5 @@
> +  {
> +    return mScrollId;
> +  }
> +  
> +  void SetScrollId(const ViewID scrollId) 

There is not much point in making a parameter passed by value 'const'. Generally an input parameter should be either a const reference, or a (non-const) value. In this case, since ViewID is just a typdef for an integer type, we might as well just pass it by (non-const) value.

Comment 16

[Botond Ballo [:botond]]

I pushed your patch, with the suggested change, to the Try server: https://tbpl.mozilla.org/?tree=Try&rev=a14b7403e2e0

Also, let's ask Kats whether we should land this now, or wait until the end of the 31 cycle to avoid annoying people trying to uplift to 30 (including b2g 1.4, which is probably expecting a few APZ-related uplifts).

Comment 17

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

I'm fine with landing it now; it'll probably bitrot otherwise.

Comment 18

[Chadd Williams]

Attachment: This is a patch to transition FrameMetrics::mScrollId to use a getter/setter

This fixes the const parameter mentioned in the review above.

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8394992 [details] [diff] [review]
This is a patch to transition FrameMetrics::mScrollId to use a getter/setter

Thanks!

Comment 20

[Botond Ballo [:botond]]

The Try run is clean.

We prefer having full names in the "user" field of a commit - could you fill out your name in your Bugzilla profile (and use it in the "user" field of future patches)? I'll then commit the patch for you. Thanks!

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> The Try run is clean.
> 
> We prefer having full names in the "user" field of a commit - could you fill
> out your name in your Bugzilla profile (and use it in the "user" field of
> future patches)? I'll then commit the patch for you. Thanks!

Whoops, my bad, just realized the patch already has a proper "user" field. (It got lost for me when I 'hg import'-ed your patch.) All's good then!

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/39f409d75550

Comment 22

[Chadd Williams]

Excellent. Thanks for all of your help. This is really the perfect [good first bug].

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39f409d75550

Comment 24

[amanjain110893]

Actually I saw a lot of activity in comments and I don't know whether this bug is fixed or not ? If not I want to work on this bug

I am a newbie and urge proper guidance

Comment 25

[Botond Ballo [:botond]]

(In reply to amanjain110893 from comment #24)
> Actually I saw a lot of activity in comments and I don't know whether this
> bug is fixed or not ? If not I want to work on this bug
> 
> I am a newbie and urge proper guidance

We have been transitioning one field at a time. Three of the fields have been transitioned (mScrollOffset, mZoom, and mScrollId); the rest still need to be transitioned.

If you'd like to work on this bug, pick any other field and transition it and post the patch here. Feel free to transition multiple fields, just keep them one per patch.

Thanks!

Comment 26

[Mike Hoye [:mhoye]]

Hi, Amanjain - 

Here are some links that will help you get started setting up your development environment:

https://wiki.mozilla.org/Mobile/Get_Involved
https://developer.mozilla.org/docs/Introduction

If you're still interested in pursuing this bug, please do!

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8394315 [details] [diff] [review]
Update uses of mScrollOffset and mZoom in APZC_LOG calls

The first two patches in this bug landed in 30, the third one only in 31. As a result, 30 does not compile if APZC_LOG debugging is enabled. Requesting uplift of third patch to fix this.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): First two patches in this bug.
User impact if declined: None. Developer impact is that you get compiler errors if you turn on APZC_LOG debugging.
Testing completed (on m-c, etc.): N/A
Risk to taking this patch (and alternatives if risky): None. This patch only touches debugging code that has to be explicitly enabled via a code change.
String or IDL/UUID changes made by this patch: None

Comment 28

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

I'm going to close this bug because it's already got patches split across two releases and I don't want it split across a third as well. We should file follow-up bugs for other fields we want converted if needed, after deciding if we want one bug per field or one bug for everything.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Patches need rebasing for Aurora uplift.

Comment 30

[Botond Ballo [:botond]]

Attachment: Update uses of mScrollOffset and mZoom in APZC_LOG calls, rebased to aurora

Rebased to aurora. Note that this is the only patch that needs to be uplifted.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment 14 was post-uplift, no? Is that going to be 31-only?

Comment 32

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31)
> Comment 14 was post-uplift, no? Is that going to be 31-only?

Comment 14 is the one I rebased and am asking to have uplifted

Comment 23 can be 31-only. (In general the patches in this bug aren't high-priority and don't need uplifting. The only reason to uplift the comment 14 patch is that it fixes compiler errors in patches that landed in 30.)

Comment 33

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> I'm going to close this bug because it's already got patches split across
> two releases and I don't want it split across a third as well. We should
> file follow-up bugs for other fields we want converted if needed, after
> deciding if we want one bug per field or one bug for everything.

I prefer to do one bug per train, as doing one bug per field is tedious while doing one bug for everything is unfriendly to contributors (because it requires them writing a giant patch, or else rebase other people's older patches).

Filed bug 1001582 for 32.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f238addfdac3