Make lost context callback a base::Closure thru the WGC3DProvider.

Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6d1bfc385d5f190c3b37e8df0793ae75af5ae6f5
Cr-Commit-Position: refs/heads/master@{#383233}

[danakj, 2016-03-24 02:32:15]

Description was changed from

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
==========

to

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-03-24 02:32:44]

Description was changed from

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-03-24 02:32:44]

danakj@chromium.org changed reviewers:
+ chrishtr@chromium.org

[danakj, 2016-03-24 02:35:30]

piman/kbr: please review. what do you think?

This makes it a function on the provider instead of introducing a
WebContextSupport or something. But it's already a method on the real
ContextProvider so this matches, and I think that probably will stay since the
OnLostContext handler needs to kill share groups, and I don't think anything
above the ContextProvider is going to be aware of them.

The OnError function tho can go directly from the GLES2Impl, so that could go on
gpu::ContextSupport, are you ok with that inconsistency? Or should one of the 2
move?

kinuko: please review public/, are you okay with these DEPS rules?

chrishtr: please review public/, are you ok with the new directory?

[danakj, 2016-03-24 02:35:44]

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

[commit-bot: I haz the power, 2016-03-24 02:35:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830033003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830033003/1

[commit-bot: I haz the power, 2016-03-24 02:51:03]

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

[commit-bot: I haz the power, 2016-03-24 02:51:04]

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

[piman, 2016-03-24 03:41:43]

LGTM for content/ and gpu/

[kinuko, 2016-03-24 05:55:56]

yup, the deps and bind usage lgtm

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/DEPS (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/DEPS:2: "+base/bind.h",
nit: maybe have a short comment to note we allow these only in limited
subdirectories for now like this

[chrishtr, 2016-03-24 15:10:05]

chrishtr@chromium.org changed reviewers:
+ esprehn@chromium.org

[chrishtr, 2016-03-24 15:10:06]

+esprehn for review of WebClosure.

[dcheng, 2016-03-24 15:53:56]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-03-24 15:53:57]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:45: m_haveClosure =
other.m_haveClosure;
Should m_have closure be set to false?

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:46: m_closure =
base::ResetAndReturn(&other.m_closure);
std::move() this too?

[danakj, 2016-03-24 17:53:51]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:45: m_haveClosure =
other.m_haveClosure;
On 2016/03/24 15:53:56, dcheng wrote:
> Should m_have closure be set to false?

But what if the |other| had a closure, now this one does?

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:46: m_closure =
base::ResetAndReturn(&other.m_closure);
On 2016/03/24 15:53:56, dcheng wrote:
> std::move() this too?

But it's an rvalue no?

[danakj, 2016-03-24 17:59:21]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/DEPS (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/DEPS:2: "+base/bind.h",
On 2016/03/24 05:55:56, kinuko wrote:
> nit: maybe have a short comment to note we allow these only in limited
> subdirectories for now like this

Done.

[dcheng, 2016-03-24 18:00:18]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:45: m_haveClosure =
other.m_haveClosure;
On 2016/03/24 at 17:53:51, danakj wrote:
> On 2016/03/24 15:53:56, dcheng wrote:
> > Should m_have closure be set to false?
> 
> But what if the |other| had a closure, now this one does?

Sorry, to clarify, I meant other.m_haveClosure should become false.

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:46: m_closure =
base::ResetAndReturn(&other.m_closure);
On 2016/03/24 at 17:53:51, danakj wrote:
> On 2016/03/24 15:53:56, dcheng wrote:
> > std::move() this too?
> 
> But it's an rvalue no?

Not sure what you mean. I believe that
  m_closure = std::move(other.m_closure);
should be equivalent to what's currently written, no?

[danakj, 2016-03-24 18:03:16]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:45: m_haveClosure =
other.m_haveClosure;
On 2016/03/24 18:00:18, dcheng wrote:
> On 2016/03/24 at 17:53:51, danakj wrote:
> > On 2016/03/24 15:53:56, dcheng wrote:
> > > Should m_have closure be set to false?
> > 
> > But what if the |other| had a closure, now this one does?
> 
> Sorry, to clarify, I meant other.m_haveClosure should become false.

Oh oh ok!

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:46: m_closure =
base::ResetAndReturn(&other.m_closure);
On 2016/03/24 18:00:18, dcheng wrote:
> On 2016/03/24 at 17:53:51, danakj wrote:
> > On 2016/03/24 15:53:56, dcheng wrote:
> > > std::move() this too?
> > 
> > But it's an rvalue no?
> 
> Not sure what you mean. I believe that
>   m_closure = std::move(other.m_closure);
> should be equivalent to what's currently written, no?

OOh, I see. Does callback null itself when you move from it? I figured it
didn't. *reads*

[danakj, 2016-03-24 18:06:05]

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/callback/WebClosure.h:46: m_closure =
base::ResetAndReturn(&other.m_closure);
On 2016/03/24 18:03:16, danakj wrote:
> On 2016/03/24 18:00:18, dcheng wrote:
> > On 2016/03/24 at 17:53:51, danakj wrote:
> > > On 2016/03/24 15:53:56, dcheng wrote:
> > > > std::move() this too?
> > > 
> > > But it's an rvalue no?
> > 
> > Not sure what you mean. I believe that
> >   m_closure = std::move(other.m_closure);
> > should be equivalent to what's currently written, no?
> 
> OOh, I see. Does callback null itself when you move from it? I figured it
> didn't. *reads*

It totes does. Done.

[danakj, 2016-03-24 18:06:49]

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

[commit-bot: I haz the power, 2016-03-24 18:07:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830033003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830033003/40001

[Ken Russell (switch to Gerrit), 2016-03-24 18:29:04]

LGTM. Thanks for thinking this through so carefully. A few minor questions.

https://codereview.chromium.org/1830033003/diff/60001/content/renderer/webgra...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/1830033003/diff/60001/content/renderer/webgra...
content/renderer/webgraphicscontext3d_provider_impl.cc:32: blink::WebClosure c)
{
Just to clarify: is the plan eventually to remove WebGraphicsContext3DProvider
entirely?

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

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:496: class
WebGLRenderingContextLostCallback final : public
GarbageCollectedFinalized<WebGLRenderingContextLostCallback>, public
WebGraphicsContext3D::WebGraphicsContextLostCallback {
Can this class be removed now?

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/callback/WebClosure.h:34: auto run_and_delete
= +[](scoped_ptr<SameThreadClosure> c)
Is the "+" strictly needed to avoid compile errors?
http://stackoverflow.com/questions/17822131/resolving-ambiguous-overload-on-f...

[danakj, 2016-03-24 18:36:50]

https://codereview.chromium.org/1830033003/diff/60001/content/renderer/webgra...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/1830033003/diff/60001/content/renderer/webgra...
content/renderer/webgraphicscontext3d_provider_impl.cc:32: blink::WebClosure c)
{
On 2016/03/24 18:29:04, Ken Russell wrote:
> Just to clarify: is the plan eventually to remove WebGraphicsContext3DProvider
> entirely?

Yeah. Ideally we just use cc::ContextProvider (I think it's moving to
gpu::ContextProvider). If we're not able to do that, we'll either typedef or
continue to wrap it in platform (which is what this is now).

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

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:496: class
WebGLRenderingContextLostCallback final : public
GarbageCollectedFinalized<WebGLRenderingContextLostCallback>, public
WebGraphicsContext3D::WebGraphicsContextLostCallback {
On 2016/03/24 18:29:04, Ken Russell wrote:
> Can this class be removed now?

Oh, ya! Thanks

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/callback/WebClosure.h:34: auto run_and_delete
= +[](scoped_ptr<SameThreadClosure> c)
On 2016/03/24 18:29:04, Ken Russell wrote:
> Is the "+" strictly needed to avoid compile errors?
>
http://stackoverflow.com/questions/17822131/resolving-ambiguous-overload-on-f...

The + let's bind work. Bind won't let you use a lambda, but it can take a
function pointer.

[danakj, 2016-03-24 18:38:09]

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/callback/WebClosure.h:34: auto run_and_delete
= +[](scoped_ptr<SameThreadClosure> c)
On 2016/03/24 18:36:50, danakj wrote:
> On 2016/03/24 18:29:04, Ken Russell wrote:
> > Is the "+" strictly needed to avoid compile errors?
> >
>
http://stackoverflow.com/questions/17822131/resolving-ambiguous-overload-on-f...
> 
> The + let's bind work. Bind won't let you use a lambda, but it can take a
> function pointer.

But apparently does not work in MSVC. Cool.

[danakj, 2016-03-24 18:40:39]

Updated without lambda, and deleted the dead class.

[Ken Russell (switch to Gerrit), 2016-03-24 18:59:02]

Awesome, still LGTM.

[danakj, 2016-03-24 22:59:53]

So.. I'm pretty confused by the new ContextLost.WebGLContextLostFromQuantity
failure.

Here's what I see happening:

1. The test makes a context 0x3254a5142f08 and marks it active.
2. The test makes a whole lot more contexts and marks them active. They start
evicting older contexts. For each time this happens it looks like:
  - activateContext(A)
    - Sees too many active contexts.
    - forciblyLoseOldestContext(B)
    - deactivateContext(B)
    - removes B from activeContexts(), making the list size <
maxGLActiveContexts
    - adds A to activeContexts(), making the list size == maxGLActiveContexts
3. Eventually 0x3ebb608c2f08 is deactivated/lost and put in the evicted list.
4. Continues for a long time... throwing a lot of stuff into the evicted list,
eventually we have 113 contexts in evicted list.
5. Now we have activeContexts() with maxGLActiveContexts things.
6. GC happens, and evicted WebGL contexts start getting destroyed, but each time
they get to the willDestroyContext(), they see that we have too many
activeContexts() to try restore any.


I didn't change anything in the above, so I'm rather confused how it worked
before, I guess I'll add prints without this patch now.

[danakj, 2016-03-24 23:09:46]

Before this patch, when willDestroyContext happens, the activeContexts set is
empty.

It appears that OilPan magic deletes stuff in both activeContexts() and
forciblyEvictedContexts() before willDestroyContext() happens for any GL
context.

After this patch OilPan magic deletes stuff only in the evictedContexts list.

Which means, somehow active contexts are getting GC'd before this patch?

[danakj, 2016-03-24 23:12:56]

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

https://codereview.chromium.org/1830033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:978:
drawingBuffer()->contextProvider()->setLostContextCallback(WebClosure(WTF::bind(&WebGLRenderingContextBase::forceLostContext,
this, WebGLRenderingContextBase::RealLostContext,
WebGLRenderingContextBase::Auto)));
Maybe this bind is keeping things alive from being collected?

[dcheng, 2016-03-24 23:19:47]

On 2016/03/24 at 23:09:46, danakj wrote:
> Before this patch, when willDestroyContext happens, the activeContexts set is
empty.
> 
> It appears that OilPan magic deletes stuff in both activeContexts() and
forciblyEvictedContexts() before willDestroyContext() happens for any GL
context.
> 
> After this patch OilPan magic deletes stuff only in the evictedContexts list.
> 
> Which means, somehow active contexts are getting GC'd before this patch?

Hmm, does Oilpan understand how to trace into WTF::bind to make sure objects
bound into SameThreadClosure stay alive? I don't see anything in particular in
WTF::bind that indicates it would support this, and WebGLRenderingContextBase
appears to be GCed. So I guess you would have to bind an Oilpan smart pointer
(probably persistent?) for this to work. Looking at the code, that appears to be
what createWeakThisPointer() does.

[danakj, 2016-03-24 23:35:05]

using createWeakThisPointer() does in fact allow oilpan to destroy the active
contexts before they are lost (aka before we destroy the SameThreadClosure that
has |this| bound into it).

I still fail to find how on earth oilpan is finding the reference to |this| in
there when the SameThreadClosure is inside a base::Callback owned by
content::ContextProviderCommandBuffer.

[Ken Russell (switch to Gerrit), 2016-03-24 23:42:35]

On 2016/03/24 23:35:05, danakj wrote:
> using createWeakThisPointer() does in fact allow oilpan to destroy the active
> contexts before they are lost (aka before we destroy the SameThreadClosure
that
> has |this| bound into it).
> 
> I still fail to find how on earth oilpan is finding the reference to |this| in
> there when the SameThreadClosure is inside a base::Callback owned by
> content::ContextProviderCommandBuffer.

Hmm. WebGLRenderingContextBase traces m_contextLostCallbackAdapter, which turns
around and traces WebGLRenderingContextBase?

Oilpan conservatively traces stack frames, plus known roots. Is the pointer in
question active on the stack or is it being registered into a root Oilpan knows
about?

Is there a preexisting race condition in the earlier code?

[Ken Russell (switch to Gerrit), 2016-03-24 23:43:13]

+haraken and sigbjornf in case either has ideas about this strange behavior.

[danakj, 2016-03-24 23:48:39]

On 2016/03/24 23:43:13, Ken Russell wrote:
> +haraken and sigbjornf in case either has ideas about this strange behavior.

Elliot figured it out.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

ParamStorageTraits for garbage collected things will make a
CrossThreadPersistent in the SameThreadClosure.

[esprehn, 2016-03-25 01:05:06]

lgtm w/ nits fixed.

https://codereview.chromium.org/1830033003/diff/120001/content/renderer/webgr...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/1830033003/diff/120001/content/renderer/webgr...
content/renderer/webgraphicscontext3d_provider_impl.cc:33:
provider_->SetLostContextCallback(c.ToBaseClosure());
TakeBaseClosure() ? It has side effects.

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/callback/WebClosure.h:43: other.m_haveClosure
= false;
ditto, guard the m_haveClosure code with debug ifdef

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/callback/WebClosure.h:64: bool m_haveClosure
= true;
ifdef this to debug only, the boolean is only used for asserts.

[danakj, 2016-03-25 01:41:54]

Thanks!

https://codereview.chromium.org/1830033003/diff/120001/content/renderer/webgr...
File content/renderer/webgraphicscontext3d_provider_impl.cc (right):

https://codereview.chromium.org/1830033003/diff/120001/content/renderer/webgr...
content/renderer/webgraphicscontext3d_provider_impl.cc:33:
provider_->SetLostContextCallback(c.ToBaseClosure());
On 2016/03/25 01:05:05, esprehn wrote:
> TakeBaseClosure() ? It has side effects.

Done.

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/callback/WebClosure.h (right):

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/callback/WebClosure.h:43: other.m_haveClosure
= false;
On 2016/03/25 01:05:06, esprehn wrote:
> ditto, guard the m_haveClosure code with debug ifdef

Done.

https://codereview.chromium.org/1830033003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/callback/WebClosure.h:64: bool m_haveClosure
= true;
On 2016/03/25 01:05:05, esprehn wrote:
> ifdef this to debug only, the boolean is only used for asserts.

Done, to DCHECK_IS_ON().

[danakj, 2016-03-25 01:42:22]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-03-25 01:42:22]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
kinuko@chromium.org, kbr@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1830033003/#ps140001
(title: "lostcontext-webclosure: public-review")

[commit-bot: I haz the power, 2016-03-25 01:42:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830033003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830033003/140001

[haraken, 2016-03-25 02:14:02]

LGTM

[commit-bot: I haz the power, 2016-03-25 03:12:12]

Description was changed from

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-03-25 03:12:16]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-25 03:13:50]

Description was changed from

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make lost context callback a base::Closure thru the WGC3DProvider.

This makes the WebGraphicsContext3DProvider have a method to set a lost
context callback, and has that method take a SameThreadClosure, wrapped
inside a WebClosure.

The SameThreadClosure is converted to a base::Closure and passed thru
by the WebGraphicsContext3DProviderImpl (which lives in content/) to
the actual ContextProvider::SetLostContextCallback() method. This skips
going through WebGraphicsContext3D in blink for the callback, though
internally the provider does call the same method on
WebGraphicsContext3DImpl, which blink used to hijack from it later.

This removes the setter method from the WebGraphicsContext3D public API
exposed to blink.

It introduces a new public/platform/callback/ directory for the
WebClosure in order to make separate DEPS rules for this class. It
makes use of base::Bind/Callback which has previously not been allowed
in blink, though only to wrap a WTF::SameThreadClosure.

It also makes use of base/logging.h directly (previously we were unable
to use DCHECK in public/platform since the files there build both
with and without BLINK_IMPLEMENTATION and in the without case wtf/ is
not available to be included.

It also uses scoped_ptr since that is nicer than raw pointers and
base::Callback understands it but doesn't understand OwnPtr.

R=kbr@chromium.org, kinuko@chromium.org, piman@chromium.org
BUG=584497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/6d1bfc385d5f190c3b37e8df0793ae75af5ae6f5
Cr-Commit-Position: refs/heads/master@{#383233}
==========

[commit-bot: I haz the power, 2016-03-25 03:13:52]

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