Summary: | Web Inspector: Refactoring: extract async stack trace logic from InspectorInstrumentation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, inspector-bugzilla-changes, joepeck, kangil.han, keith_miller, mark.lam, msaboff, saam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 174742, 167084, 174739 | ||||||||||
Attachments: |
|
Description
Matt Baker
2017-07-21 18:24:36 PDT
Created attachment 316157 [details]
Patch
Comment on attachment 316157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316157&action=review r- unless everyone else disagrees on the enum vs int signature issue. I wasn't paying attention when this code originally landed. > Source/WebCore/inspector/InspectorAsynchronousCallType.h:28 > +namespace WebCore { Please, use namespace Inspector unless it's really not possible. > Source/WebCore/inspector/InspectorAsynchronousCallType.h:31 > + AnimationFrame = 1, Nit: RequestAnimationFrame > Source/WebCore/inspector/InspectorAsynchronousCallType.h:32 > + Timer, Nit: can we call this DOMTimer to be consistent with the relevant C++ class? > Source/WebCore/inspector/InspectorInstrumentation.cpp:349 > + debuggerAgent->didCancelAsyncCall(static_cast<int>(AsynchronousCallType::Timer), timerId); Nooo stop it :( EDIT: I see that we did this to avoid mentioning Web-specific concepts in JSC / Debugger.json. I have no such aversion. This code should be in Inspector namespace anyway. Passing around ints is worse than some optional enum values having no meaning in JavaScriptCore proper. > Source/WebCore/inspector/PageDebuggerAgent.cpp:160 > + JSC::ExecState* scriptState = document->execState(); The dereference of document needs a null check, or passed to here by reference. Comment on attachment 316157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316157&action=review > Source/WebCore/ChangeLog:3 > + Web Inspector: Refactoring: remove async stack trace logic from InspectorInstrumentation I'd prefer "extract" over "remove". Created attachment 316338 [details]
Patch
Comment on attachment 316338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316338&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + Move AsyncCallType enum to InspectoprDebuggerAgent, which manages async Nit: InspectorDebuggerAgent Created attachment 316389 [details]
Patch
Comment on attachment 316389 [details] Patch Clearing flags on attachment: 316389 Committed r219889: <http://trac.webkit.org/changeset/219889> All reviewed patches have been landed. Closing bug. |