Bug 174311 – Web Inspector: Refactoring: replace InspectorCanvasAgent::CanvasEntry with a helper class

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

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.

[Matt Baker]

Created attachment 314985 [details]
Patch

[Matt Baker]

Created attachment 314986 [details]
Patch

[Build Bot]

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.

[Matt Baker]

Created attachment 314989 [details]
Patch

[Build Bot]

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.

[Matt Baker]

Created attachment 314995 [details]
Patch

[Build Bot]

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.

[Devin Rousso]

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.

[Matt Baker]

(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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 314997 [details]
Patch

[Build Bot]

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.

[Matt Baker]

Created attachment 315000 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #13)
> Created attachment 315000 [details]
> Patch

Added missing file to CMakeLists.txt and updated Changelogs.

[Build Bot]

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.

[Devin Rousso]

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*`?

[Matt Baker]

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.

[Matt Baker]

Created attachment 315030 [details]
Patch

[Build Bot]

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.

[WebKit Commit Bot]

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

[Matt Baker]

Created attachment 315844 [details]
Patch

[Build Bot]

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.

[WebKit Commit Bot]

Comment on attachment 315844 [details]
Patch

Clearing flags on attachment: 315844

Committed r219634: <http://trac.webkit.org/changeset/219634>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.