Fixed expando-loss.html test. (issue 1387743002 by kbr@chromium.org)

[k...@chromium.org]

Reviewers: bajones, Zhenyao Mo, jochen,

Message:
bajones/zmo: please review.

jochen: please advise on whether this seems like a good approach
(specifically,
dynamically building new strings and allocating V8 strings for them).
Unfortunately, many of these binding points have an arbitrary number of
indices,
so the names of those hidden values can't be preallocated.

Thanks.


Description:
Fixed expando-loss.html test.

Use V8HiddenValue to link the JavaScript wrappers of related
objects (like framebuffers/attachments, programs/shaders, and anything
bound to the context's state) so that any expando properties added to
these wrappers aren't lost.

Do the same for fetched WebGL extensions.
github.com/KhronosGroup/WebGL/1254 makes these tests more rigorous.

Remove the suppression for the expando-loss.html test.

BUG=485634

Please review this at https://codereview.chromium.org/1387743002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+155, -67 lines):
M content/test/gpu/gpu_tests/webgl_conformance_expectations.py
M third_party/WebKit/Source/modules/webgl/OESVertexArrayObject.h
M third_party/WebKit/Source/modules/webgl/OESVertexArrayObject.cpp
M third_party/WebKit/Source/modules/webgl/OESVertexArrayObject.idl
M third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.h
M third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
M third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.idl
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.idl

[k...@chromium.org]

Note: while this fixes expando-loss.html, more work is needed to handle the
new
binding points in WebGL 2.0 -- and to test them. I'll do that in a
follow-on CL
and bug.


https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Finally: I'm slightly concerned about the amount of work that's being added
to
methods like vertexAttribPointer, but it's basically inevitable -- it must
be
done to be spec compliant. Still, any other ideas for speeding this up
appreciated.


https://codereview.chromium.org/1387743002/

[baj...@chromium.org]

One question, but otherwise LGTM.


https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5031
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5031:
preserveObjectWrapper(scriptState, m_boundVertexArrayObject,
"arraybuffer", index, m_boundArrayBuffer);
I don't think so, but just so we're clear: This won't prevent developers
for attaching an "arraybuffer" property (or other names used throughout
this patch) to the context in their scripts? If so I have some (very
very tiny) concerns about behavior regression and think we should use
more obfuscated named.

https://codereview.chromium.org/1387743002/

[joc...@chromium.org]

approach lgtm, but adding Kentaro for double-checking

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5031
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5031:
preserveObjectWrapper(scriptState, m_boundVertexArrayObject,
"arraybuffer", index, m_boundArrayBuffer);

On 2015/10/03 14:39:23, bajones wrote:
> I don't think so, but just so we're clear: This won't prevent
developers for
> attaching an "arraybuffer" property (or other names used throughout
this patch)
> to the context in their scripts? If so I have some (very very tiny)
concerns
> about behavior regression and think we should use more obfuscated
named.

No. These properties don't show up on the object. I triple-checked this
by adding printf logging and using every JavaScript API I could think of
to fetch these hidden properties, and they aren't visible at the user
level.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Thanks for your reviews. I'm confident in the correctness of this change so
am
CQ'ing it now. If issues are discovered we can revise or revert it.


https://codereview.chromium.org/1387743002/

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode2620
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:2620:
v8::Local<v8::Value> wrappedExtension = toV8(extension,
scriptState->context()->Global(), scriptState->isolate());

It is sad we have to write v8:: outside bindings/, but this is not a
regression in this CL. OK :)

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6735
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735:
CString name = builder.toString().utf8();

It is inefficient to build up a hidden name string every time. Can we
define the hidden name in V8HiddenValue.h and use V8HiddenValue::xxx()
to get the name string?

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6760
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6760:
m_defaultVertexArrayObject->wrap(scriptState->isolate(),
scriptState->context()->Global());

Why do you use wrap instead of toV8?

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6762
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6762:
m_preservedDefaultVAOObjectWrapper = true;

How is it guaranteed that m_preservedDefaultVAOObjectWrapper becomes
false when the wrapper gets collected by a V8 GC?

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

haraken: thanks for your review. Will revise.

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6735
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735:
CString name = builder.toString().utf8();

On 2015/10/05 23:40:07, haraken wrote:

> It is inefficient to build up a hidden name string every time. Can we
define the
> hidden name in V8HiddenValue.h and use V8HiddenValue::xxx() to get the
name
> string?

Not with the current code structure in V8HiddenValue.h, since the number
of names we may have to generate isn't known until runtime (it depends
on things like the number of available texture units on the system). If
you can suggest how to improve V8HiddenValue to support the kind of
"indexed" hidden value names this CL needs -- maybe by populating them
lazily -- I'll be happy to implement it.

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6760
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6760:
m_defaultVertexArrayObject->wrap(scriptState->isolate(),
scriptState->context()->Global());

On 2015/10/05 23:40:07, haraken wrote:

> Why do you use wrap instead of toV8?

Thanks, good point. I didn't know that was preferred. Will fix.

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6762
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6762:
m_preservedDefaultVAOObjectWrapper = true;

On 2015/10/05 23:40:07, haraken wrote:

> How is it guaranteed that m_preservedDefaultVAOObjectWrapper becomes
false when
> the wrapper gets collected by a V8 GC?

That's not how it's handled -- the default VAO's object wrapper is
supposed to be preserved for the lifetime of the
WebGLRenderingContextBase, or until the underlying OpenGL context is
deleted. See that m_preservedDefaultVAOObjectWrapper is set to false in
WebGLRenderingContextBase::initializeNewContext.

https://codereview.chromium.org/1387743002/

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6735
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735:
CString name = builder.toString().utf8();

On 2015/10/06 00:14:19, Ken Russell wrote:
> On 2015/10/05 23:40:07, haraken wrote:
> >
> > It is inefficient to build up a hidden name string every time. Can
we define
> the
> > hidden name in V8HiddenValue.h and use V8HiddenValue::xxx() to get
the name
> > string?

> Not with the current code structure in V8HiddenValue.h, since the
number of
> names we may have to generate isn't known until runtime (it depends on
things
> like the number of available texture units on the system). If you can
suggest
> how to improve V8HiddenValue to support the kind of "indexed" hidden
value names
> this CL needs -- maybe by populating them lazily -- I'll be happy to
implement
> it.

Yes, it will be great if you can encapsulate the logic in V8HiddenValue.
The point is that we don't want to spread out V8 APIs outside bindings/
:)

https://codereview.chromium.org/1387743002/

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6755
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6755:
void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

Just to confirm: maybePreserveDefaultVAOObjectWrapper is called only
from JavaScript, right?

If it is called from JavaScript, it's guaranteed that we're in a correct
ScriptState when we reach here. Otherwise, we can be in a wrong
ScriptState and end up with creating the wrapper in a wrong ScriptState
(which causes a wrapper leak between worlds).

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

On 2015/10/06 00:29:10, haraken wrote:

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):

Do you have a suggestion for the form of the API in V8HiddenValue? The name
needs to be built up somehow. I can implement a specialized hash table that
takes a fixed string and an index, and lazily instantiates v8AtomicStrings.
I
would strongly prefer to do that as a follow-on optimization though.


https://codereview.chromium.org/1387743002/

[k...@chromium.org]

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6755
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6755:
void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

On 2015/10/06 00:31:29, haraken wrote:

> Just to confirm: maybePreserveDefaultVAOObjectWrapper is called only
from
> JavaScript, right?

> If it is called from JavaScript, it's guaranteed that we're in a
correct
> ScriptState when we reach here. Otherwise, we can be in a wrong
ScriptState and
> end up with creating the wrapper in a wrong ScriptState (which causes
a wrapper
> leak between worlds).

Yes, it's only called from other methods in this class that are called
from JavaScript -- bindBuffer and vertexAttribPointer.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Please re-review. Using toV8 now, rather than wrap() directly.

haraken: if this looks good to you please CQ it. I'd like to design
whatever new
V8HiddenValue API is needed as follow-on work. Thanks.

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6735
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735:
CString name = builder.toString().utf8();

On 2015/10/06 00:29:10, haraken wrote:
> On 2015/10/06 00:14:19, Ken Russell wrote:
> > On 2015/10/05 23:40:07, haraken wrote:
> > >
> > > It is inefficient to build up a hidden name string every time. Can
we define
> > the
> > > hidden name in V8HiddenValue.h and use V8HiddenValue::xxx() to get
the name
> > > string?
> >
> > Not with the current code structure in V8HiddenValue.h, since the
number of
> > names we may have to generate isn't known until runtime (it depends
on things
> > like the number of available texture units on the system). If you
can suggest
> > how to improve V8HiddenValue to support the kind of "indexed" hidden
value
> names
> > this CL needs -- maybe by populating them lazily -- I'll be happy to
implement
> > it.

> Yes, it will be great if you can encapsulate the logic in
V8HiddenValue. The
> point is that we don't want to spread out V8 APIs outside bindings/ :)

Sorry, I should have replied here rather than globally on the code
review.

Do you have a suggestion for the form of the API in V8HiddenValue? The

name still needs to be built up somehow. I can implement a specialized

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode998
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:998:
m_preservedDefaultVAOObjectWrapper = false;

The WebGLRenderingContext is always created by JavaScript, right? If it
is the case, you can just record the ScriptState as:

class WebGLRenderingContext {
RefPtr<ScriptState> m_scriptState;
};

and use the ScriptState to create a wrapper here. (This is a common
programming pattern of ScriptStates.)

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1475
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1475:
preserveObjectWrapper(scriptState, this, "framebuffer", 0, buffer);

0 => target ?

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1491
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1491:
preserveObjectWrapper(scriptState, this, "renderbuffer", 0,
renderBuffer);

0 => target ?

https://codereview.chromium.org/1387743002/

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6735
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735:
CString name = builder.toString().utf8();

Yes, a follow-up CL is fine.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode998
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:998:
m_preservedDefaultVAOObjectWrapper = false;

On 2015/10/06 03:46:06, haraken wrote:

> The WebGLRenderingContext is always created by JavaScript, right? If
it is the
> case, you can just record the ScriptState as:

> class WebGLRenderingContext {
> RefPtr<ScriptState> m_scriptState;
> };

> and use the ScriptState to create a wrapper here. (This is a common
programming
> pattern of ScriptStates.)

Thanks for this suggestion, but I think I'll leave this as is. I'd like
to avoid adding more RefPtrs in this code and it's not that urgent to
immediately create the wrapper for the default VAO.

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1475
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1475:
preserveObjectWrapper(scriptState, this, "framebuffer", 0, buffer);

On 2015/10/06 03:46:06, haraken wrote:

> 0 => target ?

It's not strictly necessary since there's only one target. I was
thinking of having another code path in preserveObjectWrapper which at
least skipped the call to StringBuilder::appendNumber.

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1491
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1491:
preserveObjectWrapper(scriptState, this, "renderbuffer", 0,
renderBuffer);

On 2015/10/06 03:46:06, haraken wrote:

> 0 => target ?

Same.

https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_rel_ng/builds/78678)
linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/123138)
mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/122928)

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Filed http://crbug.com/540492 about these failures.


https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

[har...@chromium.org]

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode998
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:998:
m_preservedDefaultVAOObjectWrapper = false;

On 2015/10/06 21:28:42, Ken Russell wrote:
> On 2015/10/06 03:46:06, haraken wrote:
> >
> > The WebGLRenderingContext is always created by JavaScript, right? If
it is the
> > case, you can just record the ScriptState as:
> >
> > class WebGLRenderingContext {
> > RefPtr<ScriptState> m_scriptState;
> > };
> >
> > and use the ScriptState to create a wrapper here. (This is a common
> programming
> > pattern of ScriptStates.)

> Thanks for this suggestion, but I think I'll leave this as is. I'd
like to avoid
> adding more RefPtrs in this code and it's not that urgent to
immediately create
> the wrapper for the default VAO.

I begin to think that this CL may be using a wrong ScriptState. I think
a right implementation would be:

- Record a ScriptState to WebGLRenderingContextBase::m_scriptState when
the WebGLRenderingContextBase is constructed (from JavaScript).

- Use the recorded ScriptState when the attachShader, bindFramebuffer
etc are called.

It is not guaranteed that the current ScriptState when the attachShader
etc are called is equal to the ScriptState when
WebGLRenderingContextBase was constructed. I haven't considered how the
difference results in a security risk and/or wrong behavior in this
particular case, but a safer and more correct implementation is to use
the recorded ScriptState.

Other binding classes follow the pattern:

$ grep -r 'RefPtr<ScriptState>' Source/bindings/

More background theory is written in the section 5 of this document:
https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1UU/edit

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

haraken: addressed your review feedback, caching the ScriptState and using
it.

junov: I had to modify the CanvasRenderingContext2D constructors too. Please
review.

Thanks. Re-tested locally. I'd really like to land this tomorrow to make
more
progress on these tests so appreciate your prompt re-reviews.


https://codereview.chromium.org/1387743002/

[har...@chromium.org]

Thanks for being persistent! This is the final round of comments.



https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right):

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/core/html/HTMLCanvasElement.h#newcode113
third_party/WebKit/Source/core/html/HTMLCanvasElement.h:113: // The
WebGLRenderingContext requires a non-null ScriptState, but the
Canvas2DRenderingContext does not.

In general, it is not nice to pass a nullptr to ScriptState. If this is
a testing-only issue, you can use V8TestingScope which gives you a
ScriptState to use.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/core/html/canvas/CanvasFontCacheTest.cpp
File third_party/WebKit/Source/core/html/canvas/CanvasFontCacheTest.cpp
(right):

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/core/html/canvas/CanvasFontCacheTest.cpp#newcode64
third_party/WebKit/Source/core/html/canvas/CanvasFontCacheTest.cpp:64:
m_canvasElement->getCanvasRenderingContext(nullptr, canvasType,
attributes);

Consider using V8TestingScope.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DAPITest.cpp
File
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DAPITest.cpp
(right):

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DAPITest.cpp#newcode65
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DAPITest.cpp:65:
m_canvasElement->getCanvasRenderingContext(nullptr, canvasType,
attributes);

Ditto.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp#newcode2953
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2953:
bindFramebuffer(nullptr, GL_READ_FRAMEBUFFER,
m_readFramebufferBinding.get());

I'm wondering why we can't use m_scriptState.

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

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1001
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1001:
toV8(m_defaultVertexArrayObject, m_scriptState->context()->Global(),
m_scriptState->isolate());

Just for the record: We don't need to enter the ScriptState before
calling these toV8()s because we're already in a correct ScriptState
when the WebGLRenderingContextBase's constructor gets called (from
JavaScript).

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1477
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1477:
if (scriptState)

It would be great if we could remove the null check. By replacing the
'nullptr' with 'm_scriptState', I guess we can guarantee that
scriptState is never null here.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1535
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1535:
if (scriptState) {

Ditto.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode2623
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:2623:

v8::Local<v8::Value> wrappedExtension = toV8(extension,
scriptState->context()->Global(), scriptState->isolate());

toV8 does ScriptState-dependent operations, so we must enter a correct
ScriptState before calling toV8. I guess we need the following code:

if (!scriptState->contextIsValid())
return ScriptValue();

ScriptState::Scope scope(scriptState);
...; // You can call toV8 here.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6700
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6700:
bindFramebuffer(nullptr, GL_FRAMEBUFFER, m_framebufferBinding.get());

I'm wondering why we can't use m_scriptState.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6705
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6705:
bindTexture(nullptr, GL_TEXTURE_2D,
m_textureUnits[m_activeTextureUnit].m_texture2DBinding.get());

Ditto.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6730
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6730:
void WebGLRenderingContextBase::preserveObjectWrapper(ScriptState*
scriptState, ScriptWrappable* sourceObject, const char* baseName,
unsigned long index, ScriptWrappable* targetObject)

Actually preserveObjectWrapper doesn't need to take ScriptState (it is
doing nothing ScriptState-dependent). Passing an Isolate* would be
enough.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6734
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6734:
StringBuilder builder;

Add a TODO: This logic should be moved to V8HiddenValue.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Per offline conversation between haraken and me, the storing of
m_scriptState is
not necessary. Since bindBuffer and vertexAttribPointer -- the two methods
which
would need to lazily instantiate the JavaScript wrapper for
m_defaultVertexArrayObject -- are both always called from JavaScript, the
ScriptState passed to them is always legal to use to create this JS wrapper.

I'm reverting the state of this CL to Patch Set 3, and making a couple of
modifications per the code review below.

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

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1001
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1001:
toV8(m_defaultVertexArrayObject, m_scriptState->context()->Global(),
m_scriptState->isolate());

On 2015/10/13 02:02:04, haraken wrote:

> Just for the record: We don't need to enter the ScriptState before
calling these
> toV8()s because we're already in a correct ScriptState when the
> WebGLRenderingContextBase's constructor gets called (from JavaScript).

Actually, this code is called from a timer after the context is lost, so
this code as written is incorrect. It would have to enter m_scriptState.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1477
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1477:
if (scriptState)

On 2015/10/13 02:02:04, haraken wrote:

> It would be great if we could remove the null check. By replacing the
'nullptr'
> with 'm_scriptState', I guess we can guarantee that scriptState is
never null
> here.

There's no need to do this work if we're calling this function
internally. The only time it's necessary to update the references
between these wrappers is if the user has called bindFramebuffer from
JavaScript.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1535
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1535:
if (scriptState) {

On 2015/10/13 02:02:04, haraken wrote:

> Ditto.

Same answer.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode2623
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:2623:
v8::Local<v8::Value> wrappedExtension = toV8(extension,
scriptState->context()->Global(), scriptState->isolate());

On 2015/10/13 02:02:04, haraken wrote:

> toV8 does ScriptState-dependent operations, so we must enter a correct
> ScriptState before calling toV8. I guess we need the following code:

> if (!scriptState->contextIsValid())
> return ScriptValue();

> ScriptState::Scope scope(scriptState);
> ...; // You can call toV8 here.

This function is always called from JavaScript, so the incoming
ScriptState is always valid.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6700
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6700:
bindFramebuffer(nullptr, GL_FRAMEBUFFER, m_framebufferBinding.get());

On 2015/10/13 02:02:04, haraken wrote:

> I'm wondering why we can't use m_scriptState.

There's no need to update the references between the JavaScript wrappers
when this is called internally; we're temporarily modifying the bound
framebuffer and restoring its state before returning to JavaScript.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6705
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6705:
bindTexture(nullptr, GL_TEXTURE_2D,
m_textureUnits[m_activeTextureUnit].m_texture2DBinding.get());

On 2015/10/13 02:02:04, haraken wrote:

> Ditto.

Same thing for the TEXTURE_2D binding on the active texture unit. We
don't want to modify the references between the context's objects when
doing some temporary internal modifications.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6730
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6730:
void WebGLRenderingContextBase::preserveObjectWrapper(ScriptState*
scriptState, ScriptWrappable* sourceObject, const char* baseName,
unsigned long index, ScriptWrappable* targetObject)

On 2015/10/13 02:02:04, haraken wrote:

> Actually preserveObjectWrapper doesn't need to take ScriptState (it is
doing
> nothing ScriptState-dependent). Passing an Isolate* would be enough.

Thanks for that hint. I'll change its signature.

https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6734
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6734:
StringBuilder builder;

On 2015/10/13 02:02:04, haraken wrote:

> Add a TODO: This logic should be moved to V8HiddenValue.

OK.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

Please re-review.

Justin: your review no longer needed; Canvas 2D code is no longer touched.


https://codereview.chromium.org/1387743002/

[har...@chromium.org]

LGTM



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

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode998

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:998:
m_preservedDefaultVAOObjectWrapper = false;

m_preservedDefaultVAOObjectWrapper carries two meanings:

(a) whether the wrapper is already preserved or not
(b) whether the ScriptState is available or not

It would be better to limit the responsibility of
m_preservedDefaultVAOObjectWrapper only to (a). (b) should be checked by
if(!scriptState).

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1975
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1975:
preserveObjectWrapper(scriptState, this, "boundvao", 0, arrayObject);

Can we move this into the if(arrayObject) block?

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6729
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6729:
{

Add ASSERT(scriptState).

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6732
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6732:
StringBuilder builder;

Add the TODO.

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6755

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6755:
void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

preserveDefaultVAOObjectWrapperIfNeeded ?

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6755

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6755:
void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

Add ASSERT(scriptState).

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

https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h#newcode1107
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1107:
void maybePreserveDefaultVAOObjectWrapper(ScriptState*);

It's still not clear to me when maybePreserveDefaultVAOObjectWrapper
should be called. Shall we add a comment?

https://codereview.chromium.org/1387743002/

[har...@chromium.org]

LGTM



https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1002
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1002:
m_preservedDefaultVAOObjectWrapper = false;

Wouldn't it be better to call this when the context is lost (instead of
when the context is re-initialized)?

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1981
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1981:

preserveObjectWrapper(scriptState, this, "boundvao", 0, arrayObject);

Can we move this into the if(arrayObject) block?

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6734
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6734:

void WebGLRenderingContextBase::preserveObjectWrapper(ScriptState*
scriptState, ScriptWrappable* sourceObject, const char* baseName,
unsigned long index, ScriptWrappable* targetObject)

Add ASSERT(scriptState)

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6765
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6765:

void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

Add ASSERT(scriptState)

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1002
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1002:
m_preservedDefaultVAOObjectWrapper = false;

On 2015/10/13 06:40:58, haraken wrote:

> Wouldn't it be better to call this when the context is lost (instead
of when the
> context is re-initialized)?

It's best to keep it close to the logic which actually instantiates the
default VAO rather than putting just this code in loseContextImpl.

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode1981
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1981:
preserveObjectWrapper(scriptState, this, "boundvao", 0, arrayObject);

On 2015/10/13 06:40:58, haraken wrote:

> Can we move this into the if(arrayObject) block?

No, we want to delete the hidden value for "boundvao" if arrayObject is
null.

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6734
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6734:
void WebGLRenderingContextBase::preserveObjectWrapper(ScriptState*
scriptState, ScriptWrappable* sourceObject, const char* baseName,
unsigned long index, ScriptWrappable* targetObject)

On 2015/10/13 06:40:58, haraken wrote:

> Add ASSERT(scriptState)

Done.

https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode6765
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6765:
void
WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState*
scriptState)

On 2015/10/13 06:40:58, haraken wrote:

> Add ASSERT(scriptState)

Done.

https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/1387743002/140001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1387743002/140001


https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #8 (id:140001)

https://codereview.chromium.org/1387743002/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/1387743002/

[ju...@chromium.org]

Probably not very important, but I think the expando-loss test is missing
coverage for texture types TEXTURE_3D and TEXTURE_2D_ARRAY.

https://codereview.chromium.org/1387743002/

[baj...@chromium.org]

On 2015/10/13 19:49:43, Justin Novosad wrote:
> Probably not very important, but I think the expando-loss test is missing
> coverage for texture types TEXTURE_3D and TEXTURE_2D_ARRAY.

Those should be covered in the WebGL 2 variant of the test, which I added
yesterday. Ken has already asked me to add those types.

https://codereview.chromium.org/1387743002/

[k...@chromium.org]

On 2015/10/13 19:51:11, bajones wrote:
> On 2015/10/13 19:49:43, Justin Novosad wrote:
> > Probably not very important, but I think the expando-loss test is
> missing
> > coverage for texture types TEXTURE_3D and TEXTURE_2D_ARRAY.

> Those should be covered in the WebGL 2 variant of the test, which I added
> yesterday. Ken has already asked me to add those types.

Note also that follow-on Issue 538938 covers doing this for new binding
points,
etc. in WebGL 2.0.


https://codereview.chromium.org/1387743002/