Implement OffscreenCanvas.getContext('webgl')

Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.

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

Committed: https://crrev.com/a7270ac8f1b15170a6b28b5ebf02c65beaa6374e
Cr-Commit-Position: refs/heads/master@{#390368}

[xidachen, 2016-04-11 19:20:38]

Description was changed from

==========
Implement OffscreenCanvas.getContext('webgl')

BUG=602391
==========

to

==========
Implement OffscreenCanvas.getContext('webgl')

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

[xidachen, 2016-04-20 19:27:55]

Description was changed from

==========
Implement OffscreenCanvas.getContext('webgl')

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

to

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.

In this CL, we make WebGLRenderingContextBase inherited from 
OffscreenCanvasRenderingContext. That is the only inheritance
change to the WebGL classes.

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

[xidachen, 2016-04-20 19:27:55]

xidachen@chromium.org changed reviewers:
+ ajuma@chromium.org, bajones@chromium.org, junov@chromium.org,
kbr@chromium.org, xlai@chromium.org, zmo@chromium.org

[xidachen, 2016-04-20 19:31:10]

PTAL.

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:519:
contextAttributes, scriptState->getExecutionContext()->url(), 0, &glInfo));
Doing offscreenCanvas.getContext('webgl') works fine on main thread, but crashes
when it is on a worker thread. It crashes at this point, and glInfo.errorMessage
says: Failed to run in Current RenderThreadImpl
This error message comes from this statement in
"renderer_blink_platform_impl.cc":

if (!RenderThreadImpl::current())

kbr@, zmo@, bajones@: Do you know why this returns nullptr in worker but not
main thread.

[Justin Novosad, 2016-04-20 20:07:28]

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D-in-worker.html
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D-in-worker.html:15:
self.postMessage("bCanvas.getContext('webgl') does not return [object
WebGLRenderingContext]");
test is called "getContext2D" but it covers webgl too.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:36:
var ctx4 = bCanvas.getContext("webgl");
Hmmm... test is called "getContext2D" but it covers webgl too.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl:9:
WebGL2RenderingContext) OffscreenRenderingContext;
WebGL2RenderingContext creation is not covered by layout tests.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasRenderingContext.h
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasRenderingContext.h:15:
class MODULES_EXPORT OffscreenCanvasRenderingContext : public
GarbageCollectedMixin {
Thanks for figuring this out.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasRenderingContext.h:28:
virtual ContextType getOffscreenCanvasRenderingContextType() const = 0;
Why the name change?

[xlai (Olivia), 2016-04-20 20:42:16]

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl:18:
[CallWith=ScriptState] OffscreenRenderingContext? getContext(DOMString
contextId, optional CanvasContextCreationAttributes attributes);
I can see that the only reason that you added [CallWith=ScriptState] here (and
one more new parameter to many corresponding functions in Factory, 2D, Webgl
etc) is to eventually use the ExecutionState in
WebGLRenderingContextBase::createWebGraphicsContext3DProviderHelper. But I think
you can obtain ScriptState* by just using
ScriptState::current(v8::Isolate::GetCurrent()). (Example:
WebDocument::registerEmbedderCustomElement()).
Otherwise, you can also change to CallWith=ExecutionContext.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h:16:
class MODULES_EXPORT OffscreenCanvasRenderingContext2D final : public
GarbageCollectedFinalized<OffscreenCanvasRenderingContext2D>, public
OffscreenCanvasRenderingContext, public BaseRenderingContext2D, public
ScriptWrappable {
Good change to move the ScriptWrappable. So now only the specific rendering
contexts (2d, webgl) are explicitly exposed in scripts, instead of staying in
generic renderingcontext class.

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:145: class
MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext, public
OffscreenCanvasRenderingContext, public ScriptWrappable {
[continued from previous comment] But then why is ScriptWrappable in WebGLBase,
instead of being in WebGL2 and WebGL separately? Are they sharing the same
interface in scripts?

[xidachen, 2016-04-20 20:52:09]

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext2D.html:36:
var ctx4 = bCanvas.getContext("webgl");
On 2016/04/20 20:07:27, Justin Novosad wrote:
> Hmmm... test is called "getContext2D" but it covers webgl too.

Good catch, this will be changed in a future CL once I fix crash.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl:9:
WebGL2RenderingContext) OffscreenRenderingContext;
On 2016/04/20 20:07:27, Justin Novosad wrote:
> WebGL2RenderingContext creation is not covered by layout tests.

I think we should start with small CLs, so this CL implements
getContext('webgl') only, and getContext('webgl2') is not implemented. Since
webgl2 is still experimental, I think we can leave it until webgl API works.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvas.idl:18:
[CallWith=ScriptState] OffscreenRenderingContext? getContext(DOMString
contextId, optional CanvasContextCreationAttributes attributes);
On 2016/04/20 20:42:16, xlai (Olivia) wrote:
> I can see that the only reason that you added [CallWith=ScriptState] here (and
> one more new parameter to many corresponding functions in Factory, 2D, Webgl
> etc) is to eventually use the ExecutionState in
> WebGLRenderingContextBase::createWebGraphicsContext3DProviderHelper. But I
think
> you can obtain ScriptState* by just using
> ScriptState::current(v8::Isolate::GetCurrent()). (Example:
> WebDocument::registerEmbedderCustomElement()).
> Otherwise, you can also change to CallWith=ExecutionContext.

Thank you for the suggestion, will do that.

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasRenderingContext.h
(right):

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/offscreencanvas/OffscreenCanvasRenderingContext.h:28:
virtual ContextType getOffscreenCanvasRenderingContextType() const = 0;
On 2016/04/20 20:07:27, Justin Novosad wrote:
> Why the name change?

CanvasRenderingContext also has a method getContextType(), and
WebGLRenderingContext gets confused because it inherits from both
CanvasRenderingContext and OffscreenCanvasRenderingContext.

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:145: class
MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext, public
OffscreenCanvasRenderingContext, public ScriptWrappable {
On 2016/04/20 20:42:16, xlai (Olivia) wrote:
> [continued from previous comment] But then why is ScriptWrappable in
WebGLBase,
> instead of being in WebGL2 and WebGL separately? Are they sharing the same
> interface in scripts?

WebGLBase has a idl and it is the actual interface for WebGLRenderingContext.
WebGL2RenderingContext just extend the WebGLBase.

[Ken Russell (switch to Gerrit), 2016-04-21 01:05:24]

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:145: class
MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext, public
OffscreenCanvasRenderingContext, public ScriptWrappable {
Based on past experience with multiple inheritance I think that making
WebGLRenderingContextBase inherit from both CanvasRenderingContext and
OffscreenCanvasRenderingContext is not a good direction to take.

The introduction of OffscreenCanvasRenderingContext as a concept does not appear
necessary to me. All that needs to happen is:

 - The few methods that deal with the HTMLCanvasElement in
CanvasRenderingContext2D, WebGLRenderingContext, etc. and their base classes
need to be considered optional.

 - Methods need to be added to the base class (CanvasRenderingContext) allowing
it to be used with OffscreenCanvas.

 - CanvasRenderingContextFactory needs to be extended with a new factory method
taking OffscreenCanvas instead of HTMLCanvasElement as the owner of the context.

Fundamentally, the context classes will have to understand their two modes of
operation -- rendering to either an on-screen or off-screen canvas. Using
multiple inheritance to achieve this is deeply problematic. Pointers to the
objects change value depending on which area of the code is touching it.
Accidental diamond inheritance causes behaviors that can't be easily explained.
Interaction with the JavaScript layer is made more confusing. There are many
other reasons for preferring single inheritance.

Please make this change. I think that using multiple inheritance for the WebGL
contexts is not a good design decision and I can't support moving the code in
this direction.

[xlai (Olivia), 2016-04-21 15:31:54]

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:145: class
MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext, public
OffscreenCanvasRenderingContext, public ScriptWrappable {
> The introduction of OffscreenCanvasRenderingContext as a concept does not
appear
> necessary to me. All that needs to happen is:
> 
>  - The few methods that deal with the HTMLCanvasElement in
> CanvasRenderingContext2D, WebGLRenderingContext, etc. and their base classes
> need to be considered optional.
> 
>  - Methods need to be added to the base class (CanvasRenderingContext)
allowing
> it to be used with OffscreenCanvas.
> 
>  - CanvasRenderingContextFactory needs to be extended with a new factory
method
> taking OffscreenCanvas instead of HTMLCanvasElement as the owner of the
context.
> 

First, I agree with merging OffscreenCanvasRenderingContext and
CanvasRenderingContext into one class (so does the merging of non-offscreen and
offscreen factory classes into one). It is also possible to implement by:
- Adding one more create function in Factory that takes in OffscreenCanvas* and
returns CanvasRenderingContext*. This create function is invoked by
OffscreenCanvas.getContext() only.
- From what I see, CanvasRenderingContext only interacts with HTMLCanvasElement
by having a member instance m_canvas; we can set it null in the case of
OffscreenCanvas and have one more member instance called m_offscreencanvas.
- Adding all out offscreen context types to the list of enum in
CanvasRenderingContext.ContextType with prefix "Offscreen" to distinguish them
with the existing rendering context types.

Second, I disagree with changing the status quo in
OffscreenCanvasRenderingContext2D and CanvasRenderingContext2D. I think attempts
to merge these two classes will make things messy. Right now,
BaseRenderingContext2D is parent of 3 classes--PaintRenderingContext2D,
CanvasRenderingContext2D, OffscreenCanvasRenderingContext2D. It is problematic
to adapt BaseRC2D for only the canvas cases, without affecting the PaintRC2D.
Also, there is no reason to let OffCRC2D and CRC2D to become one class, as many
functions that solely exist in CRC2D (hitregion, focus) are not applicable to
OffscreenCanvas.

[Ken Russell (switch to Gerrit), 2016-04-21 19:52:01]

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

https://codereview.chromium.org/1881563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:145: class
MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext, public
OffscreenCanvasRenderingContext, public ScriptWrappable {
On 2016/04/21 15:31:54, xlai (Olivia) wrote:
> > The introduction of OffscreenCanvasRenderingContext as a concept does not
> appear
> > necessary to me. All that needs to happen is:
> > 
> >  - The few methods that deal with the HTMLCanvasElement in
> > CanvasRenderingContext2D, WebGLRenderingContext, etc. and their base classes
> > need to be considered optional.
> > 
> >  - Methods need to be added to the base class (CanvasRenderingContext)
> allowing
> > it to be used with OffscreenCanvas.
> > 
> >  - CanvasRenderingContextFactory needs to be extended with a new factory
> method
> > taking OffscreenCanvas instead of HTMLCanvasElement as the owner of the
> context.
> > 
> 
> First, I agree with merging OffscreenCanvasRenderingContext and
> CanvasRenderingContext into one class (so does the merging of non-offscreen
and
> offscreen factory classes into one). It is also possible to implement by:
> - Adding one more create function in Factory that takes in OffscreenCanvas*
and
> returns CanvasRenderingContext*. This create function is invoked by
> OffscreenCanvas.getContext() only.
> - From what I see, CanvasRenderingContext only interacts with
HTMLCanvasElement
> by having a member instance m_canvas; we can set it null in the case of
> OffscreenCanvas and have one more member instance called m_offscreencanvas.
> - Adding all out offscreen context types to the list of enum in
> CanvasRenderingContext.ContextType with prefix "Offscreen" to distinguish them
> with the existing rendering context types.
> 
> Second, I disagree with changing the status quo in
> OffscreenCanvasRenderingContext2D and CanvasRenderingContext2D. I think
attempts
> to merge these two classes will make things messy. Right now,
> BaseRenderingContext2D is parent of 3 classes--PaintRenderingContext2D,
> CanvasRenderingContext2D, OffscreenCanvasRenderingContext2D. It is problematic
> to adapt BaseRC2D for only the canvas cases, without affecting the PaintRC2D.
> Also, there is no reason to let OffCRC2D and CRC2D to become one class, as
many
> functions that solely exist in CRC2D (hitregion, focus) are not applicable to
> OffscreenCanvas.
> 

OK. Please do as you see fit with CanvasRenderingContext2D and
OffscreenCanvasRenderingContext2D, but please merge the base class so that
CanvasRenderingContext is the sole base class for all of the derived classes.
Thanks.

Also please note that https://wiki.whatwg.org/wiki/OffscreenCanvas doesn't
define an OffscreenCanvasRenderingContext2D type. The behavior of these methods
that make no sense on OffscreenCanvases needs to be specified, or a
CanvasRenderingContext2DBase needs to be defined with the common methods, and
CanvasRenderingContext2D specified to add the methods that only make sense with
on-screen canvases.

[xidachen, 2016-04-21 19:56:50]

Hi Ken,

I am starting to Merge the class OffscreenCanvasRenderingContext into
CanvasRenderingContext.

In terms of https://wiki.whatwg.org/wiki/OffscreenCanvas to define a
OffscreenCanvasRenderingContext2D type, I don't think that is necessary. As you
can see this line:

RenderingContext? getContext(DOMString contextId, any... arguments);

The return type is RenderingContext, and that is a union of
"CanvasRenderingContext2D", "WebGLRenderingContext", "WebGL2RenderingContext"
and "ImageBitmapRenderingContext". In my personal opinion,
OffscreenCanvasRenderingContext2D should be also belong to that union, does that
make sense?

[Ken Russell (switch to Gerrit), 2016-04-22 01:58:53]

On 2016/04/21 19:56:50, xidachen wrote:
> Hi Ken,
> 
> I am starting to Merge the class OffscreenCanvasRenderingContext into
> CanvasRenderingContext.
> 
> In terms of https://wiki.whatwg.org/wiki/OffscreenCanvas to define a
> OffscreenCanvasRenderingContext2D type, I don't think that is necessary. As
you
> can see this line:
> 
> RenderingContext? getContext(DOMString contextId, any... arguments);
> 
> The return type is RenderingContext, and that is a union of
> "CanvasRenderingContext2D", "WebGLRenderingContext", "WebGL2RenderingContext"
> and "ImageBitmapRenderingContext". In my personal opinion,
> OffscreenCanvasRenderingContext2D should be also belong to that union, does
that
> make sense?

OffscreenCanvasRenderingContext2D isn't documented in any public specification.
It needs to be in order for Chrome to legally return this type to JavaScript.
There might not be sufficient web tests for the return type of
OffscreenCanvas.getContext(), but Chrome shouldn't differ in behavior from other
browsers.

If there are problematic methods in CanvasRenderingContext2D that inform the
creation of OffscreenCanvasRenderingContext2D, then let's specify
OffscreenCanvasRenderingContext2D in
https://wiki.whatwg.org/wiki/OffscreenCanvas , and show which methods it shares
with CanvasRenderingContext2D, and whether there is an inheritance relationship
between them.

[xidachen, 2016-04-26 13:47:30]

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:517:
contextAttributes, scriptState->getExecutionContext()->url(), 0, &glInfo));
kbr@, zmo@, bajones@: when calling createOffscreenGraphicsContext3DProvider, I
pass the scriptState->getExecutionContext()->url() in here, but it seems that
the contextProvider I got is a nullptr when this is called on a worker. Do you
know why would this happen?

Also, the code works perfectly fine when it runs on main thread.

[Ken Russell (switch to Gerrit), 2016-04-27 10:30:46]

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

[Ken Russell (switch to Gerrit), 2016-04-27 10:30:47]

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:517:
contextAttributes, scriptState->getExecutionContext()->url(), 0, &glInfo));
On 2016/04/26 13:47:29, xidachen wrote:
> kbr@, zmo@, bajones@: when calling createOffscreenGraphicsContext3DProvider, I
> pass the scriptState->getExecutionContext()->url() in here, but it seems that
> the contextProvider I got is a nullptr when this is called on a worker. Do you
> know why would this happen?
> 
> Also, the code works perfectly fine when it runs on main thread.

That's probably expected at this point. Look at
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
. It calls RenderThreadImpl::current(), which will be null on worker threads.

The code needs to be generalized.

I'm not 100% sure how to make sure the GPU channel is established when not on
the main thread. Would have to study the underlying APIs. Sorry, but am at a
conference this week and won't be able to help with this until next week.
danakj@ is an expert in this code and hopefully can help you before then.

[xlai (Olivia), 2016-04-27 15:11:27]

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515:
ScriptState* scriptState = ScriptState::current(v8::Isolate::GetCurrent());
I need to take back my previous comment. Today I just realized that
ScriptState::current() should be only used in binding layer; haraken@ has
constantly removed ScriptState::current() from core/ and modules/ (e.g.,
https://codereview.chromium.org/303103002/,
https://codereview.chromium.org/295163005/), mentioning that this is a misuse.
Could you please change it back to [CallWith=ScriptState] or
[CallWith=ExecutionContext].

[haraken, 2016-04-27 15:12:51]

On 2016/04/27 15:11:27, xlai (Olivia) wrote:
>
https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/1881563003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515:
> ScriptState* scriptState = ScriptState::current(v8::Isolate::GetCurrent());
> I need to take back my previous comment. Today I just realized that
> ScriptState::current() should be only used in binding layer; haraken@ has
> constantly removed ScriptState::current() from core/ and modules/ (e.g.,
> https://codereview.chromium.org/303103002/,
> https://codereview.chromium.org/295163005/), mentioning that this is a misuse.
> Could you please change it back to [CallWith=ScriptState] or
> [CallWith=ExecutionContext].

Yes, we should get the ScriptState with [CallWith=ScriptState] and pass it down
to core/ and modules/.

[xidachen, 2016-04-27 17:25:21]

kbr@, junov@: could we land this CL as it is now? With this CL,
offscreenCanvas.getContext('webgl') works on main thread, but returns a null
object in worker. Getting this to work on a worker thread is my top priority
item but it may take a bit of time.

Note that in this CL I changed the return type of getContext to be a union type,
which I believe could be useful for some other offscreenCanvas work.

What do you think of landing the CL as it is now, and having another CL to make
this work on a worker thread.

[xidachen, 2016-04-27 17:25:53]

Description was changed from

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.

In this CL, we make WebGLRenderingContextBase inherited from 
OffscreenCanvasRenderingContext. That is the only inheritance
change to the WebGL classes.

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

to

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.


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

[Justin Novosad, 2016-04-27 17:48:30]

lgtm for moldules/offscreencanvas and core/html

[Justin Novosad, 2016-04-27 17:49:24]

https://codereview.chromium.org/1881563003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html
(right):

https://codereview.chromium.org/1881563003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html:11:
//TODO(xidachen): implement get webgl context in worker
Nit: reference a crbug here.

[xidachen, 2016-04-27 19:53:44]

Per offline discussion with piman@ and danakj@, it looks like the only way to
create the WebGraphicsContextProvider is to postTask to the main thread, and
postTask back to the worker thread.

I will try to do that in a future CL.

zmo@, bajones@: could you take a look at modules/webgl/? The changes there is
relatively small.

[bajones, 2016-04-27 20:15:13]

On 2016/04/27 19:53:44, xidachen wrote:
> Per offline discussion with piman@ and danakj@, it looks like the only way to
> create the WebGraphicsContextProvider is to postTask to the main thread, and
> postTask back to the worker thread.
> 
> I will try to do that in a future CL.
> 
> zmo@, bajones@: could you take a look at modules/webgl/? The changes there is
> relatively small.

modules/webgl/ LGTM with a couple of name change suggestions.

[bajones, 2016-04-27 20:18:48]

Oops! zmo@ pointed out I forgot to include the suggestions I mentioned. :P

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:70: static
bool shouldCreateFactory(WebGraphicsContext3DProvider* contextProvider)
This name caused me some confusion, because it doesn't indicate if a factory
should be created but whether the factory should return a context. Suggest
changing to "shouldFactoryCreateContext" or simply "shouldCreateContext"

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:506:
static PassOwnPtr<WebGraphicsContext3DProvider>
shouldCreateWebGraphicsContext3DProvider(HTMLCanvasElement* canvas, ScriptState*
scriptState, WebGLContextAttributes attributes, unsigned webGLVersion)
Similarly, this function doesn't return whether or not a context provider should
be created but actually returns one (or NULL). Suggest renaming to
"createWebGraphicsContext3DProviderInternal" or similar for clarity.

[xidachen, 2016-04-27 20:32:59]

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

[commit-bot: I haz the power, 2016-04-27 20:33:31]

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

[xidachen, 2016-04-27 20:34:04]

Thanks bajones@, doing dry run now, will commit once all bots are green.

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/1881563003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContext.cpp:70: static
bool shouldCreateFactory(WebGraphicsContext3DProvider* contextProvider)
On 2016/04/27 20:18:48, bajones wrote:
> This name caused me some confusion, because it doesn't indicate if a factory
> should be created but whether the factory should return a context. Suggest
> changing to "shouldFactoryCreateContext" or simply "shouldCreateContext"

Done.

[commit-bot: I haz the power, 2016-04-27 20:49:30]

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

[commit-bot: I haz the power, 2016-04-27 20:49:31]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[xidachen, 2016-04-28 01:15:13]

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

[commit-bot: I haz the power, 2016-04-28 01:15:31]

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

[commit-bot: I haz the power, 2016-04-28 03:37:25]

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

[commit-bot: I haz the power, 2016-04-28 03:37:27]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-04-28 10:35:11]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-04-28 10:35:12]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/1881563003/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-28 10:35:27]

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

[commit-bot: I haz the power, 2016-04-28 10:42:52]

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

[commit-bot: I haz the power, 2016-04-28 10:42:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xidachen, 2016-04-28 10:45:38]

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

[xidachen, 2016-04-28 10:45:39]

haraken@: Looks like I need a LGTM for modules/ModulesInitializer.cpp.

Could you please take a look? Thank you.

[haraken, 2016-04-28 11:14:50]

LGTM

https://codereview.chromium.org/1881563003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1881563003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp:22:
: CanvasRenderingContext(static_cast<HTMLCanvasElement*>(nullptr), canvas)

Is the static_cast needed?

[xidachen, 2016-04-28 12:39:55]

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

[commit-bot: I haz the power, 2016-04-28 12:40:11]

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

[commit-bot: I haz the power, 2016-04-28 13:55:52]

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

[commit-bot: I haz the power, 2016-04-28 13:55:53]

Dry run: This issue passed the CQ dry run.

[xidachen, 2016-04-28 13:56:19]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-04-28 13:56:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, junov@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1881563003/#ps260001
(title: "rebase and remove static_cast")

[commit-bot: I haz the power, 2016-04-28 13:56:33]

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

[commit-bot: I haz the power, 2016-04-28 14:06:09]

Description was changed from

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.


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

to

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.


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

[commit-bot: I haz the power, 2016-04-28 14:06:11]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-04-30 17:18:09]

Patchset 14 (id:??) landed as
https://crrev.com/a7270ac8f1b15170a6b28b5ebf02c65beaa6374e
Cr-Commit-Position: refs/heads/master@{#390368}

[commit-bot: I haz the power, 2016-04-30 17:55:34]

Description was changed from

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.


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

to

==========
Implement OffscreenCanvas.getContext('webgl')

At this moment, getting webgl renderingcontext for offscreenCanvas
is not yet implemented and this CL implements that.

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

Committed: https://crrev.com/a7270ac8f1b15170a6b28b5ebf02c65beaa6374e
Cr-Commit-Position: refs/heads/master@{#390368}
==========