Adding WebGL instrumentation support on the backend side.
Created attachment 145064 [details] Patch
Comment on attachment 145064 [details] Patch Attachment 145064 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12859347
Comment on attachment 145064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145064&action=review > Source/WebCore/DerivedSources.pri:779 > +InjectedWebGLScriptSource.commands = perl $$PWD/inspector/xxd.pl InjectedWebGLScriptSource_js $$PWD/inspector/InjectedWebGLScriptSource.js ${QMAKE_FILE_OUT} $$PWD/inspector/InjectedWebGLScriptSource.js -- > ${QMAKE_FILE_IN}
Comment on attachment 145064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145064&action=review > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:81 > + if (context->is3d() && InspectorInstrumentation::isWebGLInspectorEnabled(canvas->document())) { > + ScriptObject wrapped = InspectorInstrumentation::wrapWebGLRenderingContextForInstrumentation(static_cast<WebGLRenderingContext*>(context)); Can we merge isWebGLInspectorEnabled() and wrapWebGLRenderingContextForInstrumentation() into one call, e.g. wrapWebGLRenderingContextForInstrumentationIfNecessary()? > Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 > + // FIXME: Implement this! Please file a bug to have this implemented. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:62 > +template<typename T> static v8::Local<v8::Object> toV8InLocalContext(v8::Handle<v8::FunctionTemplate> functionTemplate, WrapperTypeInfo* type, T* cptr) We avoid abbreviations in WebKit, please find a more descriptive name for cptr. > Source/WebCore/inspector/InjectedScriptManager.cpp:42 > +#if ENABLE(WEBGL) > +#include "InjectedWebGLScriptSource.h" > +#endif Is this really necessary? > Source/WebCore/inspector/InjectedScriptManager.cpp:182 > + ScriptObject injectedWebGLScript = injectWebGLContextWrapperScript(injectedWebGLScriptSource(), scriptState); > + return executeInjectedWebGLContextWrapperScript(&injectedWebGLScript, scriptState, glContext); Do we ever call any of these two function independently? > Source/WebCore/inspector/InjectedWebGLScriptSource.js:2 > + * Copyright (C) 2012 Apple Inc. All rights reserved. Google copyright, please :) > Source/WebCore/inspector/InspectorWebGLAgent.cpp:60 > + m_instrumentingAgents = 0; This appears redundant. I know we occasionally do this in other places, but I don't see justification for this there either. > Source/WebCore/inspector/InspectorWebGLInstrumentation.h:47 > + if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(document)) { InstrumentingAgents * => InstrumentingAgents* > Source/WebCore/inspector/InspectorWebGLInstrumentation.h:48 > + if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent()) ditto > Source/WebCore/inspector/InspectorWebGLInstrumentation.h:59 > + if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(glContext->canvas()->document())) { ditto > Source/WebCore/inspector/InspectorWebGLInstrumentation.h:60 > + if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent()) ditto
Comment on attachment 145064 [details] Patch r- per Andrey's comments
Comment on attachment 145064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145064&action=review >> Source/WebCore/DerivedSources.pri:779 >> +InjectedWebGLScriptSource.commands = perl $$PWD/inspector/xxd.pl InjectedWebGLScriptSource_js $$PWD/inspector/InjectedWebGLScriptSource.js ${QMAKE_FILE_OUT} > > $$PWD/inspector/InjectedWebGLScriptSource.js -- > ${QMAKE_FILE_IN} Done. >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:81 >> + ScriptObject wrapped = InspectorInstrumentation::wrapWebGLRenderingContextForInstrumentation(static_cast<WebGLRenderingContext*>(context)); > > Can we merge isWebGLInspectorEnabled() and wrapWebGLRenderingContextForInstrumentation() into one call, e.g. wrapWebGLRenderingContextForInstrumentationIfNecessary()? Removed isWebGLInspectorEnabled. >> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 >> + // FIXME: Implement this! > > Please file a bug to have this implemented. Filed https://bugs.webkit.org/show_bug.cgi?id=87975 >> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:62 >> +template<typename T> static v8::Local<v8::Object> toV8InLocalContext(v8::Handle<v8::FunctionTemplate> functionTemplate, WrapperTypeInfo* type, T* cptr) > > We avoid abbreviations in WebKit, please find a more descriptive name for cptr. Done. cptr -> nativeObject >> Source/WebCore/inspector/InjectedScriptManager.cpp:42 >> +#endif > > Is this really necessary? Yes, for the InjectedWebGLScriptSource_js variable >> Source/WebCore/inspector/InjectedScriptManager.cpp:182 >> + return executeInjectedWebGLContextWrapperScript(&injectedWebGLScript, scriptState, glContext); > > Do we ever call any of these two function independently? No. Merged into one injectWebGLScript() >> Source/WebCore/inspector/InjectedWebGLScriptSource.js:2 >> + * Copyright (C) 2012 Apple Inc. All rights reserved. > > Google copyright, please :) Done. >> Source/WebCore/inspector/InspectorWebGLAgent.cpp:60 >> + m_instrumentingAgents = 0; > > This appears redundant. I know we occasionally do this in other places, but I don't see justification for this there either. What exactly is redundant? Only the last line, or both? As far as I see, it is to avoid a circular dependency, and both lines are "symmetrical" in that sense. If this is the case, I'd suggest preserving the symmetry, although it may be redundant. >> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:47 >> + if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(document)) { > > InstrumentingAgents * => InstrumentingAgents* Done. >> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:48 >> + if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent()) > > ditto Done. >> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:59 >> + if (InstrumentingAgents * instrumentingAgents = instrumentingAgentsForDocument(glContext->canvas()->document())) { > > ditto Done. >> Source/WebCore/inspector/InspectorWebGLInstrumentation.h:60 >> + if (InspectorWebGLAgent * webGLAgent = instrumentingAgents->inspectorWebGLAgent()) > > ditto Done.
Created attachment 145219 [details] Patch
Comment on attachment 145219 [details] Patch Attachment 145219 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12866431
Created attachment 145233 [details] Patch
Comment on attachment 145219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145219&action=review > Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 > + // FIXME: Implement this! Please file a bug against JSC and put its number after the fixme. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138 > + windowGlobal->SetHiddenValue(key, v); I don't think you need to store the function in a hidden property. As I understand it is called only once to wrap glContextObject and then you use the wrapper. If so the hidden property is not needed. Please explain what the expected behavior is here. > Source/WebCore/inspector/InspectorWebGLAgent.cpp:100 > + ScriptState* scriptState = mainWorldScriptState(glContext->canvas()->document()->frame()); You should pass ScriptState as an explicit parameter. It is known in the bindings, you will need to covert v8::Context into ScriptState though.
Comment on attachment 145219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145219&action=review >> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 >> + // FIXME: Implement this! > > Please file a bug against JSC and put its number after the fixme. Done. >> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138 >> + windowGlobal->SetHiddenValue(key, v); > > I don't think you need to store the function in a hidden property. As I understand it is called only once to wrap glContextObject and then you use the wrapper. If so the hidden property is not needed. Please explain what the expected behavior is here. Removed caching in the hidden property. >> Source/WebCore/inspector/InspectorWebGLAgent.cpp:100 >> + ScriptState* scriptState = mainWorldScriptState(glContext->canvas()->document()->frame()); > > You should pass ScriptState as an explicit parameter. It is known in the bindings, you will need to covert v8::Context into ScriptState though. Done.
Created attachment 145251 [details] Patch
Comment on attachment 145251 [details] Patch Attachment 145251 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12873223
Comment on attachment 145251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145251&action=review > Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:82 > + if (wrapped.jsValue().isObject()) Drop the .jsValue(). Just wrapped.isObject(). It's cleaner. > Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:46 > +#include "JSWebGLRenderingContext.h" This could be replaced with forward declaration for the time being, I guess? > Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94 > + if (!wrapped.v8Value().IsEmpty()) ditto. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:144 > + v8::Local<v8::Value> injectedScriptValue = v8::Function::Cast(*v)->Call(windowGlobal, 1, args); IIRC, this will crash if !v->IsFunction(). This might be ok given the assert above, but considering this may result from any JS error in a frequently modified injected script, we may want to fail more gracefully.
Comment on attachment 145251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145251&action=review > Source/WebCore/CMakeLists.txt:2602 > +ADD_CUSTOM_COMMAND( Should we check if WebGL is enabled here? > Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 > + // FIXME(87975): Implement this! It shouldn't be hard to implement this for JSC, we already do similar things with the inspector injected script. > Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94 > + if (!wrapped.v8Value().IsEmpty()) This code will create ScriptState instance for all contexts even if WebGL instrumentation is disabled. This should be fixed. We should probably check if WebGL inspection is enabled before getting ScriptState. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:-60 > - v8::Local<v8::Function> function = V8InjectedScriptHost::GetTemplate()->GetFunction(); These changes are not needed anymore, please revert them if so. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:129 > + v8::Local<v8::Object> glContextObject = toV8InLocalContext(V8WebGLRenderingContext::GetTemplate(), &V8WebGLRenderingContext::info, glContext); You should be able to use standard toV8 mechanism here, it will create a binding for you which you will wrap with the instrumenting code. Existing code could probably changed as well I need to check.
Comment on attachment 145251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145251&action=review >> Source/WebCore/CMakeLists.txt:2602 >> +ADD_CUSTOM_COMMAND( > > Should we check if WebGL is enabled here? Done. >> Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp:82 >> + if (wrapped.jsValue().isObject()) > > Drop the .jsValue(). Just wrapped.isObject(). It's cleaner. Done. Using the hasNoValue() method now. >> Source/WebCore/bindings/js/JSInjectedScriptManager.cpp:87 >> + // FIXME(87975): Implement this! > > It shouldn't be hard to implement this for JSC, we already do similar things with the inspector injected script. Will do it in a separate patch. Let us finish with this first. >>> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94 >>> + if (!wrapped.v8Value().IsEmpty()) >> >> ditto. > > This code will create ScriptState instance for all contexts even if WebGL instrumentation is disabled. This should be fixed. We should probably check if WebGL inspection is enabled before getting ScriptState. Done. >> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:129 >> + v8::Local<v8::Object> glContextObject = toV8InLocalContext(V8WebGLRenderingContext::GetTemplate(), &V8WebGLRenderingContext::info, glContext); > > You should be able to use standard toV8 mechanism here, it will create a binding for you which you will wrap with the instrumenting code. Existing code could probably changed as well I need to check. Done.
Created attachment 145277 [details] Patch
Comment on attachment 145277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145277&action=review > Source/WebCore/inspector/InspectorWebGLAgent.cpp:62 > + m_instrumentingAgents = 0; This one still appears redundant. Do we really need to clear this field in desturctor? Also, it belongs to the base class and is initialized by the base class, so it's unclear why it's cleared by derived class.
Comment on attachment 145277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145277&action=review >> Source/WebCore/inspector/InspectorWebGLAgent.cpp:62 >> + m_instrumentingAgents = 0; > > This one still appears redundant. Do we really need to clear this field in desturctor? Also, it belongs to the base class and is initialized by the base class, so it's unclear why it's cleared by derived class. Good point. Removed the line.
Comment on attachment 145277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145277&action=review > Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94 > + if (InspectorInstrumentation::isWebGLInspectorEnabled(imp->document())) { Alternatively you could check InspectorInstrumentation::hasFrontends() here to avoid adding additional is* method on the inspector instrumentation. > Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138 > + v8::Local<v8::Object> injectedScriptObject(v8::Object::Cast(*injectedScriptValue)); You could do v8::Handle<v8::Object>::Cast(injectedScriptValue) instead to avoid creating additional handle.
Created attachment 145282 [details] Patch
Created attachment 145287 [details] Patch
Comment on attachment 145277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145277&action=review >> Source/WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp:94 >> + if (InspectorInstrumentation::isWebGLInspectorEnabled(imp->document())) { > > Alternatively you could check InspectorInstrumentation::hasFrontends() here to avoid adding additional is* method on the inspector instrumentation. Done. >> Source/WebCore/bindings/v8/custom/V8InjectedScriptManager.cpp:138 >> + v8::Local<v8::Object> injectedScriptObject(v8::Object::Cast(*injectedScriptValue)); > > You could do v8::Handle<v8::Object>::Cast(injectedScriptValue) instead to avoid creating additional handle. Done.
Comment on attachment 145287 [details] Patch Attachment 145287 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12864557
Created attachment 145310 [details] Patch
Created attachment 145325 [details] Patch
Comment on attachment 145325 [details] Patch Attachment 145325 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12864605
Created attachment 145331 [details] Patch
Created attachment 145336 [details] Patch
Comment on attachment 145336 [details] Patch Attachment 145336 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12866608
Comment on attachment 145336 [details] Patch Attachment 145336 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12869564
Created attachment 145526 [details] Patch Checking build bots
Comment on attachment 145526 [details] Patch Attachment 145526 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12893213
Comment on attachment 145526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145526&action=review > Source/WebCore/WebCore.gypi:2824 > + 'inspector/InspectorWebGLAgent.cpp', > + 'inspector/InspectorWebGLAgent.h', > + 'inspector/InspectorWebGLInstrumentation.h', Please also add this to WebCore.vcproj.
Created attachment 145551 [details] Patch Checking build bots: Updated project.pbxproj, reverted WebCode.exp.in
Created attachment 145553 [details] Patch Checking build bots
Comment on attachment 145551 [details] Patch Attachment 145551 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12898201
Created attachment 145555 [details] Patch Checking build bots: added WebCode.exp.in back
Comment on attachment 145553 [details] Patch Attachment 145553 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12903155
Comment on attachment 145555 [details] Patch Attachment 145555 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12902169
Comment on attachment 145555 [details] Patch Attachment 145555 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12894281
Created attachment 145705 [details] Patch Checking build bots
Created attachment 145714 [details] Patch Checking build bots
Comment on attachment 145705 [details] Patch Attachment 145705 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12900423
Comment on attachment 145705 [details] Patch Attachment 145705 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12901418
Comment on attachment 145714 [details] Patch Attachment 145714 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12898475
Created attachment 145734 [details] Patch Checking build bots: added WebCore.order
(In reply to comment #47) > Created an attachment (id=145734) [details] > Patch > > Checking build bots: added WebCore.order You need to add those symbols to WebCore.exp.in. WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it
(In reply to comment #48) > (In reply to comment #47) > > Created an attachment (id=145734) [details] [details] > > Patch > > > > Checking build bots: added WebCore.order > > You need to add those symbols to WebCore.exp.in. > > WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it I did add those symbols to WebCore.exp.in - no luck so far...
(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #47) > > > Created an attachment (id=145734) [details] [details] [details] > > > Patch > > > > > > Checking build bots: added WebCore.order > > > > You need to add those symbols to WebCore.exp.in. > > > > WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it > > I did add those symbols to WebCore.exp.in - no luck so far... ah, indeed on closer inspection, your patch to project.pbxproj looks incomplete. It doesn't list the new files in the PBXBuildFile section
(In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > (In reply to comment #47) > > > > Created an attachment (id=145734) [details] [details] [details] [details] > > > > Patch > > > > > > > > Checking build bots: added WebCore.order > > > > > > You need to add those symbols to WebCore.exp.in. > > > > > > WebCore.order is only used for Safari builds or something, Apple engineers will take care of updating it > > > > I did add those symbols to WebCore.exp.in - no luck so far... > > ah, indeed > > on closer inspection, your patch to project.pbxproj looks incomplete. It doesn't list the new files in the PBXBuildFile section yeah I played with it earlier at patch https://bugs.webkit.org/attachment.cgi?id=145551&action=review (my xcode would not add PBXBuildFile section for some reason) and got the following error from the bot: xcodebuild: error: Unable to read project 'WebCore.xcodeproj'. Reason: Project /Volumes/Data/WebKit/Source/WebCore/WebCore.xcodeproj cannot be opened because because an exception occurred.: -[PBXFileReference _setBuildPhase:]: unrecognized selector sent to instance 0x4010c9140
Comment on attachment 145734 [details] Patch Attachment 145734 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12905415
Comment on attachment 145734 [details] Patch Attachment 145734 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12902459
Comment on attachment 145734 [details] Patch Attachment 145734 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12893550
Created attachment 145765 [details] Patch Checking build bots: tweaked project.pbxproj, removed WebCore.order
Created attachment 145770 [details] Patch Checking build bots: fixed project.pbxproj
Comment on attachment 145765 [details] Patch Attachment 145765 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12901478
Comment on attachment 145770 [details] Patch Attachment 145770 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12902506
Created attachment 145791 [details] Patch Checking build bots (hoping for the last time)
Comment on attachment 145770 [details] Patch Attachment 145770 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12893593
Created attachment 145802 [details] Patch Checking build bots: reverted WebCore.exp.in
Created attachment 145959 [details] =Rebased patch Rebased patch
Committed r119572: <http://trac.webkit.org/changeset/119572>