Closed
Bug 1030741
Opened 10 years ago
Closed 10 years ago
Transition more fields of FrameMetrics to use getters/setters
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
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!
Assignee | ||
Comment 1•10 years ago
|
||
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)
Reporter | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Reporter | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
(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 ?
Reporter | ||
Comment 6•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(kylma)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
Note that mViewport has been transitioned in bug 912700.
Assignee | ||
Comment 9•10 years ago
|
||
Try-push is here: https://tbpl.mozilla.org/?tree=Try&rev=de20812f6141
Reporter | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8449816 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8477887 -
Flags: feedback?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8449816 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8477887 -
Flags: feedback?(bugmail.mozilla) → review+
Reporter | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1135f5842f1f
Reporter | ||
Comment 14•10 years ago
|
||
.. 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
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1135f5842f1f https://hg.mozilla.org/mozilla-central/rev/e6abbd1b9555
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Reporter | ||
Comment 16•10 years ago
|
||
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.
Description
•