Implement canvas tooling in Inspector frontend and backend. Initial patches should allow inspection of WebGL canvas resources (shaders and textures).
<rdar://problem/18509072>
Created attachment 239038 [details] Patch
Comment on attachment 239038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239038&action=review Overall this is a great first patch! Very clean. Nit: Needs ChangeLogs > Source/WebCore/DerivedSources.make:1143 > + $(WebCore)/inspector/protocol/Canvas.json \ Same will have to be done to the CMakeLists.txt, adding the JSON and new file to compile. Adding the new files to Windows vsproj as well. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:15 > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of Apple Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. This is the wrong license. We have a 2 clause LICENSE now, not a 3 clause. You can use the one from Source/WebCore/inspector/CommandLineAPIModule.h as an example (updating the year to 2014). Ditto for other files. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:42 > +#include "InspectorWebFrontendDispatchers.h" Nit: Unnecessary. This is already in the .h. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:46 > +#include <wtf/Vector.h> Is Vector used below? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:103 > +void InspectorCanvasAgent::canvasDestroyed(HTMLCanvasElement& canvas) Does this get called when navigating? To be sure we remove canvases from the previous page? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:130 > + HashMap<const HTMLCanvasElement*, int>::const_iterator it = m_canvasToId.find(canvas); Style: You could make this "auto it", as the type is not really helpful here. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:150 > + RefPtr<Inspector::Protocol::Canvas::Canvas> result = Inspector::Protocol::Canvas::Canvas::create() > + .setCanvasId(idForCanvas(canvas)) > + .setFrameId(m_pageAgent->frameId(frame)) > + .setIs2d(renderingContext->is2d()) > + .setIs3d(renderingContext->is3d()); > + > + return result.release(); Style: You can one line this instead of assigning to a temporary RefPtr which may increment the retain count unnecessarily. return I:P:C:C::create() .setFoo().release(); > Source/WebCore/inspector/InspectorCanvasAgent.h:51 > +class InspectorCanvasAgent Nit: "final" > Source/WebCore/inspector/InspectorCanvasAgent.h:58 > + explicit InspectorCanvasAgent(InstrumentingAgents*, InspectorPageAgent*); Nit: explicit is not needed with a constructor with more than a single argument. It is only useful on constructors with 1 argument, to prevent implicit cast conversions. > Source/WebCore/inspector/protocol/Canvas.json:57 > + { "name": "canvasId", "$ref": "DOM.NodeId", "description": "Id of the canvas that was removed." } This should be a CanvasId, not a DOM.NodeId. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:33 > + this._canvasIdMap = {}; This can and probably should be a Map. this._canvasIdMap = new Map; You would have to change the getters/setters/removal/iteration below. But at least then the key is kept an integer instead of strings. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:99 > + this._canvasIdMap = {}; Just put this in reset() and call reset()? > Source/WebInspectorUI/UserInterface/Models/Canvas.js:33 > + this._is2d = false; > + this._is3d = false; These should be set to the incoming parameters. this._is2d = is2d || false; this._is3d = is3d || false; > Source/WebInspectorUI/UserInterface/Models/Canvas.js:51 > + > + constructor: WebInspector.Canvas, You can put the proto here and remove it from the bottom of the file: __proto__: WebInspector.Object.prototype, See Probe.js as an example of the new, preferred, style. Ditto for the other files. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:82 > + get displayName() > + { > + // Assign a unique number to the script object so it will stay the same. > + if (!this._uniqueDisplayNameNumber) > + this._uniqueDisplayNameNumber = this.constructor._nextUniqueDisplayNameNumber++; > + > + return WebInspector.UIString("Canvas %d").format(this._uniqueDisplayNameNumber); > + }, It may also be nice to have a better name for a canvas if it is unique. E.g. "Canvas Element #foo" for <canvas id="foo">. Or "CSS Canvas 'foo'" for -webkit-canvas(foo). > Source/WebInspectorUI/UserInterface/Protocol/DebuggerObserver.js:39 > + WebInspector.canvasManager.reset(); I don't think this is necessary. The MainFrame navigated event should be fine. > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:44 > + WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasAdded, this._canvasWasAdded, this); > + WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasRemoved, this._canvasWasRemoved, this); This is dangerous, it may be leaking the FrameTreeElement forever because it is adding itself as a strong listener of a singleton. At some point this will have to be removed as a listener. This will require some thought. In fact, the WebInspector.notifications code below might also cause a leak for all FrameTreeElements that are MainFrames...
Created attachment 239073 [details] Patch
Comment on attachment 239073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239073&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:84 > +void InspectorCanvasAgent::enable(ErrorString*) ErrorString& now after Brian's change. Will need to rebase. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:77 > + // Assign a unique number to the script object so it will stay the same. Canvas not script.
Comment on attachment 239073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239073&action=review Looking good, some more substantial comments now. Like Tim said, you'll need to rebase and tweak to get the bots building with some of the recent changes. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:70 > + if (!canvas) > + return; This is covered by idForCanvas in the next statement. So you can remove it here. Also, we always expect a canvas, so maybe we should convert to HTMLCanvasElement references, or ASSERT(canvas). It would be nice to move to references. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:115 > + HashMap<int, const HTMLCanvasElement*>::const_iterator it = m_idToCanvas.find(id); Nit: Could be auto here as well. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:135 > +PassRefPtr<Inspector::Protocol::Canvas::Canvas> InspectorCanvasAgent::buildObjectForCanvas(const HTMLCanvasElement* canvas) > +{ > + if (!canvas) > + return nullptr; Another place where switching to references we can avoid an unnecessary null check. > Source/WebCore/inspector/InspectorInstrumentation.h:277 > + static void didCreateRenderingContextForCanvas(Document*, HTMLCanvasElement*); If we move to references, this would be the starting point. HTMLCanvasElement& instead of HTMLCanvasElement*. > Source/WebCore/inspector/protocol/Canvas.json:17 > + "id": "ShaderType", > + "type": "string", > + "enum": ["FragmentShader", "VertexShader"], > + "description": "Shader type. WebGL 1.0 supports VERTEX_SHADER and FRAGMENT_SHADER types." Is the word Shader in the enum redundant? Could it just be "Fragment" and "Vertex"? > Source/WebInspectorUI/UserInterface/Models/Canvas.js:30 > +WebInspector.Canvas = function(id, parentFrame, is2d, is3d) > +{ > + WebInspector.Object.call(this); > + > + this._id = id || null; Nit: We should assert we have an id and a parentFrame, those we always expect. console.assert(id); console.assert(parentFrame); > Source/WebInspectorUI/UserInterface/Models/Shader.js:30 > + this._id = id || null; Same thing here, the "id || null" seems sketchy because they are different types. A null id would likely cause problems. > Source/WebInspectorUI/UserInterface/Models/Shader.js:49 > + return this._documentNodeIdentifier + ":" + this._name; This is wrong. Should just be: return this._id; > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:111 > + WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasAdded, this._canvasWasAdded, this); > + WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasRemoved, this._canvasWasRemoved, this); Given that when we attach we start listening for NEW canvases. Then at some point we need to get the list of CURRENT canvases. One place you could do that is in FrameTreeElement.prototype.onpopulate. It look up the current list of canvases for this frame. I'd expect something like: var canvases = WebInspector.canvasManager.canvases; for (var canvas of canvases) { if (canvas.parentFrame === this) this._addTreeElementForRepresentedObject(canvas); } Perhaps it will always be the case that this is empty. But maybe not (I think reshuffling of Resources elements into folders might do this). Test case would be: 1. A page with a <canvas> 2. Load 1 resource a second until resources get sorted into folders => does the Canvas still show up or not? > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:129 > + if (this._frame.isMainFrame()) { > + WebInspector.notifications.removeEventListener(WebInspector.Notification.PageArchiveStarted, this._pageArchiveStarted, this); > + WebInspector.notifications.removeEventListener(WebInspector.Notification.PageArchiveEnded, this._pageArchiveEnded, this); > + } Nit: Should also reset state. Either here or in onattach. this._downloadingPage = false; > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:301 > + _canvasWasAdded: function(event) > + { > + this._addRepresentedObjectToNewChildQueue(event.data.canvas); > + }, > + > + _canvasWasRemoved: function(event) > + { > + this._removeChildForRepresentedObject(event.data.canvas); > + }, These need to check that the canvas is part of this frame. For example: var canvas = event.data.canvas; if (canvas.parentFrame === this) this._addRepresentedObjectToNewChildQueue(canvas); A test case would be a page with a <canvas> in an <iframe>. Seems right now the canvas would show up in both the iframe and main frame.
Created attachment 239374 [details] Patch
Created attachment 239375 [details] Patch
Comment on attachment 239375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239375&action=review Looking real good. Some more tweaks and it will be ready! > Source/WebCore/html/HTMLCanvasElement.h:36 > +#include <wtf/HashSet.h> Not needed now? > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:49 > + canvasesForFrame: function(frameId, callback) Spell out frameIdentifier. This can use promises. The callback system is on its way out. It should clean up this function a lot! See: http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#UnderstandingandUsingPromises Some examples are DebuggerManager.pause and ReplayManager.getSession. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43 > +WebInspector.CanvasTreeElement.prototype = { > + constructor: WebInspector.CanvasTreeElement > +}; > + > +WebInspector.CanvasTreeElement.prototype.__proto__ = WebInspector.GeneralTreeElement.prototype; __proto__ should move into the block next to constructor property. > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:208 > + for (var i = 0; i < canvases.length; ++i) > + this._addTreeElementForRepresentedObject(canvases[i]); Should do: for (var canvas of canvases) this._addTreeElementForRepresentedObject(canvas); > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:564 > + if (representedObject instanceof WebInspector.ShaderProgram) { > + if (!this._shadersFolderTreeElement) > + this._shadersFolderTreeElement = createFolderTreeElement.call(this, "shaders", WebInspector.UIString("Shaders")); > + return this._shadersFolderTreeElement; > + } Sounds like this will change, since Shaders will go under the owning Canvas. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:41 > +WebInspector.ShaderProgramTreeElement.prototype = { > + constructor: WebInspector.ShaderProgramTreeElement > +}; > + > +WebInspector.ShaderProgramTreeElement.prototype.__proto__ = WebInspector.GeneralTreeElement.prototype; Ditto re: __proto__.
Comment on attachment 239375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239375&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:62 > +void InspectorCanvasAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason) Seems this should call disable. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:72 > + int id = idForCanvas(canvas); > + if (id) > + return; Should this ASSERT(!id)? didCreate shouldn't happen for an existing canvas, right?
Comment on attachment 239375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239375&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:62 >> +void InspectorCanvasAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason) > > Seems this should call disable. We can probably just remove enable and disable in the agent! The places that use m_enabled already also null check m_frontendDispatcher. Down with enable!
Created attachment 241326 [details] Patch
Comment on attachment 241326 [details] Patch This patch doesn't seem to build. I'll have more comments after lunch.
Created attachment 241372 [details] Patch
Comment on attachment 241372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241372&action=review Overall it's much better than last time I looked at this patch. There are a few style nits in the canvas agent. I think the backend is almost ready to land. I haven't looked at the frontend part of this patch yet. My main concerns (r- for missing tests): * you should write some basic tests for the canvas protocol domain. It should hit all major code paths: add/remove canvas, shaders, programs; ensure proper reset across main frame navigations; ensure that graphics context<-->inspector agent association works correctly (by using nested iframes). This is really only like inspector-protocol 5 tests at most. You can pattern the test structure off the ones in LayoutTests/inspector-protocol/debugger/. You may have to reload the test page or dynamically append the canvas in order to reliably receive context creation events, though. * i would prefer that the frontend side of this patch was split off from backend things, so that it requires less domain knowledge for each half to get reviewed. And, less thrashing on either side if you have to roll out a commit. You can use a local git branch to continue making progress on dependent patches. > Source/WebCore/ChangeLog:10 > + I would say a little something about what events are now instrumented, and what new data sources are available to the frontend (just shader sources, I think?) Also, the instrumentation strategy for canvas DOM elements is a little unusual compared to other agents so I would call that out here. > Source/WebInspectorUI/ChangeLog:10 > + view (this may change in the future), however shader programs (and eventually textures) belongiong to the typo: belonging > Source/JavaScriptCore/inspector/protocol/Canvas.json:3 > + "description": "Canvas and WebGL stuff.", "Supports inspection of DOM elements rendered using 2D (canvas) and 3D (WebGL) graphics contexts." > Source/JavaScriptCore/inspector/protocol/Canvas.json:14 > + { "name": "objectId", "type": "integer", "description": "The program child object id within the canvas." } Not sure what 'program child object id' is. > Source/JavaScriptCore/inspector/protocol/Canvas.json:30 > + { "name": "is2d", "type": "boolean", "description": "True if this canvas is backed by a 2D rendering context." }, These are mutually exclusive, right? Then we should make it an enum. > Source/JavaScriptCore/inspector/protocol/Canvas.json:32 > + { "name": "programs", "type": "array", "items": { "$ref": "Program" }, "description": "Array of shader program objects." } This includes all programs, or just linked programs? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:79 > + for (auto it = m_canvasEntryMap.begin(); it != m_canvasEntryMap.end(); ++it) { New code (here and elsewhere) should use range-based for loop, unless there's a reason not to: for (const auto& canvasEntry : m_canvasEntryMap) > Source/WebCore/inspector/InspectorCanvasAgent.cpp:84 > + for (auto canvas : toRemove) { According to mailing list discussions, in most uses of auto we should almost always have the pointer type (* or &) and constness as part of the local variable declaration. Otherwise, you might copy everything that's iterated over, rather than taking a reference. I think here we want |const auto*| to match the vector type parameter. Nit: since we have several objects per canvas, maybe rename canvas to canvasElement? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:103 > + canvas.addObserver(*this); Its a little weird to use per-element observers like this rather than unconditionally calling InspectorInstrumentation, but I suppose there's nothing morally wrong with it since CanvasObservers cannot retain canvases. Most pages probably don't have > 100 canvases so it shouldn't be a performance problem. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:115 > + auto it = canvasEntry->programIdMap.find(&program); Nit: auto&, and I usually name the result of .find as 'findResult' or 'foundEntry' > Source/WebCore/inspector/InspectorCanvasAgent.cpp:154 > +void InspectorCanvasAgent::getShaderSource(ErrorString&, const RefPtr<Inspector::InspectorObject>& programId, const String& shaderType, String* shaderSource) I would not include these two functions in the initial patch, since they don't do anything and aren't testable yet. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:156 > + // FIXME: implement for <rdar://problem/18936194> Nit: unless a feature is unannounced, you should prefer bugzilla links. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:219 > +PassRefPtr<Inspector::Protocol::Canvas::Canvas> InspectorCanvasAgent::buildObjectForCanvas(const CanvasEntry& entry, const HTMLCanvasElement& canvas) These buildObjectFor* methods are only used by this file, so maybe they should be static methods. AFAICT m_pageAgent->frameId is the only member accessed. > Source/WebCore/inspector/InspectorCanvasAgent.h:117 > + virtual void canvasChanged(HTMLCanvasElement&, const FloatRect&) { } Nit: 'override' keyword
Comment on attachment 241372 [details] Patch (Re: windows build failure, you need to add the new agent to InspectorAllInOne.cpp.)
Comment on attachment 241372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241372&action=review > Source/JavaScriptCore/inspector/protocol/Canvas.json:2 > + "domain": "Canvas", This needs: "availability": "web", Otherwise it will exist unnecessarily in JSContext inspection.
Comment on attachment 241372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241372&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:154 >> +void InspectorCanvasAgent::getShaderSource(ErrorString&, const RefPtr<Inspector::InspectorObject>& programId, const String& shaderType, String* shaderSource) > > I would not include these two functions in the initial patch, since they don't do anything and aren't testable yet. Shader related stubs will be removed and added back in https://bugs.webkit.org/show_bug.cgi?id=138593.
Created attachment 241639 [details] Patch [backend]
Created attachment 241643 [details] Patch [backend]
Created attachment 241644 [details] Patch [frontend]
Comment on attachment 241643 [details] Patch [backend] View in context: https://bugs.webkit.org/attachment.cgi?id=241643&action=review Great work. Please fix the nits before landing. > Source/JavaScriptCore/ChangeLog:8 > + relocated Canvas protocol from WebCore. Not sure this is accurate- we nuked the old Canvas.json, right? > Source/WebCore/ChangeLog:17 > + children of the DOM. In the absense of the inspector frontend the agent does only what typo: absence > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1665 > +#if ENABLE(INSPECTOR) This should not be guarded. If compiled as !ENABLE(INSPECTOR), then we should still call out to the stub (which does ENABLE(INSPECTOR) test before forwarding to didDeleteProgramImpl()) > Source/WebCore/inspector/InspectorCanvasAgent.cpp:84 > + for (const auto& canvasElement : canvasesForFrame) { Err, canvasesForFrame has element type HTMLCanvasElement*, not by reference. (Right?) I'm not sure why this compiles. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:147 > + results = Inspector::Protocol::Array<Inspector::Protocol::Canvas::Canvas>::create(); Usually we assign at the end if !errorString (otherwise, early return). But since the sub-calls can't cause an error, this is fine as-is.
Comment on attachment 241644 [details] Patch [frontend] (per IRC, This is a WIP patch)
Comment on attachment 241643 [details] Patch [backend] View in context: https://bugs.webkit.org/attachment.cgi?id=241643&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + relocated Canvas protocol from WebCore. > > Not sure this is accurate- we nuked the old Canvas.json, right? Technically it is the same protocol file; the one I created and which was originally located in WebCore. Are you referring to the older canvas inspector protocol from an earlier canvas inspection effort? >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:84 >> + for (const auto& canvasElement : canvasesForFrame) { > > Err, canvasesForFrame has element type HTMLCanvasElement*, not by reference. (Right?) I'm not sure why this compiles. auto and auto* are semantically equivalent when the r-value is a pointer type, as are auto/auto& when it is a reference type. AFAIK, in this case we are essentially taking a reference to a pointer, but since it is a reference to a pointer to const, there is no risk of the pointer address being changed. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:147 >> + results = Inspector::Protocol::Array<Inspector::Protocol::Canvas::Canvas>::create(); > > Usually we assign at the end if !errorString (otherwise, early return). But since the sub-calls can't cause an error, this is fine as-is. I've seen this pattern used in other agents.
Comment on attachment 241644 [details] Patch [frontend] View in context: https://bugs.webkit.org/attachment.cgi?id=241644&action=review Almost ready! > Source/WebInspectorUI/ChangeLog:10 > + displayed as child nodes of their respective canvas. A canvas does not have an associated content What about individual shaders, are they shown underneath the linked program? What if the shader is compiled but not linked? How are shader compilation failures displayed? > Source/WebInspectorUI/ChangeLog:13 > + New tests are missing from Changelog. They should be listed here and in the itemization below. See some recent trac change sets for the format. > Source/WebInspectorUI/UserInterface/Base/Test.js:98 > +//InspectorTest.dumpMessagesToConsole = false; Oops. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:48 > + if (this._didCompleteRequestCanvases) { It would delight me if this returned a promise, and the payload handling was also implemented using promises. It's annoying to hook this up the first time, but simplifies this code considerably, especially the mechanism to notify multiple callbacks. (Feel free to defer to a later bug.) Jono is working on a similar conversion for SourceCode.requestContent over here: https://bugs.webkit.org/show_bug.cgi?id=135777 And there are some good examples in ReplayManager, and good tips on the wiki (http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide) > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:63 > + this._pendingCanvasesForFrameCallbacks.set(frame.id, [callback]); I would name this something like |this._pendingCallbacks| or |this._waitingCallbacks|. I had to read it several times to figure out why it's named that. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:78 > + // NOTE: Should a separate canvases array be created for each callback, Historically we have not done defensive programming like this in the inspector, but I actually prefer doing this for arrays. Plus, this code is not on any hot paths, so it shouldn't make too much garbage. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:81 > + var frameId = keyValue[0]; var [frameId, callbacks] = keyValue; > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:84 > + for (var callback of callbacks) callbacks.map(canvases) > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:94 > + { So, I take it that the previous method canvasesForFrame is handling the case where we open the inspector after the page is already loaded, right? Once we've fetched all canvases once, do we ever need to ask the backend again for all canvases? > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:115 > + if (canvas) { I would turn this into two early-returns to avoid nesting. if (!canvas) return; ... if (!program) return; ... do stuff > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:140 > + delete this._pendingCanvasesForFrameCallbacks; Not sure whether this is the right thing to do, or explicitly call each callback with an error code (cancelled?). > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:149 > + for (var canvas of this._canvasIdMap.values()) { return this._canvasIdMap.values().filter(function(canvas) { ... }) > Source/WebInspectorUI/UserInterface/Models/Canvas.js:110 > + var programs = []; You can use no-args Array.prototype.slice() here instead to copy it. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:121 > + // FIXME: It may also be nice to have a better name for a canvas if it is unique. sounds like a good idea. Maybe this could go in a subtitle, since the ID could be generated and very long... please file a follow-up bugzilla bug and add the link in the FIXME. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:134 > + if (exists) Hmm, this is interesting. Can a shader be linked multiple times? I don't see how something could ever see both events. I don't see any users of the WasLinked event. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:148 > + // Do nothing. Canvases are not persisted when the inspector is This is surprising... why not? > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:40 > + function(representedObject) { return representedObject instanceof WebInspector.ShaderProgram; }, please add a name to the function after the 'function' keyword so we can know what the predicate is for without cross-referencing. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:42 > + WebInspector.ShaderProgramTreeElement We almost never break lines between arguments. Feel free to extract the functions to make this readable on one line. > LayoutTests/inspector/canvas/canvas-add-remove-events-expected.txt:1 > +FAIL: Timed out waiting for notifyDone to be called oops > LayoutTests/inspector/canvas/canvas-add-remove-events.html:30 > + if (window.GCController) Add comment to explain why we do this. > LayoutTests/inspector/canvas/canvas-add-remove-events.html:40 > + if (contextId == "2d") ===
Comment on attachment 241643 [details] Patch [backend] View in context: https://bugs.webkit.org/attachment.cgi?id=241643&action=review > Source/WebCore/inspector/InspectorCanvasAgent.h:89 > + unsigned m_canvasId = 0; > + unsigned m_objectId = 0; Is this new syntax that will always make these 0 in construction? I haven't seen it before.
Comment on attachment 241644 [details] Patch [frontend] Attachment 241644 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5546699971887104 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html inspector/canvas/canvas-add-remove-events.html
Created attachment 241649 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 241644 [details] Patch [frontend] Attachment 241644 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5280541922295808 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html inspector/canvas/canvas-add-remove-events.html
Created attachment 241652 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 241643 [details] Patch [backend] View in context: https://bugs.webkit.org/attachment.cgi?id=241643&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.h:89 >> + unsigned m_objectId = 0; > > Is this new syntax that will always make these 0 in construction? I haven't seen it before. C++11 allows non-static data members to be initialized where they are declared.
(In reply to comment #27) > Comment on attachment 241643 [details] > Patch [backend] > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241643&action=review > > > Source/WebCore/inspector/InspectorCanvasAgent.h:89 > > + unsigned m_canvasId = 0; > > + unsigned m_objectId = 0; > > Is this new syntax that will always make these 0 in construction? I haven't > seen it before. According to andersca, his preferred syntax for this feature is ... = {0}; (I think it's because that syntax is uniform across scalars, arrays and structs.. ?)
Created attachment 245260 [details] backend
Created attachment 245262 [details] frontend
Comment on attachment 245262 [details] frontend View in context: https://bugs.webkit.org/attachment.cgi?id=245262&action=review > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44 > + canvasesForFrame: function(frame, callback) Could we use promises here? Should we?
(In reply to comment #36) > Comment on attachment 245262 [details] > frontend > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245262&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44 > > + canvasesForFrame: function(frame, callback) > > Could we use promises here? Should we? I'm planning on doing a refactor to use promises throughout the CanvasManager (and associated classes) as a separate issue.
(In reply to comment #37) > (In reply to comment #36) > > Comment on attachment 245262 [details] > > frontend > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=245262&action=review > > > > > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44 > > > + canvasesForFrame: function(frame, callback) > > > > Could we use promises here? Should we? > > I'm planning on doing a refactor to use promises throughout the > CanvasManager (and associated classes) as a separate issue. Oh yes, you'd said that before. Very good.
Comment on attachment 245260 [details] backend Attachment 245260 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5734140733292544 New failing tests: webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
Created attachment 245265 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245262 [details] frontend Attachment 245262 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5636481766916096 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/canvas/canvas-context-attributes.html inspector/css/selector-dynamic-specificity.html inspector/canvas/shader-program-linked-events.html inspector/canvas/shader-compiled-events.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html inspector/canvas/canvas-add-remove-events.html inspector/test-harness-trivially-works.html
Created attachment 245266 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245262 [details] frontend Attachment 245262 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5745949745872896 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/canvas/canvas-context-attributes.html inspector/css/selector-dynamic-specificity.html inspector/canvas/shader-program-linked-events.html inspector/canvas/shader-compiled-events.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html inspector/canvas/canvas-add-remove-events.html inspector/test-harness-trivially-works.html
Created attachment 245267 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
You might need to disable local file restrictions in the develop menu for it to work.
Wrong bug. Ignore the previous comment.
Comment on attachment 245262 [details] frontend Marked obsolete, need to fix a bug and add "expanded setting" persistence to CanvasTreeElement.
Created attachment 245318 [details] frontend
Comment on attachment 245318 [details] frontend Attachment 245318 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5182615929749504 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/css/selector-dynamic-specificity.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html
Created attachment 245320 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245318 [details] frontend Attachment 245318 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6399674906836992 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/css/selector-dynamic-specificity.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html
Created attachment 245322 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 245260 [details] backend View in context: https://bugs.webkit.org/attachment.cgi?id=245260&action=review Nice and clean! Looks good to me, but I want to see the frontend portion. > Source/JavaScriptCore/inspector/protocol/Canvas.json:31 > + "id": "InfoLog", Maybe "BuildLog"? Or do we expect it to have non-build type information. > Source/JavaScriptCore/inspector/protocol/Canvas.json:71 > + "name": "getCanvases", > + "returns": [ > + { "name": "result", "type": "array", "items": { "$ref": "Canvas" }, "description": "Array of canvas objects." } > + ], > + "description": "Returns available canvases for all frames." Nit: Lets move "description" up above "returns". Same with a few of the events below. Careful of commas when moving things around. > Source/JavaScriptCore/inspector/protocol/Canvas.json:89 > + { "name": "infoLog", "$ref": "InfoLog", "description": "Array of <code>ShaderInfoLog</code> objects." } Nit: Description is now wrong. Seems the type changed to just InfoLog. > Source/JavaScriptCore/inspector/protocol/Canvas.json:124 > + "description": "Fired when a program is linked. Does not imply that link was successful." Should this say if it was successful or not? And if it was not successful, just include the Canvas.InfoLog output? In other words, when will the frontend request get*InfoLog()? Can it just be included as part of an event? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. Nit: 2015 > Source/WebCore/inspector/InspectorCanvasAgent.cpp:28 > +#if ENABLE(INSPECTOR) ENABLE(INSPECTOR) was removed in trunk. You'll need to remove these. > Source/WebCore/inspector/InspectorCanvasAgent.h:93 > + if (!value->getInteger("canvasId", m_canvasId)) Nit: ASCIILiteral("canvasId") > Source/WebCore/inspector/InspectorCanvasAgent.h:111 > + PassRefPtr<ID> asProtocolValue() const We want to move away from PassRefPtr. This can probably just be Ref<ID>. > Source/WebCore/inspector/InspectorCanvasAgent.h:142 > + struct ProgramEntry { > + CanvasEntry* canvasEntry; > + unsigned programId = {0}; > + WebGLProgram* program; > + WebGLShader* vertexShader; > + WebGLShader* fragmentShader; > + > + ProgramEntry() { } > + > + ProgramEntry(CanvasEntry* canvasEntry, unsigned id, WebGLProgram* program) > + : canvasEntry(canvasEntry) > + , programId(id) > + , program(program) > + { > + } Shouldn't these constructors nullptr initialize the WebGL pointers? > Source/WebCore/inspector/InspectorCanvasAgent.h:179 > + unsigned m_nextCanvasId; > + HashMap<HTMLCanvasElement*, CanvasEntry> m_canvasEntries; > + HashMap<const WebGLShader*, HashSet<const ProgramEntry*>> m_shaderToProgramEntries; > + Timer m_timer; > + unsigned m_lastRemovedCanvasId; Nit: Put the two unsigned members next to eachother for better readability and potentially better memory packing.
Comment on attachment 245318 [details] frontend View in context: https://bugs.webkit.org/attachment.cgi?id=245318&action=review > Source/WebInspectorUI/ChangeLog:38 > + * UserInterface/Images/Canvas2D.png: Added. > + * UserInterface/Images/Canvas2D@2x.png: Added. > + * UserInterface/Images/Canvas3D.png: Added. > + * UserInterface/Images/Canvas3D@2x.png: Added. > + * UserInterface/Images/ShaderProgram.png: Added. > + * UserInterface/Images/ShaderProgram@2x.png: Added. Is there a follow-up for better images? > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:45 > + canvasesForFrame: function(frame, callback) > + { This should probably early return if the Canvas domain was not supported. if (!window.CanvasAgent) return; Otherwise, I would expect you get an exception inspecting iOS 6 / 7 / 8 when this is called. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:47 > + program._canvas = this; Interesting. Normally we would just have a setter, but I like this approach so we don't expose a setter as public API. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:58 > +// Keep these in sync with the "CanvasType" enum defined by the "Canvas" domain. Is this comment necessary? Also, I think you renamed CanvasType to ContextType. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:81 > + case "canvas-2d": Note that the enum in the protocol generates values here. CanvasAgent.ContextType.Canvas2d CanvasAgent.ContextType.Webgl Both of these could be fixed up in scripts/codegen to produce nicer enum names like: CanvasAgent.ContextType.Canvas2D CanvasAgent.ContextType.WebGL > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:43 > +// Keep these in sync with the "ShaderType" enum defined by the "Canvas" domain. Ditto. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:68 > + case "vertex": > + return WebInspector.ShaderProgram.ShaderType.Vertex; > + case "fragment": > + return WebInspector.ShaderProgram.ShaderType.Fragment; CanvasAgent.ShaderType.Vertex, CanvasAgent.ShaderType.Fragment > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:81 > + case WebInspector.ShaderProgram.ShaderType.Vertex: > + return "vertex"; > + case WebInspector.ShaderProgram.ShaderType.Fragment: > + return "fragment"; And more importantly, here. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:164 > + shaderWasCompiled: function(shaderTypePayload) > + { > + var shaderType = WebInspector.ShaderProgram.ShaderTypeFromPayload(shaderTypePayload); > + this._getCachedShaderInfo(shaderType).isCurrent = false; I think this is a great reason to just include build info along with the shader when it was compiled and failed. We would know immediately. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:186 > + .filter(function(msg) { return msg && msg.trim().length; }) > + .map(function(msg) { return msg.trim(); }); Flip these lines and you only have to do trim() once per line, instead of once per line and again per-non-empty line. Also, Is the "msg &&" necessary in the filter? Lets drop it. > Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. Nit: 2015 > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:36 > + this._updateExpandedSetting(); I think this could be inlined. I'd expect to see the WebInspector.Setting in the constructor. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:54 > +WebInspector.CanvasTreeElement.CanvasIconStyle2dClassName = "canvas-2d-icon"; > +WebInspector.CanvasTreeElement.CanvasIconStyle3dClassName = "canvas-3d-icon"; Sometimes I wonder if we should just inline these for use once class names. This is so verbose, it has a negligible runtime cost, and it makes searching a two step process. > Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:269 > + function() { > + // FIXME: child count callback needs to update with current count. > + // Having WebInspector.Frame listen to CanvasManager events and retain > + // a list of canvases is a possible solution. > + return canvases.length; > + }.bind(this), Unnecessary bind at the moment. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. Nit: 2015
Comment on attachment 245260 [details] backend View in context: https://bugs.webkit.org/attachment.cgi?id=245260&action=review > Source/WebCore/ChangeLog:16 > + practical way to identify those rendering contexts that belong to a frame but are not Can the frame keep a set of contexts? In some other code, I've seen code use a static global HashMap<Frame*, Set<T>> to track such things. Clearly, this can be fixed at a later time. > Source/JavaScriptCore/inspector/protocol/Canvas.json:27 > + "enum": ["fragment", "vertex"], This is fine as-is. In the future we may need to prefix with the webgl version. > Source/JavaScriptCore/inspector/protocol/Canvas.json:36 > + { "name": "lastError", "type": "string", "description": "Diagnostic messages, warning messages, and other information about the compile or link operation." } Is this optional/possibly empty? Is this a list of errors or just one big error string? The 'last' prefix probably comes from GL but not sure it's semantically useful here. You can always choose a better name than GL and note the mapping (as above). > Source/WebCore/inspector/InspectorCanvasAgent.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY I believe the most recent copyright block includes "and its contributors". > Source/WebCore/inspector/InspectorCanvasAgent.cpp:60 > + , m_lastRemovedCanvasId(0) could use default initializers in the header file, like so: unsigned m_lastRemovedCanvasId {0}; > Source/WebCore/inspector/InspectorCanvasAgent.cpp:217 > + CanvasEntry* canvasEntry = getCanvasEntry(context.canvas()); These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks? Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:295 > + // Due to the single-process model used in WebKit1, the event must be dispatched from a timer to prevent (Gross. I thought even WK1 forces async message receives, but it doesn't look like it.) > Source/WebCore/inspector/InspectorCanvasAgent.cpp:298 > + m_timer.startOneShot(0); This mechanism probably won't work if more than one canvas is destroyed per GC, right? You would need a vector of pending removed ids. An alternative is to create a runnable and call RunLoop::main()->dispatch() but I don't see this approach used very often.
Comment on attachment 245260 [details] backend View in context: https://bugs.webkit.org/attachment.cgi?id=245260&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:217 >> + CanvasEntry* canvasEntry = getCanvasEntry(context.canvas()); > > These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks? > Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions. In particular, I'm concerned about the shader/program instrumentation callbacks, which can come at any time and contain the same assertions. The CanvasObserver callbacks should be immune to this.
Comment on attachment 245318 [details] frontend View in context: https://bugs.webkit.org/attachment.cgi?id=245318&action=review >> Source/WebInspectorUI/UserInterface/Models/Canvas.js:81 >> + case "canvas-2d": > > Note that the enum in the protocol generates values here. > > CanvasAgent.ContextType.Canvas2d > CanvasAgent.ContextType.Webgl > > Both of these could be fixed up in scripts/codegen to produce nicer enum names like: > > CanvasAgent.ContextType.Canvas2D > CanvasAgent.ContextType.WebGL I believe that the enum keys are already formatted with special cases. The values are intentionally not, so as to avoid guessing the capitalization regime. To use them, you would need to make an if-else chain, since arbitrary expressions can't go in case labels. >> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:36 >> + this._updateExpandedSetting(); > > I think this could be inlined. I'd expect to see the WebInspector.Setting in the constructor. +1
(In reply to comment #56) > Comment on attachment 245260 [details] > backend > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245260&action=review > > >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:217 > >> + CanvasEntry* canvasEntry = getCanvasEntry(context.canvas()); > > > > These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks? > > Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions. > > In particular, I'm concerned about the shader/program instrumentation > callbacks, which can come at any time and contain the same assertions. The > CanvasObserver callbacks should be immune to this. InspectorCanvasAgent always exists on the InspectorController, which always exists on the WebCore::Page. No context can be created "pre-initialization", the CanvasAgent will always exist and be tracking Canvases. This is akin to ConsoleAgent which always exists, to store up a queue of console.log/* messages before the inspector has been opened for a page.
(In reply to comment #58) > InspectorCanvasAgent always exists on the InspectorController, which always > exists on the WebCore::Page. No context can be created "pre-initialization", > the CanvasAgent will always exist and be tracking Canvases. > > This is akin to ConsoleAgent which always exists, to store up a queue of > console.log/* messages before the inspector has been opened for a page. Ah, ok. We should clean up the agent lifecycles so this is obvious.
I think these patches are pretty much ready to go, pending the comments already posted. Please post an updated patch for EWS, I will add r+, and we can try to land it.
Comment on attachment 245260 [details] backend View in context: https://bugs.webkit.org/attachment.cgi?id=245260&action=review > Source/WebCore/inspector/InspectorAllInOne.cpp:34 > +#include "InspectorCanvasAgent.cpp" It would be very helpful if these InspectorCanvasAgent files were also added to the Visual Studio project as "excluded from build". They will be built using this InspectorAllInOne file, but having them in the VS solution makes it easy to search and locate this code when debugging in the future.
Lets try to get these rebased, building and landed this week.
> > Source/WebCore/inspector/InspectorAllInOne.cpp:34 > > +#include "InspectorCanvasAgent.cpp" > > It would be very helpful if these InspectorCanvasAgent files were also added > to the Visual Studio project as "excluded from build". They will be built > using this InspectorAllInOne file, but having them in the VS solution makes > it easy to search and locate this code when debugging in the future. I agree. Let's do this as part of https://bugs.webkit.org/show_bug.cgi?id=137326.
Created attachment 249439 [details] [Patch] Backend
Created attachment 249440 [details] [Patch] Frontend
Created attachment 249442 [details] [Patch] Tests
Comment on attachment 249440 [details] [Patch] Frontend Attachment 249440 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6262774938206208 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html
Created attachment 249445 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249440 [details] [Patch] Frontend Attachment 249440 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5981299961495552 New failing tests: inspector/model/parse-script-syntax-tree.html inspector/css/matched-style-properties.html inspector/protocol-promise-result.html inspector/event-listener.html inspector/test-harness-trivially-works.html inspector/css/pseudo-element-matches.html inspector/css/selector-specificity.html inspector/event-listener-set.html
Created attachment 249446 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249442 [details] [Patch] Tests Attachment 249442 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4898797477953536 New failing tests: inspector/canvas/canvas-css-name.html inspector/canvas/canvas-context-attributes.html inspector/canvas/shader-program-linked-events.html inspector/canvas/canvas-add-remove-events.html inspector/canvas/shader-compiled-events.html
Created attachment 249447 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249439 [details] [Patch] Backend Attachment 249439 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5277612519718912 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html
Created attachment 249450 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249439 [details] [Patch] Backend Attachment 249439 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5840562473140224 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html
Created attachment 249451 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249442 [details] [Patch] Tests Attachment 249442 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5586719705399296 New failing tests: inspector/canvas/canvas-css-name.html inspector/canvas/canvas-context-attributes.html inspector/canvas/shader-program-linked-events.html inspector/canvas/canvas-add-remove-events.html inspector/canvas/shader-compiled-events.html
Created attachment 249452 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249455 [details] [Patch] Backend/Frontend/Tests
Comment on attachment 249455 [details] [Patch] Backend/Frontend/Tests Attachment 249455 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6199132112814080 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html
Created attachment 249463 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249472 [details] [PATCH] Backend/Frontend/Tests
Comment on attachment 249472 [details] [PATCH] Backend/Frontend/Tests Attachment 249472 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5517818229424128 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html
Created attachment 249475 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249472 [details] [PATCH] Backend/Frontend/Tests Attachment 249472 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5933383158857728 New failing tests: webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html
Created attachment 249477 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249472 [details] [PATCH] Backend/Frontend/Tests View in context: https://bugs.webkit.org/attachment.cgi?id=249472&action=review Looks good. The layout test failures seem real though. Some minor suggestions / comments. > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:141 > + getProgramInfoLog(program, callback) > + { > + CanvasAgent.getProgramInfoLog(program.id, callback); > + } > + > + getShaderInfoLog(program, shaderType, callback) > + { > + CanvasAgent.getShaderInfoLog(program.id, WebInspector.ShaderProgram.shaderTypeToPayload(shaderType), callback); > + } Not used yet. See above. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:46 > + this._contextAttributes = contextAttributes; This leaks data from the protocol by just using the data directly. We should have a class or controlled object for attributes that isn't a direct use of the protocol object. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:51 > + program._canvas = this; Should have a setter or updateCanvas function, but I did this for Resource's parentFrame so I suck too. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:93 > + static resetUniqueDisplayNameNumbers() resetUniqueDisplayNameNumber? > Source/WebInspectorUI/UserInterface/Models/Canvas.js:141 > + return WebInspector.UIString("CSS canvas '%s'").format(this._name); Might want to use curly quotes (“”) instead of single straight quotes since it is a UI string. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:136 > + getInfoLog(callback) > + { > + if (this._isCurrent) { > + callback(this._linkStatus, this._errorMessages); > + return; > + } > + > + function processInfoLog(error, status, messages) > + { > + this._linkStatus = this._statusFromBoolean(status); > + this._errorMessages = messages; > + this._isCurrent = true; > + > + callback(this._linkStatus, this._errorMessages); > + } > + > + WebInspector.canvasManager.getProgramInfoLog(this, processInfoLog.bind(this)); > + } > + > + getShaderInfoLog(shaderType, callback) > + { > + var shaderInfo = this._getCachedShaderInfo(shaderType); > + if (shaderInfo.isCurrent) { > + callback(shaderInfo.compileStatus, shaderInfo.errorMessages); > + return; > + } > + > + function processShaderInfoLog(error, status, messages) > + { > + var shaderInfo = this._getCachedShaderInfo(shaderType); > + shaderInfo.compileStatus = this._statusFromBoolean(status); > + shaderInfo.errorMessages = messages; > + shaderInfo.isCurrent = true; > + > + callback(shaderInfo.compileStatus, shaderInfo.errorMessages); > + } > + > + WebInspector.canvasManager.getShaderInfoLog(this, shaderType, processShaderInfoLog.bind(this)); > + } These aren't used yet. We talked about doing errors differently by sending things through the ConsoleAgent, so errors work even if you aren't looking at the canvas yet. ShaderProgram or the shader sub-objects should be a SourceCode subclass to make that work. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:166 > + this.dispatchEventToListeners(WebInspector.ShaderProgram.Event.ShaderWasCompiled, { program: this, shaderType: shaderType, status: compileStatus }); Nit: No spaces padding the inside of the {}. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:196 > + Fragment: "shader-type-fragment", > + Vertex: "shader-type-vertex" These can be Symbol("shader-type-fragment"), etc. > Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:202 > + Succeeded: "shader-program-status-succeded", > + Failed: "shader-program-status-failed", > + None: "shader-program-status-none" Ditto. > Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:34 > +WebInspector.CanvasObserver = function() > +{ > + // FIXME: Convert this to a WebInspector.Object subclass, and call super(). > + // WebInspector.Object.call(this); > +}; > + > +WebInspector.CanvasObserver.prototype = { > + constructor: WebInspector.CanvasObserver, > + __proto__: WebInspector.Object.prototype, I converted the other observers today. They are super simple and clean now. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:30 > +WebInspector.CanvasTreeElement = function(canvas) > +{ > + console.assert(canvas instanceof WebInspector.Canvas); > + > + WebInspector.FolderizedTreeElement.call(this, WebInspector.CanvasTreeElement.StyleClassName, canvas.displayName, null, canvas, false); This will need to convert to ES6 to work in TOT. You should inline the value for WebInspector.CanvasTreeElement.StyleClassName and remove it. We are switching how we do one-off class names. WebInspector.CanvasTreeElement.StyleClassName isn't even defined! You are passing undefined here. It works because of the addStyleClass you do later. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:32 > + this.addClassName(WebInspector.CanvasTreeElement.CanvasIconStyleClassName); Don't need this since you can pass it in the super call. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:57 > +WebInspector.CanvasTreeElement.CanvasIconStyleClassName = "canvas-icon"; I don't see any uses of this. Should this have been WebInspector.CanvasTreeElement.CanvasIconStyleClassName? > Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:55 > + content: url(../Images/Canvas.svg); We typically use PNGs for these so they all have a similar style. What does this look like? > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:26 > +WebInspector.ShaderProgramTreeElement = function(program) Will need to convert to ES6 now. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:29 > + WebInspector.GeneralTreeElement.call(this, [WebInspector.ShaderProgramTreeElement.StyleClassName, WebInspector.ShaderProgramTreeElement.ShaderProgramIconStyleClassName], program.displayName, null, program, false); I think you are referencing undefined here. WebInspector.ShaderProgramTreeElement.StyleClassName does not exist. > Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:34 > +WebInspector.ShaderProgramTreeElement.ShaderProgramIconStyleClassName = "shader-program-icon"; Inline where this is used if it does not need to be shared.
> Looks good. The layout test failures seem real though. Some minor > suggestions / comments. The failures were caused by a bug which allowed null WebGLShader pointers to be used in a HashMap. Fixed. > > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:141 > > + getProgramInfoLog(program, callback) > > + { > > + CanvasAgent.getProgramInfoLog(program.id, callback); > > + } > > + > > + getShaderInfoLog(program, shaderType, callback) > > + { > > + CanvasAgent.getShaderInfoLog(program.id, WebInspector.ShaderProgram.shaderTypeToPayload(shaderType), callback); > > + } > > Not used yet. See above. Code and tests related to shader compilation and linking have been removed and will be merged into https://bugs.webkit.org/show_bug.cgi?id=143236. > > Source/WebInspectorUI/UserInterface/Models/Canvas.js:93 > > + static resetUniqueDisplayNameNumbers() > > resetUniqueDisplayNameNumber? The name was chosen to be consistent with an identical idiom used by Script and CSSStyleSheet. > > Source/WebInspectorUI/UserInterface/Models/Canvas.js:141 > > + return WebInspector.UIString("CSS canvas '%s'").format(this._name); > > Might want to use curly quotes (“”) instead of single straight quotes since > it is a UI string. Agreed. > > Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:55 > > + content: url(../Images/Canvas.svg); > > We typically use PNGs for these so they all have a similar style. What does > this look like? See attachment.
Created attachment 249770 [details] Canvas resource icon
Created attachment 249798 [details] [Patch] Backend/Frontend/Tests
Comment on attachment 249798 [details] [Patch] Backend/Frontend/Tests Attachment 249798 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4955296430555136 New failing tests: media/invalid-media-url-crash.html inspector-protocol/dom/remove-multiple-nodes.html
Created attachment 249814 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249798 [details] [Patch] Backend/Frontend/Tests Clearing flags on attachment: 249798 Committed r182186: <http://trac.webkit.org/changeset/182186>
All reviewed patches have been landed. Closing bug.
(In reply to comment #93) > Comment on attachment 249798 [details] > [Patch] Backend/Frontend/Tests > > Clearing flags on attachment: 249798 > > Committed r182186: <http://trac.webkit.org/changeset/182186> It broke the Apple Windows debug build, see the bot for details.
Re-opened since this is blocked by bug 143270
I had to roll this out because all the WebGL tests crash on Apple Debug bots https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/3189
As evidenced by the bajillion comments here, this bug and patch are too big in scope. We should split up into at least the following parts (each with own tests): - Instrument canvas2d contexts in backend - Instrument webgl contexts in backend - Instrument shader programs - Add frontend support for listing contexts - Add frontend support for showing shaders
I think this bug has outlived its usefulness. Separate parts that still need to be landed are in other bugs/radars.