DrawingBuffer: Clean up GL state restoration

DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/123016bf9ed08d599540dddc9a3b92a0df7eba2e
Cr-Commit-Position: refs/heads/master@{#426686}

[ccameron, 2016-10-09 20:33:28]

Description was changed from

==========
DrawingBuffer: Clean up GL state management


BUG=
==========

to

==========
DrawingBuffer: Clean up GL state management


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[ccameron, 2016-10-09 20:34:52]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 20:35:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-10-09 20:35:31]

Description was changed from

==========
DrawingBuffer: Clean up GL state management


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
DrawingBuffer: Clean up GL state management


BUG=648707
==========

[ccameron, 2016-10-09 20:46:16]

Description was changed from

==========
DrawingBuffer: Clean up GL state management


BUG=648707
==========

to

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

BUG=648707
==========

[ccameron, 2016-10-09 20:46:16]

ccameron@chromium.org changed reviewers:
+ erikchen@chromium.org, kbr@chromium.org

[ccameron, 2016-10-09 20:47:36]

Description was changed from

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

BUG=648707
==========

to

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
==========

[ccameron, 2016-10-09 21:05:05]

Description was changed from

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
==========

to

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[ccameron, 2016-10-09 21:10:54]

I think this is the last of the clean-up here.

I may have gone overboard with the fine-grained state-tracking. LMK if you think
it's better to just blindly re-set all state.

https://codereview.chromium.org/2402273002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(left):

https://codereview.chromium.org/2402273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1565:
drawingBuffer()->reset(IntSize(width, height));
This callsite has the most sin of all of them...

To start, why is this called "reset" ... what exactly are we re-setting? Was
there something wrong with calling it "resize" or "reshape"??

https://codereview.chromium.org/2402273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1568:
contextGL()->BindTexture(
Note that we already have the helper function
WebGLRenderingContextBase::restoreCurrentTexture2D to do this!

AND we have the DrawingBuffer helper function
DrawingBuffer::restoreTextureBindings to do this, too!

And yet we call neither of them, and re-roll it here!!

https://codereview.chromium.org/2402273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1572:
contextGL()->BindRenderbuffer(GL_RENDERBUFFER,
This line is also precious, because we track the current renderbuffer binding in
DrawingBuffer (as DrawingBuffer::m_renderbufferBinding), but we never have any
methods that use that variable! We're tracking the state for fun in
DrawingBuffer, and restoring it explicitly  here!

https://codereview.chromium.org/2402273002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1574:
drawingBuffer()->restoreFramebufferBindings();
We already have a method WebGLRenderingContextBase::restoreCurrentFramebuffer to
do EXACTLY this... why are we implementing it in DrawingBuffer and then calling
that????

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (left):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:145: 
Everything below here is unneeded state tracking.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:217: // Rebind
the read and draw framebuffers that WebGL is expecting.
I'm leaving this here because of interactions with ScopedDrawingBufferBinder.
Arguably, we shouldn't need this.

[erikchen, 2016-10-10 21:43:03]

lgtm.

Good cleanup, good tests. :)

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7383: if
(m_activeTextureUnit < m_textureUnits.size()) {
why is this conditional necessary. Should this be a DCHECK?

[Ken Russell (switch to Gerrit), 2016-10-11 06:21:57]

Thanks for pitching in to clean up this code. It's gotten complicated over the
past few years as more features have been added.

LGTM overall. Only one significant question below.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7383: if
(m_activeTextureUnit < m_textureUnits.size()) {
On 2016/10/10 21:43:02, erikchen wrote:
> why is this conditional necessary. Should this be a DCHECK?

Same question here. It doesn't look like that test was in the original code.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:419: Client*
m_client = nullptr;
Have you verified the relative lifetimes of WebGLRenderingContextBase and
DrawingBuffer? WebGLRenderingContextBase seems to own DrawingBuffer, but yet
DrawingBuffer right now is RefCounted and I don't remember why. It would be
problematic if the DrawingBuffer could outlive the WebGLRenderingContextBase and
therefore the Client pointer.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
(right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h:342:
void VeriyStateHasNotChangedSinceSave() const {
typo: Veriy -> Verify

[erikchen, 2016-10-11 06:33:04]

> 
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right):
> 
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:419: Client*
> m_client = nullptr;
> Have you verified the relative lifetimes of WebGLRenderingContextBase and
> DrawingBuffer? WebGLRenderingContextBase seems to own DrawingBuffer, but yet
> DrawingBuffer right now is RefCounted and I don't remember why. It would be
Resources DrawingBuffer passes to the compositor may outlive
WebGLRenderingContextBase.

> problematic if the DrawingBuffer could outlive the WebGLRenderingContextBase
and
> therefore the Client pointer.
DrawingBuffer::beginDestruction clears m_client, and
DrawingBuffer::ScopedStateRestorer::~ScopedStateRestorer has a nullptr check.

[Ken Russell (switch to Gerrit), 2016-10-11 22:53:56]

On 2016/10/11 06:33:04, erikchen wrote:
> > 
> >
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
(right):
> > 
> >
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:419: Client*
> > m_client = nullptr;
> > Have you verified the relative lifetimes of WebGLRenderingContextBase and
> > DrawingBuffer? WebGLRenderingContextBase seems to own DrawingBuffer, but yet
> > DrawingBuffer right now is RefCounted and I don't remember why. It would be
> Resources DrawingBuffer passes to the compositor may outlive
> WebGLRenderingContextBase.
> 
> > problematic if the DrawingBuffer could outlive the WebGLRenderingContextBase
> and
> > therefore the Client pointer.
> DrawingBuffer::beginDestruction clears m_client, and
> DrawingBuffer::ScopedStateRestorer::~ScopedStateRestorer has a nullptr check.

There are a couple of code paths through DrawingBuffer including the public
restoreFramebufferBindings which don't have null checks.

[Ken Russell (switch to Gerrit), 2016-10-12 00:32:14]

On 2016/10/11 22:53:56, Ken Russell wrote:
> On 2016/10/11 06:33:04, erikchen wrote:
> > > 
> > >
> >
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:419:
Client*
> > > m_client = nullptr;
> > > Have you verified the relative lifetimes of WebGLRenderingContextBase and
> > > DrawingBuffer? WebGLRenderingContextBase seems to own DrawingBuffer, but
yet
> > > DrawingBuffer right now is RefCounted and I don't remember why. It would
be
> > Resources DrawingBuffer passes to the compositor may outlive
> > WebGLRenderingContextBase.
> > 
> > > problematic if the DrawingBuffer could outlive the
WebGLRenderingContextBase
> > and
> > > therefore the Client pointer.
> > DrawingBuffer::beginDestruction clears m_client, and
> > DrawingBuffer::ScopedStateRestorer::~ScopedStateRestorer has a nullptr
check.
> 
> There are a couple of code paths through DrawingBuffer including the public
> restoreFramebufferBindings which don't have null checks.

To be clear, still LGTM. I don't think there's a significant issue in this area,
and this cleanup is great.

[ccameron, 2016-10-20 00:38:47]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 00:39:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 01:00:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 01:00:38]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2016-10-20 04:08:03]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 04:08:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 05:03:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 05:03:54]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2016-10-20 06:59:34]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 06:59:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 08:31:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 08:31:49]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-10-20 16:34:05]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 16:34:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 16:37:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 16:37:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2016-10-20 18:19:18]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 18:19:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 21:04:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:04:38]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-10-20 23:49:46]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:50:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-10-20 23:50:46]

Thanks!

I may clean up a little bit more of the state tracking to make it more
clear/manageable after this -- especially based on the "during init" issues
related to the range check.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:7383: if
(m_activeTextureUnit < m_textureUnits.size()) {
On 2016/10/11 06:21:56, Ken Russell wrote:
> On 2016/10/10 21:43:02, erikchen wrote:
> > why is this conditional necessary. Should this be a DCHECK?
> 
> Same question here. It doesn't look like that test was in the original code.

The issue here was that DrawingBuffer could end up modifying this state before
m_textureUnits was initialized.

I've restructured the init so that DrawingBuffer skips restoring state during
initialization, and then forcibly restores all state after initialization. It's
still a bit unsatisfactory, but I'll do more of that refactor later.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:419: Client*
m_client = nullptr;
On 2016/10/11 06:21:57, Ken Russell wrote:
> Have you verified the relative lifetimes of WebGLRenderingContextBase and
> DrawingBuffer? WebGLRenderingContextBase seems to own DrawingBuffer, but yet
> DrawingBuffer right now is RefCounted and I don't remember why. It would be
> problematic if the DrawingBuffer could outlive the WebGLRenderingContextBase
and
> therefore the Client pointer.

DrawingBuffer is RefCounted because it gets "mailbox released" callbacks. In
principle, I think they could be changed to be weak ptrs.

That said, WebGLRenderingContextBase calls DrawingBuffer::beginDestruction
before it resets its m_drawingBuffer pointer. I've updated this to clear
m_client at that call.

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
(right):

https://codereview.chromium.org/2402273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h:342:
void VeriyStateHasNotChangedSinceSave() const {
On 2016/10/11 06:21:57, Ken Russell wrote:
> typo: Veriy -> Verify

Done.

[ccameron, 2016-10-21 00:25:57]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2016-10-21 00:26:16]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-10-21 00:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2402273002/#ps140001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2016-10-21 00:26:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 02:06:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-21 13:26:00]

Description was changed from

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and 
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
DrawingBuffer: Clean up GL state restoration

Several of DrawingBuffer's methods leave the caller's state dirty,
and the responsibility for restoring this state comes in three forms:
- Responsibility of DrawingBuffer.
  - E.g, DrawingBuffer::PrepareTextureMailbox, which calls
    DrawingBuffer::finishPrepareTextureMailboxGpu, which restores
    texture and framebuffer bindings.
- Responsibility of the caller
  - E.g, WebGLRenderingContextBase::restoreStateAfterClear and
    everything that calls it.
  - Except..
- Both, together, in strange ways. That is, the caller will explicitly
  restore some state, but it will also call into DrawingBuffer to
  restore some of the state
  - E.g, WebGLRenderingContextBase::reshape, where we have all
    sorts of state restore calls, from all sorts of places.

Note how strange it is to have WebGLRenderingContextBase call
DrawingBuffer to have it restore state. WebGLRenderingContextBase is
the structure that knows the values to restore -- DrawingBuffer only
knows these values because WebGLRenderingContextBase told them to it.

Get rid of all of the state tracking in DrawingBuffer. Instead, have
it call back to its Client (WebGLRenderingContextBase) to restore state
at the end of all of its public entrypoints.

There exists no testing for this state restoration. Sprinkle "verify
that state was restored correctly" calls throughout the existing tests.

Also, make some method names more sensical. Change "commit" to a
name that reflects what it does and the state changes associated with it.
Change "reset" to "resize".

BUG=648707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/123016bf9ed08d599540dddc9a3b92a0df7eba2e
Cr-Commit-Position: refs/heads/master@{#426686}
==========

[commit-bot: I haz the power, 2016-10-21 13:26:02]

Patchset 8 (id:??) landed as
https://crrev.com/123016bf9ed08d599540dddc9a3b92a0df7eba2e
Cr-Commit-Position: refs/heads/master@{#426686}