Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1387743002:
Fixed expando-loss.html test. (Closed)
|
Created:
5 years, 2 months ago by Ken Russell (switch to Gerrit) Modified:
5 years, 2 months ago CC:
asanka, benjhayden+dwatch_chromium.org, blink-reviews, chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org, sigbjornf Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed 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
Committed: https://crrev.com/14e69f1d699166a027aae362248cdd6a5bc8215b
Cr-Commit-Position: refs/heads/master@{#353824}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Delete hidden value on object when setting it to null. #
Total comments: 12
Patch Set 3 : Addressed review feedback from haraken. #
Total comments: 7
Patch Set 4 : Cache ScriptState and use it to wrap m_defaultVertexArrayObject. #
Total comments: 20
Patch Set 5 : Resetting CL to the state of Patch Set 3. #
Total comments: 7
Patch Set 6 : Clarified a couple of points and simplified preserveObjectWrapper. #Patch Set 7 : One more clarifying comment. #
Total comments: 8
Patch Set 8 : Addressed review feedback from haraken. #Messages
Total messages: 50 (13 generated)
kbr@chromium.org changed reviewers: + bajones@chromium.org, jochen@chromium.org, zmo@chromium.org
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.
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.
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.
One question, but otherwise LGTM. https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/m... 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.
jochen@chromium.org changed reviewers: + haraken@chromium.org
approach lgtm, but adding Kentaro for double-checking
https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/1/third_party/WebKit/Source/m... 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.
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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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?
haraken: thanks for your review. Will revise. https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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).
On 2015/10/06 00:29:10, haraken wrote: > https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... > 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/ :) 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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.
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/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... 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 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/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1491: preserveObjectWrapper(scriptState, this, "renderbuffer", 0, renderBuffer); 0 => target ?
https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6735: CString name = builder.toString().utf8(); On 2015/10/06 02:41:51, Ken Russell wrote: > 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 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. Yes, a follow-up CL is fine.
https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/1387743002/#ps40001 (title: "Addressed review feedback from haraken.")
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
The CQ bit was unchecked by commit-bot@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_...)
The CQ bit was checked by kbr@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
The CQ bit was unchecked by commit-bot@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_r...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/10/07 00:38:06, commit-bot: I haz the power wrote: > 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_r...) > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Filed http://crbug.com/540492 about these failures.
The CQ bit was checked by kbr@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/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/40001/third_party/WebKit/Sour... 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_jcGS92a3a1...
The CQ bit was unchecked by haraken@chromium.org
kbr@chromium.org changed reviewers: + junov@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.
Thanks for being persistent! This is the final round of comments. https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLCanvasElement.h (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/core/html/canvas/CanvasFontCacheTest.cpp (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DAPITest.cpp (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1535: if (scriptState) { Ditto. https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6734: StringBuilder builder; Add a TODO: This logic should be moved to V8HiddenValue.
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/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/60001/third_party/WebKit/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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/Sour... 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.
kbr@chromium.org changed reviewers: - junov@chromium.org
Please re-review. Justin: your review no longer needed; Canvas 2D code is no longer touched.
LGTM https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Sour... 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/Sour... 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/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6729: { Add ASSERT(scriptState). https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6732: StringBuilder builder; Add the TODO. https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Sour... 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/Sour... 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/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1387743002/diff/80001/third_party/WebKit/Sour... 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?
LGTM https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:6765: void WebGLRenderingContextBase::maybePreserveDefaultVAOObjectWrapper(ScriptState* scriptState) Add ASSERT(scriptState)
https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1387743002/diff/120001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/Sou... 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.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, bajones@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1387743002/#ps140001 (title: "Addressed review feedback from haraken.")
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
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/14e69f1d699166a027aae362248cdd6a5bc8215b Cr-Commit-Position: refs/heads/master@{#353824}
Message was sent while issue was closed.
Probably not very important, but I think the expando-loss test is missing coverage for texture types TEXTURE_3D and TEXTURE_2D_ARRAY.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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. |
