The current helper structure `struct CanvasEntry` is inconvenient as a HashMap value type, and doesn't scale well as canvas instrumentation grows. Add InspectorCanvas, following the pattern used by InspectorStyleSheet and other ancillary Inspector classes.
Created attachment 314985 [details] Patch
Created attachment 314986 [details] Patch
Attachment 314986 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314989 [details] Patch
Attachment 314989 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314995 [details] Patch
Attachment 314995 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 314995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314995&action=review I think you need to add InspectorCanvas.cpp to InspectorAllInOne.cpp for win/wpe. > Source/WebCore/inspector/InspectorCanvas.cpp:71 > + contextType = Inspector::Protocol::Canvas::ContextType::Canvas2D; Style: this should be indented. > Source/WebCore/inspector/InspectorCanvas.cpp:74 > + contextType = Inspector::Protocol::Canvas::ContextType::WebGL; Ditto. > Source/WebCore/inspector/InspectorCanvas.cpp:78 > + contextType = Inspector::Protocol::Canvas::ContextType::WebGL2; Ditto. > Source/WebCore/inspector/InspectorCanvas.cpp:82 > + contextType = Inspector::Protocol::Canvas::ContextType::WebGPU; Ditto. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:244 > + String identifier = unbindCanvas(*inspectorCanvas); > if (m_enabled) > - m_frontendDispatcher->canvasRemoved(canvasEntry.identifier); > + m_frontendDispatcher->canvasRemoved(identifier); See comment below. if (unbindCanvas(*inspectorCanvas) && m_enabled) m_frontendDispatcher->canvasRemoved(inspectorCanvas->identifier()); > Source/WebCore/inspector/InspectorCanvasAgent.cpp:297 > + auto* inspectorCanvas = findInspectorCanvas(canvasElement); NIT: in the above functions, you've been using `InspectorCanvas*` instead of `auto*`. Please stay consistent to using one or the other. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:302 > + String identifier = unbindCanvas(*inspectorCanvas); See comment below. if (!unbindCanvas(*inspectorCanvas) || !m_enabled) return; ... m_removedCanvasIdentifiers.append(inspectorCanvas->identifier()); > Source/WebCore/inspector/InspectorCanvasAgent.cpp:341 > +String InspectorCanvasAgent::unbindCanvas(InspectorCanvas& inspectorCanvas) Is it necessary to have this function return the identifier of the InspectorCanvas? I'd rather have this return a boolean as to whether the remove was successful or not. The callsites still have access to the InspectorCanvas, so they can just get the identifier from that.
(In reply to Build Bot from comment #7) > Attachment 314995 [details] did not pass style-queue: > > > ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces > around : [whitespace/init] [4] > Total errors found: 1 in 12 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This is a false positive. Filed https://bugs.webkit.org/show_bug.cgi?id=174315.
Comment on attachment 314995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314995&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:297 >> + auto* inspectorCanvas = findInspectorCanvas(canvasElement); > > NIT: in the above functions, you've been using `InspectorCanvas*` instead of `auto*`. Please stay consistent to using one or the other. Good catch. I meant to fix these now that InspectorCanvas::create is returning a Ref instead of a RefPtr. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:341 >> +String InspectorCanvasAgent::unbindCanvas(InspectorCanvas& inspectorCanvas) > > Is it necessary to have this function return the identifier of the InspectorCanvas? I'd rather have this return a boolean as to whether the remove was successful or not. The callsites still have access to the InspectorCanvas, so they can just get the identifier from that. I adopted the convention from InspectorCSSAgent::unbindStyleSheet. It comes in handy when the identifier is needed after removing the canvas entry, such as in InspectorCanvasAgent::canvasDestoyed. Also, after the canvas is unbound the InspectorCanvas* is not guaranteed to be valid. The identifier could be copied beforehand, but this feels cleaner to me.
Created attachment 314997 [details] Patch
Attachment 314997 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 315000 [details] Patch
(In reply to Matt Baker from comment #13) > Created attachment 315000 [details] > Patch Added missing file to CMakeLists.txt and updated Changelogs.
Attachment 315000 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315000&action=review r=me > Source/WebCore/inspector/InspectorCanvasAgent.cpp:241 > + for (auto& inspectorCanvas : inspectorCanvases) { Shouldn't this be an `auto*`?
Comment on attachment 315000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315000&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:241 >> + for (auto& inspectorCanvas : inspectorCanvases) { > > Shouldn't this be an `auto*`? It should. As written the type will be InspectorCanvas*&, which isn't an error but is weird.
Created attachment 315030 [details] Patch
Attachment 315030 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315030 [details] Patch Rejecting attachment 315030 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 315030, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: nes). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/inspector/canvas/css-canvas-clients-expected.txt patching file LayoutTests/inspector/canvas/requestContent-2d-expected.txt patching file LayoutTests/inspector/canvas/requestNode-expected.txt patching file LayoutTests/inspector/canvas/resolveCanvasContext-2d-expected.txt Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/4143460
Created attachment 315844 [details] Patch
Attachment 315844 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:57: Missing spaces around : [whitespace/init] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315844 [details] Patch Clearing flags on attachment: 315844 Committed r219634: <http://trac.webkit.org/changeset/219634>
All reviewed patches have been landed. Closing bug.