Fixed bookkeeping bug in management of dimensions of mailbox's texture.

Fixed bookkeeping bug in management of dimensions of mailbox's texture.

This fixes several problems with rapid resizing of WebGL-rendered canvases.

Also added a WebGL conformance test for this bug.

BUG=299371
TEST=DrawingBufferTest.verifyResizingProperlyAffectsMailboxes

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163509

[Ken Russell (switch to Gerrit), 2013-12-06 04:02:53]

@jamesr: please review. Thanks.

I'd appreciate any guidance on how to force the TestRunner to composite a frame.
Right now the layout test doesn't catch the bug, and I think that's because the
compositor is not running, and displaying to the screen, during the test's
execution.

[jamesr, 2013-12-06 19:41:59]

Why not add a unit test to DrawingBufferTest.cpp ?

[jamesr, 2013-12-06 19:45:16]

content_shell when running layout tests does not go through the browser
presentation path or use the browser's presentation path's output as output, so
it probably won't detect this sort of issue.  You could probably write a browser
test that goes through the full path, but given that this is a bug in
DrawingBuffer's bookkeeping I think a unit test that direct verifies the
behavior of DrawingBuffer is much more useful than an integration test that
observes one possible side effect of the change.

I don't think it's beneficial to add a layout test that runs for this long that
does not provide regression coverage.

[Ken Russell (switch to Gerrit), 2013-12-09 02:34:36]

Thanks for the suggestion to test this with a unit test.

Please re-review. Despite the complexity of the unit test, it's the minimum
amount of code which really verifies the fix. I confirmed by manually
reintroducing the bug that the test catches it.

[jamesr, 2013-12-09 19:18:35]

I'm getting "Old chunk mismatch" on the side-by-side diffs :(.  Could you
reupload?  I'll take a look at the raw diff in the meantime.

[Ken Russell (switch to Gerrit), 2013-12-09 19:21:46]

On 2013/12/09 19:18:35, jamesr wrote:
> I'm getting "Old chunk mismatch" on the side-by-side diffs :(.  Could you
> reupload?  I'll take a look at the raw diff in the meantime.

Re-uploaded. New patch works.

[Ken Russell (switch to Gerrit), 2013-12-09 19:22:18]

On 2013/12/09 19:21:46, Ken Russell wrote:
> On 2013/12/09 19:18:35, jamesr wrote:
> > I'm getting "Old chunk mismatch" on the side-by-side diffs :(.  Could you
> > reupload?  I'll take a look at the raw diff in the meantime.
> 
> Re-uploaded. New patch works.

FYI, I'll be watching the try jobs on Patch Set 3.

[jamesr, 2013-12-09 19:30:23]

It looks like the test is trying to verify that the textures produced into the
mailbox are the right size.  Verifying the specific calls is one way to do this,
but it's fragile as it's specific to the exact call sequence we make.  Instead,
why not keep track of the currently bound texture and texture definition calls
in the test context and then verify that this matches what we expect in
produceTextureCHROMIUM calls?  I.e. something like

class WGC3DForTests : public MockWGCD3D {
public:
    virtual void bindTexture(...) { m_boundTexture = ..; }
    virtual void texImage2D(...) { m_textureSizes[m_boundTexture] = ...; }
    virtual void produceTextureCHROMIUM(...) { m_producedSize =
m_textureSizes[m_boundTexture]; }

    IntSize producedSize() const { return m_producedSize; }
private:
    IntSize m_producedSize;
    unsigned m_boundTexture;
    HashMap<unsigned, IntSize> m_textureSizes;
};


then in the test run through the exact sequence you have now except instead of
having EXPECT_CALL/etcs, check the producedSize() after the prepareMailbox()
call.  WDYT?

The code change itself lgtm and this test is probably OK for now - it'll just
bite whoever tries to refactor this code next.

[Ken Russell (switch to Gerrit), 2013-12-09 20:47:19]

On 2013/12/09 19:30:23, jamesr wrote:
> It looks like the test is trying to verify that the textures produced into the
> mailbox are the right size.  Verifying the specific calls is one way to do
this,
> but it's fragile as it's specific to the exact call sequence we make. 
Instead,
> why not keep track of the currently bound texture and texture definition calls
> in the test context and then verify that this matches what we expect in
> produceTextureCHROMIUM calls?  I.e. something like
> 
> class WGC3DForTests : public MockWGCD3D {
> public:
>     virtual void bindTexture(...) { m_boundTexture = ..; }
>     virtual void texImage2D(...) { m_textureSizes[m_boundTexture] = ...; }
>     virtual void produceTextureCHROMIUM(...) { m_producedSize =
> m_textureSizes[m_boundTexture]; }
> 
>     IntSize producedSize() const { return m_producedSize; }
> private:
>     IntSize m_producedSize;
>     unsigned m_boundTexture;
>     HashMap<unsigned, IntSize> m_textureSizes;
> };
> 
> 
> then in the test run through the exact sequence you have now except instead of
> having EXPECT_CALL/etcs, check the producedSize() after the prepareMailbox()
> call.  WDYT?
> 
> The code change itself lgtm and this test is probably OK for now - it'll just
> bite whoever tries to refactor this code next.

Thanks very much for this suggestion. I'd been trying to figure out how to test
this in the way you suggested but hadn't been able to. Did this instead; the
test is much simpler and should be a lot more robust to changes in the
DrawingBuffer code. CQ'ing.

[commit-bot: I haz the power, 2013-12-09 20:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/105123003/80001

[jamesr, 2013-12-09 21:18:37]

looks great!  You might get some unused variable warnings for the 'using
testing::' statements on some bots, if so just remove them.

[Ken Russell (switch to Gerrit), 2013-12-09 22:05:39]

On 2013/12/09 21:18:37, jamesr wrote:
> looks great!  You might get some unused variable warnings for the 'using
> testing::' statements on some bots, if so just remove them.

Oops, forgot about those. I'll abort the CQ and fix them now.

[commit-bot: I haz the power, 2013-12-09 22:28:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/105123003/60002

[commit-bot: I haz the power, 2013-12-10 03:18:07]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-12-10 07:01:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/105123003/60002

[commit-bot: I haz the power, 2013-12-10 07:42:47]

Change committed as 163509