Closed Bug 1030741 Opened 5 years ago Closed 5 years ago

Transition more fields of FrameMetrics to use getters/setters

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: kylma, Mentored)

Details

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

Attachments

(1 file, 1 obsolete file)

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

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.

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

I ran reftest before and after my patch, and two tests failed:
TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled
TEST-UNEXPECTED-FAIL | file:///.../layout/reftests/reftest-sanity/needs-focus.html | load failed: timed out waiting for reftest-wait to be removed
How can I fix this ?
Attachment #8449816 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8449816 [details] [diff] [review]
IsRoot transitioned, getter and setter added

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

(In reply to Emma Benoit from comment #1)
> I ran reftest before and after my patch, and two tests failed:
> TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled
> TEST-UNEXPECTED-FAIL |
> file:///.../layout/reftests/reftest-sanity/needs-focus.html | load failed:
> timed out waiting for reftest-wait to be removed
> How can I fix this ?

Do these tests fail consistently with the patch and succeed without the patch? We have a number of tests which will fail intermittently and it may have nothing to do with your patch at all. That seems likely in this case because the patch shouldn't have any functional effects at all. If the failure is in fact caused by your patch it's mostly likely a pre-existing race condition in the code that is aggravated by your change, but that just means we should figure out what the race condition is and fix it properly.

Also, do you have tryserver privileges? If so, you can do a try push with this patch to run it on the official build machines to see if the tests are failing there too. (If you don't have have tryserver privileges let me know and we can get you set up).
Attachment #8449816 - Flags: feedback?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)

> Do these tests fail consistently with the patch and succeed without the
> patch? 
If I remember well, the first test failed with and without the patch.
The second one failed appeared after my patch I think.
I'll test more precisely when I get home.

> Also, do you have tryserver privileges? 
No, I don't have them.
(In reply to Emma Benoit from comment #3)
> If I remember well, the first test failed with and without the patch.
> The second one failed appeared after my patch I think.
> I'll test more precisely when I get home.

Sounds good, thanks.

> > Also, do you have tryserver privileges? 
> No, I don't have them.

If you want to get access to the tryserver, please read through the instructions at http://www.mozilla.org/hacking/committer/ and apply for level 1 access.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> If you want to get access to the tryserver, please read through the
> instructions at http://www.mozilla.org/hacking/committer/ and apply for
> level 1 access.
Can you vouch for me on Bug 1034812 ?
Hi Emma,

Any progress here? Were you able to verify the failures you were seeing are intermittent (or not)? Let me know if you need help moving forward on this bug.
Flags: needinfo?(kylma)
Hi Kartikaya,

I was away, I haven't had the time to look how to try-push yet, I'll do it this week.
Flags: needinfo?(kylma)
Note that mViewport has been transitioned in bug 912700.
The tests look fine. I suspect the issues you were seeing before were just intermittent problems. If you're satisfied with it, please go ahead and flag the latest version of your patch for review.
Attachment #8449816 - Flags: review?(bugmail.mozilla)
Comment on attachment 8449816 [details] [diff] [review]
IsRoot transitioned, getter and setter added

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

It's looks good, but will probably need to be rebased onto the latest code in mozilla-central. I know the code in RenderFrameParent at least was touched recently and this patch is now almost two months old so it probably doesn't apply cleanly anymore. Please rebase and upload a new version. Also, the commit message should probably be more clear, something like "Make FrameMetrics::mIsRoot private and add getter/setter methods to manipulate it".

Other than that it looks good, thanks!
Attachment #8449816 - Flags: review?(bugmail.mozilla) → review+
Attachment #8449816 - Attachment is obsolete: true
Attachment #8477887 - Flags: feedback?(bugmail.mozilla) → review+
.. and a follow-up to fix android build bustage, because we missed one of the uses of mIsRoot that was added to the code relatively recently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6abbd1b9555
https://hg.mozilla.org/mozilla-central/rev/1135f5842f1f
https://hg.mozilla.org/mozilla-central/rev/e6abbd1b9555
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Nice work, Emma! Feel free to pick up another bug to work on, or let me know if you'd like me to suggest another one that would be suitable.
You need to log in before you can comment on or make changes to this bug.