Bug 174311 - Web Inspector: Refactoring: replace InspectorCanvasAgent::CanvasEntry with a helper class
Summary: Web Inspector: Refactoring: replace InspectorCanvasAgent::CanvasEntry with a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-10 07:40 PDT by Matt Baker
Modified: 2017-07-18 15:24 PDT (History)
4 users (show)

See Also:


Attachments
Patch (35.61 KB, patch)
2017-07-10 08:59 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (35.74 KB, patch)
2017-07-10 09:08 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (37.01 KB, patch)
2017-07-10 09:22 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (37.59 KB, patch)
2017-07-10 10:50 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (34.34 KB, patch)
2017-07-10 11:18 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (38.54 KB, patch)
2017-07-10 11:42 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (38.54 KB, patch)
2017-07-10 14:27 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (38.51 KB, patch)
2017-07-18 14:53 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-07-10 07:40:41 PDT
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.
Comment 1 Matt Baker 2017-07-10 08:59:51 PDT
Created attachment 314985 [details]
Patch
Comment 2 Matt Baker 2017-07-10 09:08:32 PDT
Created attachment 314986 [details]
Patch
Comment 3 Build Bot 2017-07-10 09:11:25 PDT
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.
Comment 4 Matt Baker 2017-07-10 09:22:47 PDT
Created attachment 314989 [details]
Patch
Comment 5 Build Bot 2017-07-10 09:25:05 PDT
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.
Comment 6 Matt Baker 2017-07-10 10:50:45 PDT
Created attachment 314995 [details]
Patch
Comment 7 Build Bot 2017-07-10 10:53:09 PDT
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 8 Devin Rousso 2017-07-10 11:02:21 PDT
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.
Comment 9 Matt Baker 2017-07-10 11:07:01 PDT
(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 10 Matt Baker 2017-07-10 11:16:35 PDT
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.
Comment 11 Matt Baker 2017-07-10 11:18:12 PDT
Created attachment 314997 [details]
Patch
Comment 12 Build Bot 2017-07-10 11:19:53 PDT
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.
Comment 13 Matt Baker 2017-07-10 11:42:15 PDT
Created attachment 315000 [details]
Patch
Comment 14 Matt Baker 2017-07-10 11:43:46 PDT
(In reply to Matt Baker from comment #13)
> Created attachment 315000 [details]
> Patch

Added missing file to CMakeLists.txt and updated Changelogs.
Comment 15 Build Bot 2017-07-10 11:44:56 PDT
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 16 Devin Rousso 2017-07-10 14:19:19 PDT
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 17 Matt Baker 2017-07-10 14:22:15 PDT
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.
Comment 18 Matt Baker 2017-07-10 14:27:53 PDT
Created attachment 315030 [details]
Patch
Comment 19 Build Bot 2017-07-10 16:04:54 PDT
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 20 WebKit Commit Bot 2017-07-18 13:17:03 PDT
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
Comment 21 Matt Baker 2017-07-18 14:53:35 PDT
Created attachment 315844 [details]
Patch
Comment 22 Build Bot 2017-07-18 14:55:35 PDT
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 23 WebKit Commit Bot 2017-07-18 15:24:58 PDT
Comment on attachment 315844 [details]
Patch

Clearing flags on attachment: 315844

Committed r219634: <http://trac.webkit.org/changeset/219634>
Comment 24 WebKit Commit Bot 2017-07-18 15:24:59 PDT
All reviewed patches have been landed.  Closing bug.