Closed Bug 980493 Opened 6 years ago Closed 6 years ago

Transition other fields of FrameMetrics to use getters/setters

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(5 files, 3 obsolete files)

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).
Assignee: nobody → botond
Attachment #8389848 - Flags: review?(bugmail.mozilla)
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.
Attachment #8389848 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8389891 - Flags: review?(bugmail.mozilla)
Attachment #8389891 - Flags: review?(bugmail.mozilla) → review+
(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.
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
Attachment #8389848 - Attachment is obsolete: true
Attachment #8389922 - Flags: review+
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
Attachment #8389922 - Attachment is obsolete: true
Attachment #8389985 - Flags: review+
Marking 'leave-open' as we will likely want to transition other fields as well.
Keywords: leave-open
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.
Whiteboard: [mentor=botond] [lang=c++] [good first bug]
Assignee: botond → nobody
I missed the uses of mScrollOffset and mZoom in APZC_LOG calls, which are only compiled when APZC_LOG is defined to do something.
Attachment #8394315 - Flags: review?(bugmail.mozilla)
Attachment #8394315 - Flags: review?(bugmail.mozilla) → review+
Transition FrameMetrics::mScrollId to use a getter/setter
Attachment #8394364 - Flags: review?(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.
Attachment #8394364 - Flags: review?(botond) → review+
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).
Flags: needinfo?(bugmail.mozilla)
I'm fine with landing it now; it'll probably bitrot otherwise.
Flags: needinfo?(bugmail.mozilla)
This fixes the const parameter mentioned in the review above.
Attachment #8394364 - Attachment is obsolete: true
Attachment #8394992 - Flags: review?(botond)
Comment on attachment 8394992 [details] [diff] [review]
This is a patch to transition FrameMetrics::mScrollId to use a getter/setter

Thanks!
Attachment #8394992 - Flags: review?(botond) → review+
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!
Flags: needinfo?(chaddcw)
(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
Flags: needinfo?(chaddcw)
Excellent. Thanks for all of your help. This is really the perfect [good first bug].
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
(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!
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 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
Attachment #8394315 - Flags: approval-mozilla-aurora?
Attachment #8394315 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [mentor=botond] [lang=c++] [good first bug]
Target Milestone: --- → mozilla31
Patches need rebasing for Aurora uplift.
Assignee: nobody → botond
Flags: needinfo?(botond)
Rebased to aurora. Note that this is the only patch that needs to be uplifted.
Flags: needinfo?(botond)
Comment 14 was post-uplift, no? Is that going to be 31-only?
Flags: needinfo?(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.)
Flags: needinfo?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
You need to log in before you can comment on or make changes to this bug.