Framebuffer clear() needs to consider the situation some draw buffers are disabled.

Framebuffer clear() needs to consider the situation some draw buffers are disabled.

This is when we expose DrawBuffers extension.

BUG=376951
TEST=the attached test case, webgl conformance
R=kbr@chromium.org,bajones@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275338

[Zhenyao Mo, 2014-06-05 18:22:57]

Please take a look.

[bajones, 2014-06-05 18:31:14]

LGTM with one tiny nit

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:569: 
Could you remove this unnecessary whitespace change?

[Ken Russell (switch to Gerrit), 2014-06-05 20:00:41]

Very nice work. LGTM

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:398:
glDrawBuffersARB(manager_->max_draw_buffers_, buffers.get());
Is it guaranteed that we won't try to make these calls if the extension isn't
available?

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.h:139: void
RecoverDrawBuffersAfterClear() const;
For consistency I suggest "Restore" instead of "Recover", here and throughout.

[Zhenyao Mo, 2014-06-05 21:27:49]

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:398:
glDrawBuffersARB(manager_->max_draw_buffers_, buffers.get());
On 2014/06/05 20:00:40, Ken Russell wrote:
> Is it guaranteed that we won't try to make these calls if the extension isn't
> available?

If the extension isn't available, then different should always be false (the
beginning of the unit test is testing this), so this function is basically not
doing anything.

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:569: 
On 2014/06/05 18:31:14, bajones wrote:
> Could you remove this unnecessary whitespace change?

There is no reason that we need three blank lines instead of one.  I feel it's
so trivia we don't need a separate CL to fix it.

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.h (right):

https://codereview.chromium.org/315283002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.h:139: void
RecoverDrawBuffersAfterClear() const;
On 2014/06/05 20:00:40, Ken Russell wrote:
> For consistency I suggest "Restore" instead of "Recover", here and throughout.

Done.

[Zhenyao Mo, 2014-06-05 21:27:54]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-06-05 21:29:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zmo@chromium.org/315283002/20001

[commit-bot: I haz the power, 2014-06-06 01:51:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 05:21:45]

Change committed as 275338