Transition more fields of FrameMetrics to use getters/setters

RESOLVED FIXED in mozilla38

Status

()

Core
Panning and Zooming
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kats, Assigned: surabhi anand, Mentored)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1116008 +++

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

Anyone: feel free to pick one of the public fields in FrameMetrics (mPresShellResolution is a good one to take), transition it, and post a patch here, flagging me for review. Thanks! See bug 1116009 for an example.
Whiteboard: [lang=c++] [good first bug] → [lang=c++] [good first bug] gfx-noted
(Assignee)

Comment 1

3 years ago
Created attachment 8547680 [details] [diff] [review]
1120203.patch

Please review the patch.
Attachment #8547680 - Flags: review?(bugmail.mozilla)
(Reporter)

Comment 2

3 years ago
bustagetry
Comment on attachment 8547680 [details] [diff] [review]
1120203.patch

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

Looks pretty good! Just a a couple of small changes below, and please add a commit message to the patch. Also for future reference it's a good idea to announce you're working on a bug when you start, so we can assign the bug to you and somebody else doesn't pick it up while you're working on it :)

I've also pushed this patch to try to make sure it passes builds across all platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5604de657103

::: browser/config/mozconfig
@@ +6,5 @@
>  #  . $topsrcdir/browser/config/mozconfig
>  # to the top of your mozconfig file.
>  
>  ac_add_options --enable-application=browser
> +mk_add_options AUTOCLOBBER=1

Remove this change

::: gfx/layers/FrameMetrics.h
@@ +521,5 @@
>  
>  private:
> +  // ---------------------------------------------------------------------------
> +  // The following metrics are dimensionless.
> +  //

You can get rid of this comment
Attachment #8547680 - Flags: review?(bugmail.mozilla) → feedback+
Assignee: nobody → san13692
Please also take a look at the link to the tryserver I posted in comment 2. It shows some build failures on Android; there are uses of mPresShellResolution that are inside an #ifdef so they only get compiled on Android. Your patch will need to update those as well. Thanks!
Hi surabhi, are you still planning on finishing this up?
Flags: needinfo?(san13692)
(Assignee)

Comment 5

3 years ago
ya i will send the patch today.
Flags: needinfo?(san13692)
Great, thanks! :)
(Assignee)

Comment 7

3 years ago
Created attachment 8550349 [details] [diff] [review]
1120203.patch

Here's the updated patch ,please review it.
Attachment #8547680 - Attachment is obsolete: true
Attachment #8550349 - Flags: review?(bugmail.mozilla)
Comment on attachment 8550349 [details] [diff] [review]
1120203.patch

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

For some reason the generated patch was corrupt; one of the hunks in FrameMetrics.h didn't have the right line numbers so I had to fix it up by hand. Not sure how you generated it, but it doesn't really matter now I guess. Patch looks good and the try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f232dbc7833 looks green also. Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3df1027b8b2
Attachment #8550349 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/f3df1027b8b2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 10

3 years ago
Thanks :)
Thank you! :)
You need to log in before you can comment on or make changes to this bug.