Bug 173807 (WebInspectorCanvasRecording) - Web Inspector: [META] Allow canvas actions to be recorded/replayed
Summary: Web Inspector: [META] Allow canvas actions to be recorded/replayed
Status: ASSIGNED
Alias: WebInspectorCanvasRecording
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 174066 175462 WebInspectorCanvasTab 176009 178282 179205 180593 138941 172623 174481 174482 174483 174484 174486 174663 174967 174968 175166 175282 175283 175284 175448 175451 176008 176178 176441 176547 176569 176762 176822 176893 176936 176988 177032 177606 177762 178185 178188 178302 178767 178805 178807 178814 178980 179072 179347 180509 180596 180597 180839 181341 181865 182950 182995 183647 184990 185152 185153 185635 185758 189860 190305 190306 190473 190854 190856 191553 191628 193262 193476 195311 195324 195732 198952 198953 198955
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-24 01:27 PDT by Devin Rousso
Modified: 2019-06-17 22:26 PDT (History)
9 users (show)

See Also:


Attachments
[Patch] WIP (184.07 KB, patch)
2017-06-24 01:28 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] WIP (198.83 KB, patch)
2017-06-26 10:51 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (224.16 KB, patch)
2017-06-26 20:48 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (233.18 KB, patch)
2017-06-27 22:25 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Video] After Patch is applied (33.11 MB, video/quicktime)
2017-06-27 22:26 PDT, Devin Rousso
no flags Details
[Patch] WIP (254.98 KB, patch)
2017-06-28 22:40 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (257.90 KB, patch)
2017-07-07 10:44 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (337.71 KB, patch)
2017-07-09 23:14 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (338.43 KB, patch)
2017-07-11 19:02 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (270.19 KB, patch)
2017-07-18 22:46 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.29 MB, application/zip)
2017-07-18 23:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.40 MB, application/zip)
2017-07-18 23:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.61 MB, application/zip)
2017-07-19 00:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.10 MB, application/zip)
2017-07-19 00:20 PDT, Build Bot
no flags Details
[Patch] WIP (341.46 KB, patch)
2017-07-19 01:22 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.73 MB, application/zip)
2017-07-19 05:55 PDT, Build Bot
no flags Details
[Patch] WIP (392.44 KB, patch)
2017-07-21 16:16 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1009.43 KB, application/zip)
2017-07-21 17:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.30 MB, application/zip)
2017-07-21 17:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.87 MB, application/zip)
2017-07-21 17:53 PDT, Build Bot
no flags Details
[Patch] WIP (392.88 KB, patch)
2017-07-22 01:11 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (948.63 KB, application/zip)
2017-07-22 02:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (971.13 KB, application/zip)
2017-07-22 02:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.96 MB, application/zip)
2017-07-22 02:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (964.86 KB, application/zip)
2017-07-22 02:55 PDT, Build Bot
no flags Details
[Patch] Attempt Modifying IDL (304.33 KB, patch)
2017-07-23 16:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Attempt Modifying IDL (303.52 KB, patch)
2017-07-23 20:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Attempt Modifying IDL (267.31 KB, patch)
2017-07-23 22:47 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Attempt Modifying IDL (267.96 KB, patch)
2017-07-23 23:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Attempt Modifying IDL (267.05 KB, patch)
2017-07-24 00:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] Modifying IDL (295.71 KB, patch)
2017-07-24 13:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-06-24 01:27:51 PDT
Since <https://webkit.org/b/138941> we are able to show all active contexts on the page. We should also be able to record all actions made to that canvas in the next active frame.
Comment 1 Devin Rousso 2017-06-24 01:28:06 PDT
Created attachment 313783 [details]
[Patch] WIP
Comment 2 Devin Rousso 2017-06-26 10:51:24 PDT
Created attachment 313853 [details]
[Patch] WIP
Comment 3 Devin Rousso 2017-06-26 20:48:35 PDT
Created attachment 313891 [details]
[Patch] WIP
Comment 4 Devin Rousso 2017-06-27 22:25:57 PDT
Created attachment 313989 [details]
[Patch] WIP
Comment 5 Devin Rousso 2017-06-27 22:26:56 PDT
Created attachment 313990 [details]
[Video] After Patch is applied
Comment 6 Joseph Pecoraro 2017-06-28 11:45:00 PDT
Comment on attachment 313989 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=313989&action=review

Just skimmed the backend.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1058
> +static inline String stringForWindingRule(CanvasRenderingContext2D::WindingRule windingRule)

The `inline` here should not be needed.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068
> +    return "";

Could return just `String()` which is a tiny bit cheaper than `""`. In any case, this shouldn't be reached ;).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1532
> +            [&] (RefPtr<HTMLImageElement>& imageElement) {
> +                source = imageElement->currentSrc().string();
> +            },
> +            [&] (RefPtr<HTMLVideoElement>& videoElement) {
> +                // get dataURL of current frame
> +                source = videoElement->currentSrc().string();
> +            },

We probably can't depend on src alone. An image might be a `app://my-image` only accessible by the inspected page or an animated image at a particular frame. A video might be at a particular time / frame in the video. It would be best to get a dataURL of what is showing.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:218
> +    static inline String stringForImageSmoothingQuality(ImageSmoothingQuality quality)

No need for `inline` again. But this probably doesn't need to be inline or in the header.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:323
> +    bool isRadial = canvasGradient.gradient().isRadial();

Every use of the gradient is `canvasGradient.gradient()`. Might read better to pull that out into a local.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:325
> +    object->setString("type", isRadial ? "radial-gradient" : "linear-gradient");

ASCIILiteral

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:393
> +    RefPtr<InspectorObject> attributes = InspectorObject::create();
> +    RefPtr<InspectorObject> functions = InspectorObject::create();

I think a bunch of these which don't expect to move beyond this function can be Ref<> instead of RefPtr<>. I'd expect the usage to be the same but its just clearer that this won't be passed around.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:480
> +                [&] (unsigned value) { parametersData->addItem(static_cast<int>(value)); },

Should we assert that the `unsigned` value is not > INT_MAX?

> Source/WebCore/page/PageConsoleClient.cpp:217
> +void PageConsoleClient::record(JSC::ExecState*, Ref<ScriptArguments>&& arguments)

`console.record` should be a separate bug + patch.
Comment 7 Devin Rousso 2017-06-28 22:40:48 PDT
Created attachment 314117 [details]
[Patch] WIP
Comment 8 Radar WebKit Bug Importer 2017-07-04 04:39:57 PDT
<rdar://problem/33122149>
Comment 9 Devin Rousso 2017-07-07 10:44:43 PDT
Created attachment 314854 [details]
[Patch] WIP
Comment 10 Devin Rousso 2017-07-09 23:14:18 PDT
Created attachment 314962 [details]
[Patch] WIP
Comment 11 Devin Rousso 2017-07-11 19:02:10 PDT
Created attachment 315200 [details]
[Patch] WIP
Comment 12 Devin Rousso 2017-07-18 22:46:43 PDT
Created attachment 315897 [details]
[Patch] WIP
Comment 13 Build Bot 2017-07-18 23:51:35 PDT
Comment on attachment 315897 [details]
[Patch] WIP

Attachment 315897 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4146242

New failing tests:
inspector/canvas/recording-2d.html
fast/canvas/2d.setTransform.matrix.html
Comment 14 Build Bot 2017-07-18 23:51:36 PDT
Created attachment 315903 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 15 Build Bot 2017-07-18 23:56:06 PDT
Comment on attachment 315897 [details]
[Patch] WIP

Attachment 315897 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4146245

New failing tests:
inspector/canvas/recording-2d.html
fast/canvas/2d.setTransform.matrix.html
Comment 16 Build Bot 2017-07-18 23:56:07 PDT
Created attachment 315904 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-07-19 00:15:08 PDT
Comment on attachment 315897 [details]
[Patch] WIP

Attachment 315897 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4146307

New failing tests:
fast/canvas/2d.setTransform.matrix.html
Comment 18 Build Bot 2017-07-19 00:15:09 PDT
Created attachment 315907 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 19 Build Bot 2017-07-19 00:20:55 PDT
Comment on attachment 315897 [details]
[Patch] WIP

Attachment 315897 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4146282

New failing tests:
inspector/canvas/recording-2d.html
fast/canvas/2d.setTransform.matrix.html
Comment 20 Build Bot 2017-07-19 00:20:57 PDT
Created attachment 315908 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Devin Rousso 2017-07-19 01:22:18 PDT
Created attachment 315914 [details]
[Patch] WIP
Comment 22 Build Bot 2017-07-19 01:26:29 PDT
Attachment 315914 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorCanvasAgent.cpp:588:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4915:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/inspector/InspectorCanvasActionTypes.h:55:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 3 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Build Bot 2017-07-19 05:55:07 PDT
Comment on attachment 315914 [details]
[Patch] WIP

Attachment 315914 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4147462

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Comment 24 Build Bot 2017-07-19 05:55:09 PDT
Created attachment 315915 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 25 Devin Rousso 2017-07-21 16:16:32 PDT
Created attachment 316135 [details]
[Patch] WIP
Comment 26 Build Bot 2017-07-21 16:20:43 PDT
Attachment 316135 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorCanvas.cpp:43:  "ImageData.h" already included at Source/WebCore/inspector/InspectorCanvas.cpp:42  [build/include] [4]
Total errors found: 1 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Build Bot 2017-07-21 17:23:56 PDT
Comment on attachment 316135 [details]
[Patch] WIP

Attachment 316135 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4164287

New failing tests:
inspector/canvas/recording-2d.html
Comment 28 Build Bot 2017-07-21 17:23:57 PDT
Created attachment 316148 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 29 Build Bot 2017-07-21 17:38:30 PDT
Comment on attachment 316135 [details]
[Patch] WIP

Attachment 316135 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4164347

New failing tests:
inspector/canvas/recording-2d.html
Comment 30 Build Bot 2017-07-21 17:38:32 PDT
Created attachment 316150 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 31 Build Bot 2017-07-21 17:53:23 PDT
Comment on attachment 316135 [details]
[Patch] WIP

Attachment 316135 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4164331

New failing tests:
inspector/canvas/recording-2d.html
Comment 32 Build Bot 2017-07-21 17:53:25 PDT
Created attachment 316152 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 33 Sam Weinig 2017-07-21 17:56:22 PDT
Perhaps this has already been discussed, but it seems unfortunate for us to have graphics recording at two layers. At the CanvasRenderingContext2D layer, this new work, and at the GraphicsContext layer, DisplayList::Recorder.  Can they be consolidated in anyway?

This also adds a ton of complexity to the canvas code. If the goal is to trace the calls to the canvas operations, perhaps this could be generated from the IDL rather than added manually?
Comment 34 Devin Rousso 2017-07-21 23:01:25 PDT
(In reply to Sam Weinig from comment #33)
> Perhaps this has already been discussed, but it seems unfortunate for us to
> have graphics recording at two layers. At the CanvasRenderingContext2D
> layer, this new work, and at the GraphicsContext layer,
> DisplayList::Recorder.  Can they be consolidated in anyway?
I am unfamiliar with DisplayList::Recorder, but I think that it's too low level for our needs.  The idea for CanvasRenderingContext2D is that we want to be able to be able to replicate every call made to the context exactly as it was called in JavaScript, meaning that we can then replay it in a regular webpage.

> This also adds a ton of complexity to the canvas code. If the goal is to
> trace the calls to the canvas operations, perhaps this could be generated
> from the IDL rather than added manually?
That's a great idea, but it would probably be better suited as a followup.  I am more concerned with getting the initial feature working and available for people to use, but I do think that would make things a lot easier.
Comment 35 Devin Rousso 2017-07-22 01:11:00 PDT
Created attachment 316178 [details]
[Patch] WIP
Comment 36 Build Bot 2017-07-22 02:16:48 PDT
Comment on attachment 316178 [details]
[Patch] WIP

Attachment 316178 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4166458

Number of test failures exceeded the failure limit.
Comment 37 Build Bot 2017-07-22 02:16:50 PDT
Created attachment 316184 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 38 Build Bot 2017-07-22 02:30:49 PDT
Comment on attachment 316178 [details]
[Patch] WIP

Attachment 316178 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4166462

Number of test failures exceeded the failure limit.
Comment 39 Build Bot 2017-07-22 02:30:51 PDT
Created attachment 316189 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 40 Build Bot 2017-07-22 02:31:41 PDT
Comment on attachment 316178 [details]
[Patch] WIP

Attachment 316178 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4166476

Number of test failures exceeded the failure limit.
Comment 41 Build Bot 2017-07-22 02:31:42 PDT
Created attachment 316191 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 42 Build Bot 2017-07-22 02:55:25 PDT
Comment on attachment 316178 [details]
[Patch] WIP

Attachment 316178 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4166603

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Comment 43 Build Bot 2017-07-22 02:55:26 PDT
Created attachment 316192 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 44 Sam Weinig 2017-07-22 13:57:20 PDT
(In reply to Devin Rousso from comment #34)
> (In reply to Sam Weinig from comment #33)
> > Perhaps this has already been discussed, but it seems unfortunate for us to
> > have graphics recording at two layers. At the CanvasRenderingContext2D
> > layer, this new work, and at the GraphicsContext layer,
> > DisplayList::Recorder.  Can they be consolidated in anyway?
> I am unfamiliar with DisplayList::Recorder, but I think that it's too low
> level for our needs.  The idea for CanvasRenderingContext2D is that we want
> to be able to be able to replicate every call made to the context exactly as
> it was called in JavaScript, meaning that we can then replay it in a regular
> webpage.

I think having a broader discussion before doing something so close to what already exists still makes sense.

> > This also adds a ton of complexity to the canvas code. If the goal is to
> > trace the calls to the canvas operations, perhaps this could be generated
> > from the IDL rather than added manually?
> That's a great idea, but it would probably be better suited as a followup. 
> I am more concerned with getting the initial feature working and available
> for people to use, but I do think that would make things a lot easier.

I don't think that it is the kind of thing that makes sense as a follow up. This feature is quite a bit of new manually written code that places a burden on any additional feature work on canvas. Before add the feature, we should be sure the design scales well, and I think that probably means generating it.
Comment 45 Devin Rousso 2017-07-23 16:07:11 PDT
Created attachment 316242 [details]
[Patch] Attempt Modifying IDL
Comment 46 Build Bot 2017-07-23 16:10:12 PDT
Attachment 316242 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorInstrumentation.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 47 Devin Rousso 2017-07-23 20:09:19 PDT
Created attachment 316254 [details]
[Patch] Attempt Modifying IDL
Comment 48 Devin Rousso 2017-07-23 22:47:47 PDT
Created attachment 316267 [details]
[Patch] Attempt Modifying IDL
Comment 49 Devin Rousso 2017-07-23 23:38:11 PDT
Created attachment 316269 [details]
[Patch] Attempt Modifying IDL
Comment 50 Devin Rousso 2017-07-24 00:05:13 PDT
Created attachment 316270 [details]
[Patch] Attempt Modifying IDL
Comment 51 Brian Burg 2017-07-24 10:36:49 PDT
Comment on attachment 316270 [details]
[Patch] Attempt Modifying IDL

View in context: https://bugs.webkit.org/attachment.cgi?id=316270&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:40
> +    InspectorRecording=recordCanvasAction,

Maybe we could call this "CallTracingCallback" or something like that, and pass in a generic static method that bounces to InspectorInstrumentation. We'd probably want different methods for different types of recordings.
Comment 52 Brian Burg 2017-07-24 10:54:40 PDT
Comment on attachment 316270 [details]
[Patch] Attempt Modifying IDL

View in context: https://bugs.webkit.org/attachment.cgi?id=316270&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4679
> +            push(@$outputArray, "    if (UNLIKELY(impl.inspectorRecordingActive()))\n");

Nit: doesn't need to be named for Inspector. Just callTracingActive() would be cool, IMO. I want to do this for various DOM APIs eventually.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4889
> +            AddToImplIncludes("InspectorInstrumentation.h");

I'd like to avoid pulling in this header, as any changes to instrumentation header would then cause rebuilding of dependents. This might be not so bad in theory, but in practice it's hard to change/rebuild just one IDL file due to build system bugs.

If we are serious about making this generic (Sam, WDYT?), you can call some generic method on a singleton like CallTracer::traceFunctionCall(traceGroup, impl, $name, value). traceGroup could be what is passed as argument to the IDL extended attribute. Then you can add InspectorInstrumentation hooks inside of that. This also avoids pulling in InspectorInstrumentation to all generated files.

> Source/WebCore/bindings/scripts/IDLAttributes.json:215
> +        },

You'll need to add test cases for the code generator changes so that I can review what the generated code looks like when the attribute is set. You'll need a test case for each of the places the attribute is allowed in IDL: interface, attribute, and operation.
Comment 53 Brian Burg 2017-07-24 10:55:52 PDT
(In reply to Sam Weinig from comment #44)
> (In reply to Devin Rousso from comment #34)
> > (In reply to Sam Weinig from comment #33)
> > > Perhaps this has already been discussed, but it seems unfortunate for us to
> > > have graphics recording at two layers. At the CanvasRenderingContext2D
> > > layer, this new work, and at the GraphicsContext layer,
> > > DisplayList::Recorder.  Can they be consolidated in anyway?
> > I am unfamiliar with DisplayList::Recorder, but I think that it's too low
> > level for our needs.  The idea for CanvasRenderingContext2D is that we want
> > to be able to be able to replicate every call made to the context exactly as
> > it was called in JavaScript, meaning that we can then replay it in a regular
> > webpage.
> 
> I think having a broader discussion before doing something so close to what
> already exists still makes sense.

There's no way to go from a DisplayList ItemType back to the original canvas2d API calls, short of saving everything about the JS API call (which is what the current patch does!). Additionally, we want to preserve calls that are invalid / don't do anything; a display list may (if it's smart) prune such things. I don't understand how a dev-facing feature could be based on this approach.

> > > This also adds a ton of complexity to the canvas code. If the goal is to
> > > trace the calls to the canvas operations, perhaps this could be generated
> > > from the IDL rather than added manually?
> > That's a great idea, but it would probably be better suited as a followup. 
> > I am more concerned with getting the initial feature working and available
> > for people to use, but I do think that would make things a lot easier.
> 
> I don't think that it is the kind of thing that makes sense as a follow up.
> This feature is quite a bit of new manually written code that places a
> burden on any additional feature work on canvas. Before add the feature, we
> should be sure the design scales well, and I think that probably means
> generating it.

I tend to agree with Sam, though the bindings generator is no easy thing to hack on. If we make this generic enough, then inspector-specific changes are limited to adding the extended attribute to more things, and adding instrumentation hooks in the generic hook thing rather than in generated code directly.
Comment 54 Devin Rousso 2017-07-24 13:35:46 PDT
Created attachment 316315 [details]
[Patch] Modifying IDL
Comment 55 Joseph Pecoraro 2017-07-24 14:03:23 PDT
Comment on attachment 316315 [details]
[Patch] Modifying IDL

View in context: https://bugs.webkit.org/attachment.cgi?id=316315&action=review

> Source/WebCore/bindings/scripts/test/TestCallTracer.idl:35
> +[
> +    CallTracingCallback=testCallTracerInterface,
> +] interface TestCallTracer {
> +    [CallTracingCallback=testCallTracerAttribute] attribute boolean attributeTestCallTracer;
> +
> +    [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerNoArguments();
> +    [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithArguments(Node nodeArg);
> +    [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithNullableArgument(Node? nodeNullableArg);
> +    [CallTracingCallback=testCallTracerOperation] void operationTestCallTracerWithVariadicArgument(Node... nodeVariadicArg);
> +};

The IDLAttributes.json describes CallTrackingCallback as being supported on [interface, attribute, operation]. This doesn't show that putting it on the interface does it by default for ALL attributes/operations.

So it works in all cases:

> Interface attribute handles all getters/setters/operations:
> 
>     [
>         CallTracingCallback=testCallTracerInterface,
>     ] interface TestCallTracer {
>         attribute boolean attr;
>         void test(Node node);
>     };
> 
> Per-attribute / per-operation
> 
>     interface TestCallTracer {
>         attribute boolean unaffected;
>         void unaffectedTest(Node node);
>
>         [CallTracingCallback=testCallTracerAttribute] attribute boolean attr;
>         [CallTracingCallback=testCallTracerOperation] void test(Node node);
>     };
> 
> Mixed:
> 
>     [
>         CallTracingCallback=testCallTracerInterface,
>     ] interface TestCallTracer {
>         attribute boolean defaultAttr;
>         [CallTracingCallback=testCallTracerAttribute] attribute boolean specificAttr;
>         
>         void defaultTest(Node node);
>         [CallTracingCallback=testCallTracerOperation] void specificTest(Node node);
>     };

I think you can probably just turn this into a Mixed test and include some attributes / operations that will get the Interface's call tracer callback.
Comment 56 Brian Burg 2017-09-01 13:14:00 PDT
Obsoleting patches, this work landed as part of https://bugs.webkit.org/show_bug.cgi?id=174482. This is just a tracking bug at this point.