Added support for EXTDisjointTimerQuery on the Blink side.

Added support for EXTDisjointTimerQuery on the Blink side.

R=jochen@chromium.org, kbr@chromium.org
BUG=345227
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201806

[David Yen, 2015-09-03 17:24:25]

dyen@chromium.org changed reviewers:
+ chrishtr@chromium.org, dglazkov@chromium.org, japhet@chromium.org

[Ken Russell (switch to Gerrit), 2015-09-03 17:51:08]

kbr@chromium.org changed reviewers:
+ bajones@chromium.org, zmo@chromium.org

[dglazkov, 2015-09-03 20:43:47]

On 2015/09/03 at 17:51:09, kbr wrote:
> 

Intent to implement? :)

[Ken Russell (switch to Gerrit), 2015-09-03 21:01:00]

On 2015/09/03 20:43:47, dglazkov wrote:
> On 2015/09/03 at 17:51:09, kbr wrote:
> > 
> 
> Intent to implement? :)

Sent to blink-dev.

[dglazkov, 2015-09-03 21:02:22]

On 2015/09/03 at 21:01:00, kbr wrote:
> On 2015/09/03 20:43:47, dglazkov wrote:
> > On 2015/09/03 at 17:51:09, kbr wrote:
> > > 
> > 
> > Intent to implement? :)
> 
> Sent to blink-dev.

LGTM.

[bajones, 2015-09-03 22:00:02]

bajones@chromium.org changed reviewers:
+ haraken@chromium.org

[bajones, 2015-09-03 22:00:02]

modules/webgl LGTM modulo one critical change. Please get haraken@ or another
member of the Oilpan team to review the new uses of Oilpan.

https://codereview.chromium.org/1325453007/diff/60001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1325453007/diff/60001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContext.cpp:116:
registerExtension<EXTDisjointTimerQuery>(m_extDisjointTimerQuery);
This needs to be registered as a draft extension. Just change the call to:

registerExtension<EXTDisjointTimerQuery>(m_extDisjointTimerQuery,
DraftExtension);

[David Yen, 2015-09-03 22:39:57]

Will wait for LGTM from kbr@ over the implementation on EXTDisjointTimerQuery
based on the spec, and from haraken@ on Oilpan usage.

https://codereview.chromium.org/1325453007/diff/60001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1325453007/diff/60001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContext.cpp:116:
registerExtension<EXTDisjointTimerQuery>(m_extDisjointTimerQuery);
On 2015/09/03 22:00:02, bajones wrote:
> This needs to be registered as a draft extension. Just change the call to:
> 
> registerExtension<EXTDisjointTimerQuery>(m_extDisjointTimerQuery,
> DraftExtension);

Done.

[haraken, 2015-09-03 23:32:58]

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLTimerQueryEXT.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLTimerQueryEXT.cpp:34:
context()->webContext()->deleteQueryEXT(m_queryId);

This looks unsafe. deleteObjectImpl() is called in a destructor of
WebGLTimerQueryEXT and touches context(), which is an on-heap object. A
destructor is not allowed to touch other on-heap objects.

Can we change the code to context3D->deleteQueryEXT(m_queryId)?

[Ken Russell (switch to Gerrit), 2015-09-04 01:17:09]

Excellent work pushing this through. LGTM overall. Couple of minor issues and a
comment on haraken@'s review.

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:2872: return
ScriptValue::createNull(scriptState);
This should generate an INVALID_ENUM error if the extension isn't enabled; see
MAX_DRAW_BUFFERS_EXT above for a similar example.

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:2876: return
ScriptValue::createNull(scriptState);
Same thing here.

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLTimerQueryEXT.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLTimerQueryEXT.cpp:34:
context()->webContext()->deleteQueryEXT(m_queryId);
On 2015/09/03 23:32:58, haraken wrote:
> 
> This looks unsafe. deleteObjectImpl() is called in a destructor of
> WebGLTimerQueryEXT and touches context(), which is an on-heap object. A
> destructor is not allowed to touch other on-heap objects.
> 
> Can we change the code to context3D->deleteQueryEXT(m_queryId)?

This is a good point and should be done, but it looks like there's a further,
similar bug in WebGLContextObject::getAWebGraphicsContext3D. haraken@: should
that class be changed to cache the WebGraphicsContext3D* pointer? If David fixes
this case here, can I fix that in a follow-on patch?

[haraken, 2015-09-04 01:30:13]

LGTM

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLTimerQueryEXT.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLTimerQueryEXT.cpp:34:
context()->webContext()->deleteQueryEXT(m_queryId);
On 2015/09/04 01:17:09, Ken Russell wrote:
> On 2015/09/03 23:32:58, haraken wrote:
> > 
> > This looks unsafe. deleteObjectImpl() is called in a destructor of
> > WebGLTimerQueryEXT and touches context(), which is an on-heap object. A
> > destructor is not allowed to touch other on-heap objects.
> > 
> > Can we change the code to context3D->deleteQueryEXT(m_queryId)?
> 
> This is a good point and should be done, but it looks like there's a further,
> similar bug in WebGLContextObject::getAWebGraphicsContext3D. haraken@: should
> that class be changed to cache the WebGraphicsContext3D* pointer? If David
fixes
> this case here, can I fix that in a follow-on patch?

In conclusion, I realized that this code is safe for a complicated reason below.
Either way, it would be better to use the passed-in context3D for consistency
with other WebGLObject::deleteObjectImpls.

a) If the WebGL object is finalized before the context object, the
deleteObjectImpl gets called. Since the context object is still alive, it is
safe to touch the context object in the deleteObjectImpl.

b) If the context object is finalized before the WebGL object, the
deleteObjectImpl isn't called (See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
So we won't reach deleteObjectImpl in this case.

Sigbjorn: We should have a good comment about the lifetime relationship between
WebGLObjects and the context object.

[chrishtr, 2015-09-04 15:38:01]

lgtm

[David Yen, 2015-09-04 16:04:42]

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:2872: return
ScriptValue::createNull(scriptState);
On 2015/09/04 01:17:09, Ken Russell wrote:
> This should generate an INVALID_ENUM error if the extension isn't enabled; see
> MAX_DRAW_BUFFERS_EXT above for a similar example.

Done.

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLRenderingContextBase.cpp:2876: return
ScriptValue::createNull(scriptState);
On 2015/09/04 01:17:09, Ken Russell wrote:
> Same thing here.

Done.

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
File Source/modules/webgl/WebGLTimerQueryEXT.cpp (right):

https://codereview.chromium.org/1325453007/diff/80001/Source/modules/webgl/We...
Source/modules/webgl/WebGLTimerQueryEXT.cpp:34:
context()->webContext()->deleteQueryEXT(m_queryId);
On 2015/09/04 01:30:13, haraken wrote:
> On 2015/09/04 01:17:09, Ken Russell wrote:
> > On 2015/09/03 23:32:58, haraken wrote:
> > > 
> > > This looks unsafe. deleteObjectImpl() is called in a destructor of
> > > WebGLTimerQueryEXT and touches context(), which is an on-heap object. A
> > > destructor is not allowed to touch other on-heap objects.
> > > 
> > > Can we change the code to context3D->deleteQueryEXT(m_queryId)?
> > 
> > This is a good point and should be done, but it looks like there's a
further,
> > similar bug in WebGLContextObject::getAWebGraphicsContext3D. haraken@:
should
> > that class be changed to cache the WebGraphicsContext3D* pointer? If David
> fixes
> > this case here, can I fix that in a follow-on patch?
> 
> In conclusion, I realized that this code is safe for a complicated reason
below.
> Either way, it would be better to use the passed-in context3D for consistency
> with other WebGLObject::deleteObjectImpls.
> 
> a) If the WebGL object is finalized before the context object, the
> deleteObjectImpl gets called. Since the context object is still alive, it is
> safe to touch the context object in the deleteObjectImpl.
> 
> b) If the context object is finalized before the WebGL object, the
> deleteObjectImpl isn't called (See
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> So we won't reach deleteObjectImpl in this case.
> 
> Sigbjorn: We should have a good comment about the lifetime relationship
between
> WebGLObjects and the context object.
> 

Done.

[David Yen, 2015-09-04 16:08:04]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-09-04 16:08:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, dglazkov@chromium.org, kbr@chromium.org,
chrishtr@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1325453007/#ps100001
(title: "GL_INVALID_ENUM when timer query not enabled for GetParameter, use
context3d for deletion")

[commit-bot: I haz the power, 2015-09-04 16:08:18]

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

[commit-bot: I haz the power, 2015-09-04 17:23:27]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201806