Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1002013003:
Unit Test for WebView animating in and out of screen (Closed)
|
Created:
5 years, 9 months ago by hush (inactive) Modified:
5 years, 9 months ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnit Test for WebView animating in and out of screen
This makes sure we don't break crbug.com/417479.
BUG=
Committed: https://crrev.com/abaade8c1f7de4c2f6b26770cbb07e59cef54012
Cr-Commit-Position: refs/heads/master@{#321496}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Test #
Total comments: 4
Patch Set 3 : comments #
Total comments: 7
Patch Set 4 : comments #Patch Set 5 : TestAnimateInAndOutOfScreen #Patch Set 6 : #
Total comments: 4
Patch Set 7 : new test #
Total comments: 20
Patch Set 8 : address some comments #
Total comments: 1
Patch Set 9 : comments #
Total comments: 1
Patch Set 10 : Yet another way to write the test. #
Total comments: 7
Patch Set 11 : Rebase and address comments #Patch Set 12 : only rebase #Patch Set 13 : changes to test #
Total comments: 6
Patch Set 14 : comments #Patch Set 15 : Use a separate counter on RT. #Patch Set 16 : comments #
Total comments: 12
Patch Set 17 : comments #
Total comments: 3
Messages
Total messages: 47 (4 generated)
hush@chromium.org changed reviewers: + boliu@chromium.org
PTAL. This was deleted by https://codereview.chromium.org/944053004/diff/180001/android_webview/browser... and we shouldn't have done that.
https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer_unittest.cc:62: class TestForceInvalidateWithEmptyGlobalVisibleRect : public RenderingTest { Does this actually fail without the fix? DrawGL still happens with the contraints from the default FakeWindow values, which I assume is different than what's initialized in BVR, which means BVR will invalidate either way? I think this test needs to be much more verbose, and actually change the WillDrawOnRT to return a bool, and if false, then skip DrawGL(kModeDraw) Then simulate the whole sequence, draw normally, and wait for the parent constraints to arrive in BVR. Then move it offscreen (ie empty visible rect) and skip DrawGL. Then move it back to same place, check parents contraints still match, but we invalidate anyway.
https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer_unittest.cc:62: class TestForceInvalidateWithEmptyGlobalVisibleRect : public RenderingTest { On 2015/03/12 22:54:01, boliu wrote: > Does this actually fail without the fix? > > DrawGL still happens with the contraints from the default FakeWindow values, > which I assume is different than what's initialized in BVR, which means BVR will > invalidate either way? > > I think this test needs to be much more verbose, and actually change the > WillDrawOnRT to return a bool, and if false, then skip DrawGL(kModeDraw) > > Then simulate the whole sequence, draw normally, and wait for the parent > constraints to arrive in BVR. Then move it offscreen (ie empty visible rect) and > skip DrawGL. Then move it back to same place, check parents contraints still > match, but we invalidate anyway. Yes, it fails without the fix. SharedRendererState won't post the constraints to BVR because of this if-statement returned false: https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br... This test itself is still meaningful for asserting we forced the parent draw constraints through. I can add another test for the whole scene, something like TestAnimatingInAndOutOfScreen
https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer_unittest.cc:62: class TestForceInvalidateWithEmptyGlobalVisibleRect : public RenderingTest { On 2015/03/12 23:12:00, hush wrote: > On 2015/03/12 22:54:01, boliu wrote: > > Does this actually fail without the fix? > > > > DrawGL still happens with the contraints from the default FakeWindow values, > > which I assume is different than what's initialized in BVR, which means BVR > will > > invalidate either way? > > > > I think this test needs to be much more verbose, and actually change the > > WillDrawOnRT to return a bool, and if false, then skip DrawGL(kModeDraw) > > > > Then simulate the whole sequence, draw normally, and wait for the parent > > constraints to arrive in BVR. Then move it offscreen (ie empty visible rect) > and > > skip DrawGL. Then move it back to same place, check parents contraints still > > match, but we invalidate anyway. > > Yes, it fails without the fix. SharedRendererState won't post the constraints to > BVR because of this if-statement returned false: > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br... Without this fix, I don't see how that returns false. Shouldn't the ParentConstraints copy in SRS need to be updated at least once? Which means at least another invalidate, which means at least 2 draws? > > > This test itself is still meaningful for asserting we forced the parent draw > constraints through. But, but... you are not actually checking any of that stuff. If you need to expose a test only class that returns the ParentConstraints from BVR, I support that. > I can add another test for the whole scene, something like > TestAnimatingInAndOutOfScreen
https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer_unittest.cc:62: class TestForceInvalidateWithEmptyGlobalVisibleRect : public RenderingTest { > Shouldn't the ParentConstraints copy in SRS need to be updated at least once? Looks like you've found another bug in HardwareRenderer... The first time you call DrawGL, the viewport_ will still be an empty 0x0 rect. Then it is posted to SRS. I think we should've set the viewport before we post it the SRS. However, even with that problem in hardware renderer fixed, SRS still won't post the draw constraints to the child compositor because the surface rect does not matter if is_layer == false. Since both the old and new constraints is_layer == false, the only thing matters is the transform. And it is identity matrix. Anyways, the point is unless you set needs_force_invalidate_on_next_draw_gl, or assign a non-identity transform matrix in fake_window, SRS won't post the draw constraints to the child compositor. I agree this is a problem on the test setup...
https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer_unittest.cc:62: class TestForceInvalidateWithEmptyGlobalVisibleRect : public RenderingTest { On 2015/03/12 23:44:28, hush wrote: > > Shouldn't the ParentConstraints copy in SRS need to be updated at least once? > > Looks like you've found another bug in HardwareRenderer... The first time you > call DrawGL, the viewport_ will still be an empty 0x0 rect. Then it is posted to > SRS. I think we should've set the viewport before we post it the SRS. It's surprising what you find when you actually write tests... You can fix that one separately too :p > > However, even with that problem in hardware renderer fixed, SRS still won't post > the draw constraints to the child compositor because the surface rect does not > matter if is_layer == false. > Since both the old and new constraints is_layer == false, the only thing matters > is the transform. And it is identity matrix. Oh I see. Ok, as long as you verify those things in the test, then I'm fine. > > Anyways, the point is unless you set needs_force_invalidate_on_next_draw_gl, or > assign a non-identity transform matrix in fake_window, SRS won't post the draw > constraints to the child compositor. I agree this is a problem on the test > setup...
Updated PS2. It fixes the minor problem in hardware renderer and the added test needs it and verifies it.
I'll look tomorrow.. Question: does this need to be merged to m42?
On 2015/03/13 03:28:48, boliu wrote: > I'll look tomorrow.. > > Question: does this need to be merged to m42? I think not.. The breaking CL: https://codereview.chromium.org/944053004 landed #318126 after the branch point of m42 #317474. (if the refs/heads/master commit # monotonically increases) The hardware renderer problem has been there for a long time. So it's not a regression..
https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.h:116: ParentCompositorDrawConstraints& parent_draw_constraints() { add _for_testing to the method name, return type should be const& https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:83: gfx::Rect(100, 100)); Hmm, you need to make sure that after the first OnDraw, parent_draw_constraints already matches (pre-condition for this test to be meaningful), and it's the .Equals call you care about, not that surafce_rect matches.
https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer.h:116: ParentCompositorDrawConstraints& parent_draw_constraints() { On 2015/03/13 14:23:05, boliu wrote: > add _for_testing to the method name, return type should be const& Done. https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/20001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:83: gfx::Rect(100, 100)); On 2015/03/13 14:23:05, boliu wrote: > Hmm, you need to make sure that after the first OnDraw, parent_draw_constraints > already matches (pre-condition for this test to be meaningful), and it's the > .Equals call you care about, not that surafce_rect matches. Done.
https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:81: gfx::Rect(100, 100)); Can we get these from FakeWindow instead of hard coding? https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:88: initialConstraint)); Shouldn't this be newConstraint? Then drop initialConstraint.
PTAL the new test TestAnimateInAndOutOfScreen as well https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:81: gfx::Rect(100, 100)); On 2015/03/13 17:53:34, boliu wrote: > Can we get these from FakeWindow instead of hard coding? Yes. Sure. https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:88: initialConstraint)); On 2015/03/13 17:53:34, boliu wrote: > Shouldn't this be newConstraint? Then drop initialConstraint. No. After the first onDraw, the new constraint from parent compositor hasn't been posted back to UI yet. The new constraint will be set within the second onDrawHardware.
https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:81: gfx::Rect(100, 100)); On 2015/03/13 18:33:30, hush wrote: > On 2015/03/13 17:53:34, boliu wrote: > > Can we get these from FakeWindow instead of hard coding? > > Yes. Sure. Ehh, get the other two as well? You can get FakeWindow to return the whole ParentConstraints https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:88: initialConstraint)); On 2015/03/13 18:33:30, hush wrote: > On 2015/03/13 17:53:34, boliu wrote: > > Shouldn't this be newConstraint? Then drop initialConstraint. > > No. After the first onDraw, the new constraint from parent compositor hasn't > been posted back to UI yet. The new constraint will be set within the second > onDrawHardware. Then test setup is still wrong, no? Test is supposed to test what's in BVR already matches the "new" constraints, so without this "force invalidate" thing, a new invalidate would not happen. And on a practical note, doesn't Equals ignore surface size if is_layer is false?
https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:105: class TestAnimateInAndOutOfScreen : public RenderingTest { I think this should replace the test above.. https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:119: browser_view_renderer_->SetContinuousInvalidate(false); Let's see First draw is normal, and ParentConstraints gets updated (or I guess maybe not because Equals return true, doesn't matter Second draw sets visible rect to empty, and DrawGL gets skipped. Third draw comes from the force invalidate from the second one. I assume? But how does this work when the DrawGL from 2 got skipped (and we didn't schedule another one?
https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/40001/android_webview/browser... android_webview/browser/browser_view_renderer_unittest.cc:88: initialConstraint)); On 2015/03/13 18:50:37, boliu wrote: > On 2015/03/13 18:33:30, hush wrote: > > On 2015/03/13 17:53:34, boliu wrote: > > > Shouldn't this be newConstraint? Then drop initialConstraint. > > > > No. After the first onDraw, the new constraint from parent compositor hasn't > > been posted back to UI yet. The new constraint will be set within the second > > onDrawHardware. > > Then test setup is still wrong, no? Test is supposed to test what's in BVR > already matches the "new" constraints, so without this "force invalidate" thing, > a new invalidate would not happen. > > And on a practical note, doesn't Equals ignore surface size if is_layer is > false? The test setup has some synchronization problems I think... I added more capabilities to the fake_window class and gave the test more control. https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:105: class TestAnimateInAndOutOfScreen : public RenderingTest { On 2015/03/13 19:14:22, boliu wrote: > I think this should replace the test above.. Yes. Indeed. https://codereview.chromium.org/1002013003/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:119: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/13 19:14:22, boliu wrote: > Let's see > > First draw is normal, and ParentConstraints gets updated (or I guess maybe not > because Equals return true, doesn't matter > > Second draw sets visible rect to empty, and DrawGL gets skipped. > > Third draw comes from the force invalidate from the second one. I assume? But > how does this work when the DrawGL from 2 got skipped (and we didn't schedule > another one? Please the see the new patch. I added some new synchronization logic in fake window so that I can guarantee that the ModeDraw is finished before the next onDraw on UI thread. In this way, when the 2nd onDraw runs, the new parent draw constraints are already posted to UI thread.
https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:83: browser_view_renderer_->SetContinuousInvalidate(false); Hmm, doesn't posting of the next OnDrawHardware still race with posting the updated DrawConstraints from RT? Feels like we need a hook for when DrawConstraints is updated in BVR? Not trivial to implement that one.. We can subclass BVR and make that method virtual, then the TestBVR can add in the hook.. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:104: browser_view_renderer_->parent_draw_constraints_for_testing())); DrawGL for on_draw_count_ == 3 uses the identity matrix again. Should wait for that to come back to BVR before ending the test I think? https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.cc:165: if (hooks_->WillWaitForModeDrawToFinish()) { Save the value from above. Don't call it twice. Maybe we need a scoped thingy here.. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.cc:176: if (hooks_->WillWaitForModeDrawToFinish()) { ditto https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:39: virtual void SetParentDrawConstraints(AwDrawGLInfo& draw_info) = 0; I'd much rather have SetSurfaceRect, SetTransform etc on FakeWindow then have this be callbacks. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:42: virtual bool WillWaitForModeDrawToFinish() = 0; This can be a setter too.
https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:83: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/13 23:51:55, boliu wrote: > Hmm, doesn't posting of the next OnDrawHardware still race with posting the > updated DrawConstraints from RT? > > Feels like we need a hook for when DrawConstraints is updated in BVR? Not > trivial to implement that one.. > > We can subclass BVR and make that method virtual, then the TestBVR can add in > the hook.. It does not race I think. Both are post tasks to UI. The previous DrawGL will post a task to set parent draw constraints on UI. Then the condition variable signals, and the next FakeWindow::onDrawHardware is posted by PostInvalidate. So we're sure by the time onDrawHardware is called, the parent draw constraints are already updated. We're lucky that PostInvalidate is not immediate... There is one more thing to address to make onDraw and DrawGL absolutely interlaced and not racy: PostInvalidate could go ahead of DrawGL. I will take care of that in the next path. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:104: browser_view_renderer_->parent_draw_constraints_for_testing())); On 2015/03/13 23:51:55, boliu wrote: > DrawGL for on_draw_count_ == 3 uses the identity matrix again. Should wait for > that to come back to BVR before ending the test I think? Well.. the 4th onDraw will early out in "Previous frame unconsumed" because the 3rd DrawGL is skipped. So the 4th onDraw will not have the identity, until your CL to remove the blocking pipeline is checked in. For the time being, I just add a logic to update the parent draw constraints when the previous frame is unconsumed so that test will pass. And the test will continue passing after your CL lands. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.cc:165: if (hooks_->WillWaitForModeDrawToFinish()) { On 2015/03/13 23:51:55, boliu wrote: > Save the value from above. Don't call it twice. > > Maybe we need a scoped thingy here.. I put it as a private member in the class. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.cc:176: if (hooks_->WillWaitForModeDrawToFinish()) { On 2015/03/13 23:51:55, boliu wrote: > ditto Done. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:39: virtual void SetParentDrawConstraints(AwDrawGLInfo& draw_info) = 0; On 2015/03/13 23:51:55, boliu wrote: > I'd much rather have SetSurfaceRect, SetTransform etc on FakeWindow then have > this be callbacks. Putting it into fake window make it hard for the rendering test to override and customize these 2 functions. RenderingTest relies on overriding WindowHooks to customize FakeWindow behavior. If SetParentDrawConstraints is a function on FakeWindow, RenderingTest has no easy way to call it at the right time, just before the DrawGL because each DrawGL might have different transforms settable from the rendering test (unless the specific test creates a subclass of FakeWindow). But this will over complicate the browser_view_renderer_unittest class. What do you think? https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:42: virtual bool WillWaitForModeDrawToFinish() = 0; On 2015/03/13 23:51:55, boliu wrote: > This can be a setter too. This bool will be read by both RT and UI in the next patch. So I decide to make it required in FakeWindow constructor. (default to false.)
https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:83: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/16 18:19:21, hush wrote: > On 2015/03/13 23:51:55, boliu wrote: > > Hmm, doesn't posting of the next OnDrawHardware still race with posting the > > updated DrawConstraints from RT? > > > > Feels like we need a hook for when DrawConstraints is updated in BVR? Not > > trivial to implement that one.. > > > > We can subclass BVR and make that method virtual, then the TestBVR can add in > > the hook.. > > It does not race I think. Both are post tasks to UI. > The previous DrawGL will post a task to set parent draw constraints on UI. Then > the condition variable signals, and the next FakeWindow::onDrawHardware is > posted by PostInvalidate. PostInvalidate happen at end of BVR::OnDrawHardware, which is definitely not the end of FakeWindow::OnDrawHardware. From staring at the code, it's happening in exactly the wrong order we want, that PostTask(&OnDrawHardware) happens before PostTask(&UpdateParentDrawConstraints), no? But in general, there are no order defined between these two events. So it's not safe to assume they are ordered, because it might break in the future. > So we're sure by the time onDrawHardware is called, the parent draw constraints > are already updated. > > We're lucky that PostInvalidate is not immediate... > There is one more thing to address to make onDraw and DrawGL absolutely > interlaced and not racy: PostInvalidate could go ahead of DrawGL. I will take > care of that in the next path. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:104: browser_view_renderer_->parent_draw_constraints_for_testing())); On 2015/03/16 18:19:21, hush wrote: > On 2015/03/13 23:51:55, boliu wrote: > > DrawGL for on_draw_count_ == 3 uses the identity matrix again. Should wait for > > that to come back to BVR before ending the test I think? > > Well.. the 4th onDraw will early out in "Previous frame unconsumed" because the > 3rd DrawGL is skipped. So the 4th onDraw will not have the identity, until your > CL to remove the blocking pipeline is checked in. > > For the time being, I just add a logic to update the parent draw constraints > when the previous frame is unconsumed so that test will pass. And the test will > continue passing after your CL lands. Hmm. Let's not change production code just to make a test pass. Add a comment here instead, and I'll take care of it in my patch. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:39: virtual void SetParentDrawConstraints(AwDrawGLInfo& draw_info) = 0; On 2015/03/16 18:19:21, hush wrote: > On 2015/03/13 23:51:55, boliu wrote: > > I'd much rather have SetSurfaceRect, SetTransform etc on FakeWindow then have > > this be callbacks. > > Putting it into fake window make it hard for the rendering test to override and > customize these 2 functions. RenderingTest relies on overriding WindowHooks to > customize FakeWindow behavior. > If SetParentDrawConstraints is a function on FakeWindow, RenderingTest has no > easy way to call it at the right time, just before the DrawGL because each > DrawGL might have different transforms settable from the rendering test (unless > the specific test creates a subclass of FakeWindow). Merge it into WillDrawOnRT? > > But this will over complicate the browser_view_renderer_unittest class. What do > you think? https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:42: virtual bool WillWaitForModeDrawToFinish() = 0; On 2015/03/16 18:19:21, hush wrote: > On 2015/03/13 23:51:55, boliu wrote: > > This can be a setter too. > > This bool will be read by both RT and UI in the next patch. So I decide to make > it required in FakeWindow constructor. (default to false.) Ok for now I guess. The other option is make the instance var UI only, then just pass that bool to FakeWindow::DrawFunctorOnRT every time. https://codereview.chromium.org/1002013003/diff/140001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1002013003/diff/140001/android_webview/browse... android_webview/browser/test/fake_window.cc:111: if (will_wait_for_mode_draw_to_finish_) { Oh this is what you meant... please no :( We need a hook for when parent constraints get updated for this test. I guess you could just add it to BVRClient, just for tests. Or subclassing BVR to BVRForTesting isn't that bad either; that's what cc does.
Patchset #9 (id:160001) has been deleted
https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:83: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/16 20:25:08, boliu wrote: > On 2015/03/16 18:19:21, hush wrote: > > On 2015/03/13 23:51:55, boliu wrote: > > > Hmm, doesn't posting of the next OnDrawHardware still race with posting the > > > updated DrawConstraints from RT? > > > > > > Feels like we need a hook for when DrawConstraints is updated in BVR? Not > > > trivial to implement that one.. > > > > > > We can subclass BVR and make that method virtual, then the TestBVR can add > in > > > the hook.. > > > > It does not race I think. Both are post tasks to UI. > > The previous DrawGL will post a task to set parent draw constraints on UI. > Then > > the condition variable signals, and the next FakeWindow::onDrawHardware is > > posted by PostInvalidate. > > PostInvalidate happen at end of BVR::OnDrawHardware, which is definitely not the > end of FakeWindow::OnDrawHardware. From staring at the code, it's happening in > exactly the wrong order we want, that PostTask(&OnDrawHardware) happens before > PostTask(&UpdateParentDrawConstraints), no? > > But in general, there are no order defined between these two events. So it's not > safe to assume they are ordered, because it might break in the future. > > > So we're sure by the time onDrawHardware is called, the parent draw > constraints > > are already updated. > > > > We're lucky that PostInvalidate is not immediate... > > There is one more thing to address to make onDraw and DrawGL absolutely > > interlaced and not racy: PostInvalidate could go ahead of DrawGL. I will take > > care of that in the next path. > Okay. I added a callback in browser view renderer client informing the test when the parent draw constraints get updated. And removed the bits about "wait for mode draw to finish" https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:104: browser_view_renderer_->parent_draw_constraints_for_testing())); On 2015/03/16 20:25:08, boliu wrote: > On 2015/03/16 18:19:21, hush wrote: > > On 2015/03/13 23:51:55, boliu wrote: > > > DrawGL for on_draw_count_ == 3 uses the identity matrix again. Should wait > for > > > that to come back to BVR before ending the test I think? > > > > Well.. the 4th onDraw will early out in "Previous frame unconsumed" because > the > > 3rd DrawGL is skipped. So the 4th onDraw will not have the identity, until > your > > CL to remove the blocking pipeline is checked in. > > > > For the time being, I just add a logic to update the parent draw constraints > > when the previous frame is unconsumed so that test will pass. And the test > will > > continue passing after your CL lands. > > Hmm. Let's not change production code just to make a test pass. > > Add a comment here instead, and I'll take care of it in my patch. Done. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... File android_webview/browser/test/fake_window.h (right): https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:39: virtual void SetParentDrawConstraints(AwDrawGLInfo& draw_info) = 0; On 2015/03/16 20:25:08, boliu wrote: > On 2015/03/16 18:19:21, hush wrote: > > On 2015/03/13 23:51:55, boliu wrote: > > > I'd much rather have SetSurfaceRect, SetTransform etc on FakeWindow then > have > > > this be callbacks. > > > > Putting it into fake window make it hard for the rendering test to override > and > > customize these 2 functions. RenderingTest relies on overriding WindowHooks to > > customize FakeWindow behavior. > > If SetParentDrawConstraints is a function on FakeWindow, RenderingTest has no > > easy way to call it at the right time, just before the DrawGL because each > > DrawGL might have different transforms settable from the rendering test > (unless > > the specific test creates a subclass of FakeWindow). > > Merge it into WillDrawOnRT? > > > > > But this will over complicate the browser_view_renderer_unittest class. What > do > > you think? Done. https://codereview.chromium.org/1002013003/diff/120001/android_webview/browse... android_webview/browser/test/fake_window.h:42: virtual bool WillWaitForModeDrawToFinish() = 0; On 2015/03/16 20:25:08, boliu wrote: > On 2015/03/16 18:19:21, hush wrote: > > On 2015/03/13 23:51:55, boliu wrote: > > > This can be a setter too. > > > > This bool will be read by both RT and UI in the next patch. So I decide to > make > > it required in FakeWindow constructor. (default to false.) > > Ok for now I guess. > > The other option is make the instance var UI only, then just pass that bool to > FakeWindow::DrawFunctorOnRT every time. I just removed this flag..
PTAL PS9
https://codereview.chromium.org/1002013003/diff/180001/android_webview/browse... File android_webview/browser/test/fake_window.cc (right): https://codereview.chromium.org/1002013003/diff/180001/android_webview/browse... android_webview/browser/test/fake_window.cc:109: base::MessageLoopProxy::current()->PostTask( I need this postTask so that the onDraws won't be missed. Looks like I should set the ContinuousInvalidate bit at the right time...
On 2015/03/17 18:38:18, hush wrote: > https://codereview.chromium.org/1002013003/diff/180001/android_webview/browse... > File android_webview/browser/test/fake_window.cc (right): > > https://codereview.chromium.org/1002013003/diff/180001/android_webview/browse... > android_webview/browser/test/fake_window.cc:109: > base::MessageLoopProxy::current()->PostTask( > I need this postTask so that the onDraws won't be missed. > Looks like I should set the ContinuousInvalidate bit at the right time... hang on.. I think there are some race conditions in the test. Sorry.
Another thought for the original problem. We shouldn't be checking against the "last constraints posted", we should be comparing to the constraint used in the last frame. Would that fix the original problem this is trying to workaround?
On 2015/03/17 19:37:40, boliu wrote: > Another thought for the original problem. > > We shouldn't be checking against the "last constraints posted", we should be > comparing to the constraint used in the last frame. Would that fix the original > problem this is trying to workaround? OKay. I just updated PS10, which should work with the new fix: https://codereview.chromium.org/1001643004 (just need to make BVR::parent_draw_constraints_for_testing() reach out to SRS)
Yay, much cleaner now. https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:64: TestAnimateInAndOutOfScreen() : step_(1u) {} We are computer scientists!! We start counting from 0! Also you can put it in StartTest. https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:112: browser_view_renderer_->parent_draw_constraints_for_testing(); We don't need both ConstraintsUpdated and GetConstraints. Just put the constraints as a parameter to the client call? https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:114: step_++; Do we get more than one ParentDrawConstraintsUpdated here? ie do we need the constraints check here and below? Shouldn't we be validating those constraints?
https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:64: TestAnimateInAndOutOfScreen() : step_(1u) {} On 2015/03/19 00:15:20, boliu wrote: > We are computer scientists!! We start counting from 0! > > Also you can put it in StartTest. Done. But I can't put it in StartTest because the constructor needs to assign a value to it.. https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:112: browser_view_renderer_->parent_draw_constraints_for_testing(); On 2015/03/19 00:15:20, boliu wrote: > We don't need both ConstraintsUpdated and GetConstraints. Just put the > constraints as a parameter to the client call? Done. https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:114: step_++; On 2015/03/19 00:15:20, boliu wrote: > Do we get more than one ParentDrawConstraintsUpdated here? ie do we need the > constraints check here and below? Shouldn't we be validating those constraints? By 'validating' I guess you mean EXPECT_TRUE? I'm doing Equals check instead of EXPECT_TRUE because the constraints may not be immediately updated. There will be multiple ParentDrawConstraintsUpdated callbacks until the constraints are updated to |new_constraints_|. So I'm only advancing the steps when the constraints are what they should be.
https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:114: step_++; On 2015/03/19 01:02:51, hush wrote: > On 2015/03/19 00:15:20, boliu wrote: > > Do we get more than one ParentDrawConstraintsUpdated here? ie do we need the > > constraints check here and below? Shouldn't we be validating those > constraints? > > By 'validating' I guess you mean EXPECT_TRUE? > I'm doing Equals check instead of EXPECT_TRUE because the constraints may not be > immediately updated. There will be multiple ParentDrawConstraintsUpdated > callbacks until the constraints are updated to |new_constraints_|. So I'm only > advancing the steps when the constraints are what they should be. I don't understand why there will be multiple ParentDrawConstraintsUpdated calls though
PTAL. I just moved where "ParentDrawConstraintsUpdated" gets called from within onDrawHardware to when it really happens. And then I changed the tests accordingly.
On 2015/03/19 18:23:07, hush wrote: > PTAL. > > I just moved where "ParentDrawConstraintsUpdated" gets called from within > onDrawHardware to when it really happens. And then I changed the tests > accordingly. okay. I've run the same non-stop for half an hour and haven't failed once. No race conditions probably.
Almost there. Update the description please https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:100: if (browser_view_renderer_->global_visible_rect_for_testing().IsEmpty()) This is not thread safe in general. I guess you could either add a lock around on_draw_count_, and use that, or keep a separate count for WillDrawOnRT. Or...add a method to get the current child frame for testing. https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:128: if (on_draw_count_ == 1u) { nit, use a switch? then case 2: and default: can have NOTREACHED() and then don't have to worry about return statements https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... File android_webview/browser/test/rendering_test.h (right): https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/test/rendering_test.h:56: AwDrawGLInfo& draw_info) override; output is usually pointer type non-const ref is banned by style guide
https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:100: if (browser_view_renderer_->global_visible_rect_for_testing().IsEmpty()) On 2015/03/19 20:24:27, boliu wrote: > This is not thread safe in general. > > I guess you could either add a lock around on_draw_count_, and use that, or keep > a separate count for WillDrawOnRT. > > Or...add a method to get the current child frame for testing. Done. https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:128: if (on_draw_count_ == 1u) { On 2015/03/19 20:24:27, boliu wrote: > nit, use a switch? > > then case 2: and default: can have NOTREACHED() > > and then don't have to worry about return statements Okay for case 2. However, there is a race condition for on_draw_count_ being 4. Normally you won't get a forth ondraw. But there are times when the hardware renderer posts identity matrix back to the UI twice, after the webview goes on screen. Looks like in that situation the |child_frame_| in hardware renderer has "new_transform" for 2 frames. I'm not sure why at the moment. I might have some wrong assumptions about the synchronization model... so I'm investigating. https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... File android_webview/browser/test/rendering_test.h (right): https://codereview.chromium.org/1002013003/diff/260001/android_webview/browse... android_webview/browser/test/rendering_test.h:56: AwDrawGLInfo& draw_info) override; On 2015/03/19 20:24:27, boliu wrote: > output is usually pointer type > > non-const ref is banned by style guide Done.
Okay. I will start running the test infinitely till failure. If I don't see it fail in half an hour, I just declare victory.
Patchset #16 (id:320001) has been deleted
https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: PostInvalidate(); Can this be done in DidDrawOnRT (obviously post it to UI first), to avoid races. https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:145: browser_view_renderer_->SetContinuousInvalidate(false); move this to WillOnDraw. Actually, shouldn't SetContinuousInvalidate(false) *always* happen in WillOnDraw to avoid races? https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:149: FAIL(); 4 can happen, because ParentDrawConstraintsUpdated happens for 3
https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: PostInvalidate(); On 2015/03/19 22:48:39, boliu wrote: > Can this be done in DidDrawOnRT (obviously post it to UI first), to avoid races. Sorry... What is the race condition? At this point, the render thread is either handling draw_gl_count_on_rt_ == 1u, or has finished doing it. It is not possible for render thread to be handling the 0th DrawGL because the 0th DrawGL posts the constraints back, which then triggers on_draw_count_ 1. https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:145: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/19 22:48:39, boliu wrote: > move this to WillOnDraw. > > Actually, shouldn't SetContinuousInvalidate(false) *always* happen in WillOnDraw > to avoid races? In SetContinuousInvalidate(true) is called in BrowserViewRenderer::UpdateParentDrawConstraints(), and I don't want to invalidate infinitely. So I reset it right here. I would have the peace of mind if I reset it immediately after BVR sets it to true... https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:149: FAIL(); On 2015/03/19 22:48:39, boliu wrote: > 4 can happen, because ParentDrawConstraintsUpdated happens for 3 We will have the next onDraw, that's for sure. But the hardware renderer won't post the draw constraints back because they are the same (both are identity matrices).
https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: PostInvalidate(); On 2015/03/19 23:29:00, hush wrote: > On 2015/03/19 22:48:39, boliu wrote: > > Can this be done in DidDrawOnRT (obviously post it to UI first), to avoid > races. > > Sorry... What is the race condition? > At this point, the render thread is either handling draw_gl_count_on_rt_ == 1u, > or has finished doing it. That's the race. It's not strictly needed now, but it's just a bit hard to reason about. > It is not possible for render thread to be handling the 0th DrawGL because the > 0th DrawGL posts the constraints back, which then triggers on_draw_count_ 1. https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:145: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/19 23:29:00, hush wrote: > On 2015/03/19 22:48:39, boliu wrote: > > move this to WillOnDraw. > > > > Actually, shouldn't SetContinuousInvalidate(false) *always* happen in > WillOnDraw > > to avoid races? > > In SetContinuousInvalidate(true) is called in > BrowserViewRenderer::UpdateParentDrawConstraints(), and I don't want to > invalidate infinitely. Wait, isn't that EnsureContinuousInvalidation, not SetContinuousInvalidate(true)? > So I reset it right here. I would have the peace of mind > if I reset it immediately after BVR sets it to true... https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:149: FAIL(); On 2015/03/19 23:29:00, hush wrote: > On 2015/03/19 22:48:39, boliu wrote: > > 4 can happen, because ParentDrawConstraintsUpdated happens for 3 > > We will have the next onDraw, that's for sure. But the hardware renderer won't > post the draw constraints back because they are the same (both are identity > matrices). Can you put that in a comment?
https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: PostInvalidate(); On 2015/03/20 00:41:10, boliu wrote: > On 2015/03/19 23:29:00, hush wrote: > > On 2015/03/19 22:48:39, boliu wrote: > > > Can this be done in DidDrawOnRT (obviously post it to UI first), to avoid > > races. > > > > Sorry... What is the race condition? > > At this point, the render thread is either handling draw_gl_count_on_rt_ == > 1u, > > or has finished doing it. > > That's the race. > > It's not strictly needed now, but it's just a bit hard to reason about. > > > It is not possible for render thread to be handling the 0th DrawGL because the > > 0th DrawGL posts the constraints back, which then triggers on_draw_count_ 1. > Okay. Done https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:145: browser_view_renderer_->SetContinuousInvalidate(false); On 2015/03/20 00:41:10, boliu wrote: > On 2015/03/19 23:29:00, hush wrote: > > On 2015/03/19 22:48:39, boliu wrote: > > > move this to WillOnDraw. > > > > > > Actually, shouldn't SetContinuousInvalidate(false) *always* happen in > > WillOnDraw > > > to avoid races? > > > > In SetContinuousInvalidate(true) is called in > > BrowserViewRenderer::UpdateParentDrawConstraints(), and I don't want to > > invalidate infinitely. > > Wait, isn't that EnsureContinuousInvalidation, not > SetContinuousInvalidate(true)? > > > So I reset it right here. I would have the peace of mind > > if I reset it immediately after BVR sets it to true... > yeah...... okay. I moved all SetContinuousInvalidate(false); into WillOnDraw. https://codereview.chromium.org/1002013003/diff/340001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:149: FAIL(); On 2015/03/20 00:41:10, boliu wrote: > On 2015/03/19 23:29:00, hush wrote: > > On 2015/03/19 22:48:39, boliu wrote: > > > 4 can happen, because ParentDrawConstraintsUpdated happens for 3 > > > > We will have the next onDraw, that's for sure. But the hardware renderer won't > > post the draw constraints back because they are the same (both are identity > > matrices). > > Can you put that in a comment? Done.
lgtm https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: base::Unretained(this))); Put it in DidDrawOnRT?
https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: base::Unretained(this))); On 2015/03/20 01:01:44, boliu wrote: > Put it in DidDrawOnRT? There is no DidDrawOnRT for this particular case, because DrawGL didn't happen... That's also the reason why I incremented the counter inside the if-statement.
https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... File android_webview/browser/browser_view_renderer_unittest.cc (right): https://codereview.chromium.org/1002013003/diff/360001/android_webview/browse... android_webview/browser/browser_view_renderer_unittest.cc:98: base::Unretained(this))); On 2015/03/20 01:09:41, hush wrote: > On 2015/03/20 01:01:44, boliu wrote: > > Put it in DidDrawOnRT? > > There is no DidDrawOnRT for this particular case, because DrawGL didn't > happen... > That's also the reason why I incremented the counter inside the if-statement. Ohh, right
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1002013003/360001
Message was sent while issue was closed.
Committed patchset #17 (id:360001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/abaade8c1f7de4c2f6b26770cbb07e59cef54012 Cr-Commit-Position: refs/heads/master@{#321496} |
