Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_ whose
content is resolved from multisample_fbo_.

BUG=725579
TESTCASE=DrawingBufferSoftwareRenderingTest.framebufferBinding

Review-Url: https://codereview.chromium.org/2905633002
Cr-Commit-Position: refs/heads/master@{#476807}
Committed: https://chromium.googlesource.com/chromium/src/+/f4f805f9fb47f434766195909c50a46b484ffdbb

[xinghua.cao, 2017-05-24 10:51:02]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 10:51:15]

Dry run: CQ is trying da patch.

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

[xinghua.cao, 2017-05-24 10:51:41]

Description was changed from

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=7205579
==========

to

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=7205579
==========

[xinghua.cao, 2017-05-24 11:07:45]

xinghua.cao@intel.com changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[xinghua.cao, 2017-05-24 11:24:39]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:314: :
WebGLImageConversion::kAlphaDoNothing;
Here may be binding framebuffer on multisample_fbo_, GL_INVALID_OPERATION
happens when calling readPixels in ReadBackFramebuffer function. So we need to
be sure that currently bind framebuffer on fbo_, and restore binding status
after PrepareTextureMailbox

[commit-bot: I haz the power, 2017-05-24 12:26:37]

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

[commit-bot: I haz the power, 2017-05-24 12:26:38]

Dry run: This issue passed the CQ dry run.

[capn, 2017-05-24 13:57:31]

Thanks, I can confirm this fixes the issue! Could you also add or modify tests
in DrawingBufferSoftwareRenderingTest.cpp that verify the new requirement that
it's the caller's responsibility to manage the bindings? Thanks again.

[Zhenyao Mo, 2017-05-24 20:31:01]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
I think BindFramebuffer(_, fbo_) should be part of ReadBackFramebuffer().

[Ken Russell (switch to Gerrit), 2017-05-24 20:40:04]

I defer to Mo's review, but one comment.

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
On 2017/05/24 20:31:01, Zhenyao Mo wrote:
> I think BindFramebuffer(_, fbo_) should be part of ReadBackFramebuffer().

Looking at the code, I'm not sure it can be.
DrawingBuffer::PaintRenderingResultsToImageData has the option of reading back
the front buffer, in which case a temporary FBO is allocated and bound.

[Zhenyao Mo, 2017-05-24 20:53:30]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
On 2017/05/24 20:40:04, Ken Russell wrote:
> On 2017/05/24 20:31:01, Zhenyao Mo wrote:
> > I think BindFramebuffer(_, fbo_) should be part of ReadBackFramebuffer().
> 
> Looking at the code, I'm not sure it can be.
> DrawingBuffer::PaintRenderingResultsToImageData has the option of reading back
> the front buffer, in which case a temporary FBO is allocated and bound.

kbr is right. Sorry about the noise. The code is fine as is.  Please address
capn's test change request.

[Zhenyao Mo, 2017-05-24 23:24:32]

Description was changed from

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=7205579
==========

to

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=725579
==========

[xinghua.cao, 2017-05-25 10:43:17]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
On 2017/05/24 20:53:30, Zhenyao Mo wrote:
> On 2017/05/24 20:40:04, Ken Russell wrote:
> > On 2017/05/24 20:31:01, Zhenyao Mo wrote:
> > > I think BindFramebuffer(_, fbo_) should be part of ReadBackFramebuffer().
> > 
> > Looking at the code, I'm not sure it can be.
> > DrawingBuffer::PaintRenderingResultsToImageData has the option of reading
back
> > the front buffer, in which case a temporary FBO is allocated and bound.
> 
> kbr is right. Sorry about the noise. The code is fine as is.  Please address
> capn's test change request.

Hi, all, I am confused how to design the test case for this patch in
DrawingBufferSoftwareRenderingTest.cpp. I do not know what is expected result
for this case. Does we need to verify the framebuffer object currently is being
binded? Or the TextureMailbox object return by PrepareTextureMailbox? But I
think they cannot verify validity of this patch.
Do you have some good idea? Thank you.

[Zhenyao Mo, 2017-05-25 18:03:00]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
On 2017/05/25 10:43:17, xinghua.cao wrote:
> On 2017/05/24 20:53:30, Zhenyao Mo wrote:
> > On 2017/05/24 20:40:04, Ken Russell wrote:
> > > On 2017/05/24 20:31:01, Zhenyao Mo wrote:
> > > > I think BindFramebuffer(_, fbo_) should be part of
ReadBackFramebuffer().
> > > 
> > > Looking at the code, I'm not sure it can be.
> > > DrawingBuffer::PaintRenderingResultsToImageData has the option of reading
> back
> > > the front buffer, in which case a temporary FBO is allocated and bound.
> > 
> > kbr is right. Sorry about the noise. The code is fine as is.  Please address
> > capn's test change request.
> 
> Hi, all, I am confused how to design the test case for this patch in
> DrawingBufferSoftwareRenderingTest.cpp. I do not know what is expected result
> for this case. Does we need to verify the framebuffer object currently is
being
> binded? Or the TextureMailbox object return by PrepareTextureMailbox? But I
> think they cannot verify validity of this patch.
> Do you have some good idea? Thank you. 

May check gl errors after PrepareTextureMailbox() call in the test would be
enough?

Or you can check current fbo binding before and after the
PrepareTextureMailbox() call and make sure they don't change?

[xinghua.cao, 2017-05-26 11:09:39]

Description was changed from

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=725579
==========

to

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=725579
TESTCASE=DrawingBufferSoftwareRenderingTest.framebufferBinding
==========

[xinghua.cao, 2017-05-26 11:09:45]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 11:09:55]

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, 2017-05-26 12:37:45]

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

[commit-bot: I haz the power, 2017-05-26 12:37:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xinghua.cao, 2017-05-26 13:11:10]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 13:11:19]

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, 2017-05-26 15:24:50]

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

[commit-bot: I haz the power, 2017-05-26 15:24:52]

Dry run: This issue passed the CQ dry run.

[xinghua.cao, 2017-05-26 15:26:23]

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2905633002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:317:
ReadBackFramebuffer(pixels, Size().Width(), Size().Height(), kReadbackSkia,
On 2017/05/25 18:03:00, Zhenyao Mo wrote:
> On 2017/05/25 10:43:17, xinghua.cao wrote:
> > On 2017/05/24 20:53:30, Zhenyao Mo wrote:
> > > On 2017/05/24 20:40:04, Ken Russell wrote:
> > > > On 2017/05/24 20:31:01, Zhenyao Mo wrote:
> > > > > I think BindFramebuffer(_, fbo_) should be part of
> ReadBackFramebuffer().
> > > > 
> > > > Looking at the code, I'm not sure it can be.
> > > > DrawingBuffer::PaintRenderingResultsToImageData has the option of
reading
> > back
> > > > the front buffer, in which case a temporary FBO is allocated and bound.
> > > 
> > > kbr is right. Sorry about the noise. The code is fine as is.  Please
address
> > > capn's test change request.
> > 
> > Hi, all, I am confused how to design the test case for this patch in
> > DrawingBufferSoftwareRenderingTest.cpp. I do not know what is expected
result
> > for this case. Does we need to verify the framebuffer object currently is
> being
> > binded? Or the TextureMailbox object return by PrepareTextureMailbox? But I
> > think they cannot verify validity of this patch.
> > Do you have some good idea? Thank you. 
> 
> May check gl errors after PrepareTextureMailbox() call in the test would be
> enough?
> 
> Or you can check current fbo binding before and after the
> PrepareTextureMailbox() call and make sure they don't change?

I think it is more suitable to check the fbo binding.
And it is difficult to find rules to report GL_INVALID_OPERATION in readPixels,
which should be overrided DrawingBufferTestHelpers.h.

[Zhenyao Mo, 2017-05-26 22:38:35]

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:52:
gl_ = gl.get();
I don't think you should cache it here. Instead, just use
WebGraphicsContext3DProviderSoftwareRenderingForTests::ContextGL() for access.

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:68:
TEST_F(DrawingBufferSoftwareRenderingTest, bitmapRecycling) {
Can you fix this name? should be BitmapRecycling. Cap B.

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:106:
TEST_F(DrawingBufferSoftwareRenderingTest, framebufferBinding) {
I think you can just merge this test with the above one.

[xinghua.cao, 2017-05-27 08:57:36]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 08:57:49]

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, 2017-05-27 10:45:29]

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

[commit-bot: I haz the power, 2017-05-27 10:45:30]

Dry run: This issue passed the CQ dry run.

[xinghua.cao, 2017-06-01 02:26:51]

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:52:
gl_ = gl.get();
On 2017/05/26 22:38:34, Zhenyao Mo wrote:
> I don't think you should cache it here. Instead, just use
> WebGraphicsContext3DProviderSoftwareRenderingForTests::ContextGL() for access.

Line 58, After "std::move(provider)", it seems that provider is already null, so
the next parameter cannot use "provider->ContextGL()", is that right?

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:68:
TEST_F(DrawingBufferSoftwareRenderingTest, bitmapRecycling) {
On 2017/05/26 22:38:34, Zhenyao Mo wrote:
> Can you fix this name? should be BitmapRecycling. Cap B.

In webkit_unit_tests, the first letter of some cases is capital, others are
lowercase letter. I modify this case to capital here.

https://codereview.chromium.org/2905633002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:106:
TEST_F(DrawingBufferSoftwareRenderingTest, framebufferBinding) {
On 2017/05/26 22:38:34, Zhenyao Mo wrote:
> I think you can just merge this test with the above one. 

Hi, Zhenyao, I think there are little relationship between these two tests, if
merge them into one, I have no idea how to name this test.

[Ken Russell (switch to Gerrit), 2017-06-01 21:37:43]

Thanks Xinghua for putting this together and Mo for reviewing.

I defer to Mo's review; one comment about an earlier patch set. Let's get this
fix in as soon as possible; there seem to be multiple problems with this recent
change.

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:52:
gl_ = gl.get();
Agree with Mo's point that caching this weak pointer could lead to fragile code.
I do see however that several of the other tests are doing it this way.

Could you instead add to DrawingBufferForTests:

  GLES2InterfaceForTests* ContextGLForTests() {
    return static_cast<GLES2InterfaceForTests*>(drawing_buffer_->ContextGL());
  }

This assumes that all DrawingBufferForTests instances are set up with a
GLES2InterfaceForTests. I think that's a safe assumption. Then call this method
where the GLES2InterfaceForTests is needed. It would be nice to clean up the
other tests which are doing this caching as well.

[Zhenyao Mo, 2017-06-01 23:47:52]

I agree with kbr's suggestion.  Can you address that? Otherwise looks good.

[xinghua.cao, 2017-06-02 06:28:30]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 06:28: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, 2017-06-02 08:23:08]

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

[commit-bot: I haz the power, 2017-06-02 08:23:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xinghua.cao, 2017-06-02 11:13:14]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 11:13:53]

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, 2017-06-02 12:55:56]

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

[commit-bot: I haz the power, 2017-06-02 12:55:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xinghua.cao, 2017-06-02 13:38:11]

The CQ bit was checked by xinghua.cao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 13:38:29]

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, 2017-06-02 15:59:15]

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

[commit-bot: I haz the power, 2017-06-02 15:59:16]

Dry run: This issue passed the CQ dry run.

[xinghua.cao, 2017-06-02 16:06:29]

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:52:
gl_ = gl.get();
On 2017/06/01 21:37:43, Ken Russell wrote:
> Agree with Mo's point that caching this weak pointer could lead to fragile
code.
> I do see however that several of the other tests are doing it this way.
> 
> Could you instead add to DrawingBufferForTests:
> 
>   GLES2InterfaceForTests* ContextGLForTests() {
>     return static_cast<GLES2InterfaceForTests*>(drawing_buffer_->ContextGL());
>   }
> 
> This assumes that all DrawingBufferForTests instances are set up with a
> GLES2InterfaceForTests. I think that's a safe assumption. Then call this
method
> where the GLES2InterfaceForTests is needed. It would be nice to clean up the
> other tests which are doing this caching as well.

Hi, ken, Do you mean that add ContextGLForTests() function in
DrawingBufferForTests class? But DrawingBufferForTests does not have the
drawing_buffer_ member, I think here only need to call "ContextGL()" directly,
is that right?

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:57:
static_cast<GLES2InterfaceForTests*>(provider->ContextGL());
provider will been released after "std::move(provider)", so temporarily save gl_
here, and transfer it to DrawingBufferForTests::Create.

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
(right):

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h:417:
return static_cast<GLES2InterfaceForTests*>(ContextGL());
It seems that GLES2InterfaceForTests should be declared and defined before using
static_cast, so I move this class at the end of file. Is that right?

[Ken Russell (switch to Gerrit), 2017-06-02 21:52:50]

Thank you Xinghua, looks great. LGTM. CQ'ing for you.

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:57:
static_cast<GLES2InterfaceForTests*>(provider->ContextGL());
On 2017/06/02 16:06:28, xinghua.cao wrote:
> provider will been released after "std::move(provider)", so temporarily save
gl_
> here, and transfer it to DrawingBufferForTests::Create.

Understood. It might be desirable to clean up the ownership of this object more
later.

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h
(right):

https://codereview.chromium.org/2905633002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTestHelpers.h:417:
return static_cast<GLES2InterfaceForTests*>(ContextGL());
On 2017/06/02 16:06:29, xinghua.cao wrote:
> It seems that GLES2InterfaceForTests should be declared and defined before
using
> static_cast, so I move this class at the end of file. Is that right? 

Sounds good. This avoids any forward declarations. Thanks.

[Ken Russell (switch to Gerrit), 2017-06-02 21:54:14]

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp
(right):

https://codereview.chromium.org/2905633002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferSoftwareRenderingTest.cpp:52:
gl_ = gl.get();
On 2017/06/02 16:06:28, xinghua.cao wrote:
> On 2017/06/01 21:37:43, Ken Russell wrote:
> > Agree with Mo's point that caching this weak pointer could lead to fragile
> code.
> > I do see however that several of the other tests are doing it this way.
> > 
> > Could you instead add to DrawingBufferForTests:
> > 
> >   GLES2InterfaceForTests* ContextGLForTests() {
> >     return
static_cast<GLES2InterfaceForTests*>(drawing_buffer_->ContextGL());
> >   }
> > 
> > This assumes that all DrawingBufferForTests instances are set up with a
> > GLES2InterfaceForTests. I think that's a safe assumption. Then call this
> method
> > where the GLES2InterfaceForTests is needed. It would be nice to clean up the
> > other tests which are doing this caching as well.
> 
> Hi, ken, Do you mean that add ContextGLForTests() function in
> DrawingBufferForTests class? But DrawingBufferForTests does not have the
> drawing_buffer_ member, I think here only need to call "ContextGL()" directly,
> is that right?

Ah I misunderstood. Anyway, you fixed this in your most recent patch set --
thanks.

[Ken Russell (switch to Gerrit), 2017-06-02 21:54:21]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2017-06-02 21:54: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, 2017-06-02 22:00:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496440461023640,
"parent_rev": "e30b6cd4de5848da193845087bea38977bb2cb76", "commit_rev":
"f4f805f9fb47f434766195909c50a46b484ffdbb"}

[commit-bot: I haz the power, 2017-06-02 22:00:15]

Description was changed from

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=725579
TESTCASE=DrawingBufferSoftwareRenderingTest.framebufferBinding
==========

to

==========
Bind framebuffer to fbo_ object before readPixels, rather than multisample_fbo_

readPixels cannot get content from multisample_fbo_ directly, only from fbo_
whose
content is resolved from multisample_fbo_.

BUG=725579
TESTCASE=DrawingBufferSoftwareRenderingTest.framebufferBinding

Review-Url: https://codereview.chromium.org/2905633002
Cr-Commit-Position: refs/heads/master@{#476807}
Committed:
https://chromium.googlesource.com/chromium/src/+/f4f805f9fb47f434766195909c50...
==========

[commit-bot: I haz the power, 2017-06-02 22:00:16]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f4f805f9fb47f434766195909c50...