| Summary: | Web Inspector: [Canvas] support instrumenting canvases in iframes (backend side) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andrey Adaikin <aandrey> | ||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Andrey Adaikin <aandrey> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | apavlov, buildbot, dglazkov, keishi, loislo, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 108064, 108319 | ||||||||
| Attachments: |
|
||||||||
|
Description
Andrey Adaikin
2013-01-25 07:36:51 PST
Created attachment 184750 [details]
Patch
Comment on attachment 184750 [details] Patch Attachment 184750 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16097828 New failing tests: inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases.html inspector/profiler/canvas2d/canvas-stack-trace.html inspector/profiler/webgl/webgl-profiler-get-error.html inspector/profiler/canvas2d/canvas2d-gradient-capturing.html Comment on attachment 184750 [details] Patch Attachment 184750 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16120399 New failing tests: inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html inspector/profiler/webgl/webgl-profiler-get-error.html inspector/profiler/canvas2d/canvas2d-gradient-capturing.html Comment on attachment 184750 [details] Patch Attachment 184750 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16122431 New failing tests: inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html inspector/profiler/webgl/webgl-profiler-get-error.html inspector/profiler/canvas2d/canvas2d-gradient-capturing.html Comment on attachment 184750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184750&action=review > Source/WebCore/inspector/Inspector.json:3382 > + "name": "frameWithCanvases", Consider renaming - event should tell us what exactly is happening > Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 > + if (m_framesWithUninstrumentedCanvases.contains(frame)) { What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:190 > + InjectedScriptCanvasModule module = injectedScriptCanvasModule(errorString, traceLogId); This nice refactoring seems to be a separate change. Comment on attachment 184750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184750&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 >> + if (m_framesWithUninstrumentedCanvases.contains(frame)) { > > What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup. I do track frameNavigated and frameDetached. Is this not enough? Comment on attachment 184750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184750&action=review >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 >>> + if (m_framesWithUninstrumentedCanvases.contains(frame)) { >> >> What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup. > > I do track frameNavigated and frameDetached. Is this not enough? Why is the comment on no defering then? (In reply to comment #7) > (From update of attachment 184750 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184750&action=review > > >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 > >>> + if (m_framesWithUninstrumentedCanvases.contains(frame)) { > >> > >> What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup. > > > > I do track frameNavigated and frameDetached. Is this not enough? > > Why is the comment on no defering then? I'm just being too cautious (maybe without a good reason here) not to use-after-free. I dont want to deref a pointer, since I dont know it's ownership policy. Is it safe to deref a Frame*, provided that I remove pointers on frameDetached? Created attachment 184972 [details]
Patch
Comment on attachment 184972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:284 > ScriptProfiler::visitNodeWrappers(&nodeVisitor); What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll? (In reply to comment #10) > (From update of attachment 184972 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review > > > Source/WebCore/inspector/InspectorCanvasAgent.cpp:284 > > ScriptProfiler::visitNodeWrappers(&nodeVisitor); > > What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll? I want those canvases as well as ones that are not attached to the DOM tree (yet). Comment on attachment 184972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284 >>> ScriptProfiler::visitNodeWrappers(&nodeVisitor); >> >> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll? > > I want those canvases as well as ones that are not attached to the DOM tree (yet). Why would you want that? (In reply to comment #12) > (From update of attachment 184972 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review > > >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284 > >>> ScriptProfiler::visitNodeWrappers(&nodeVisitor); > >> > >> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll? > > > > I want those canvases as well as ones that are not attached to the DOM tree (yet). > > Why would you want that? 1) To instrument canvases that are not in the DOM. It's quite common to create a temp canvas to render some part of the scene and then discard it. 2) To handle the case when we would enable CanvasAgent between a canvas context is created and it is appended to the DOM. Comment on attachment 184972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review >>>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284 >>>>> ScriptProfiler::visitNodeWrappers(&nodeVisitor); >>>> >>>> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll? >>> >>> I want those canvases as well as ones that are not attached to the DOM tree (yet). >> >> Why would you want that? > > 1) To instrument canvases that are not in the DOM. It's quite common to create a temp canvas to render some part of the scene and then discard it. > > 2) To handle the case when we would enable CanvasAgent between a canvas context is created and it is appended to the DOM. Iterating over bindings to achieve that sounds like an overkill. It is a shame we need to do that. Comment on attachment 184972 [details] Patch Clearing flags on attachment: 184972 Committed r141098: <http://trac.webkit.org/changeset/141098> All reviewed patches have been landed. Closing bug. |