| Summary: | Web Inspector: extract Inspector Instrumentation API as a class | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Podivilov <podivilov> | ||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, tkent, yurys | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Pavel Podivilov
2010-10-05 06:42:53 PDT
Created attachment 69777 [details]
Patch.
Created attachment 69783 [details]
Patch.
Comments addressed.
(In reply to comment #2) > Created an attachment (id=69783) [details] > Patch. > > Comments addressed. Wrong patch, sorry. Comment on attachment 69777 [details]
Patch.
looks good to me
Comment on attachment 69777 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=69777&action=review > WebCore/dom/Node.cpp:2605 > + int instrumentationCookie = InspectorInstrumentation::willDispatchEvent(document(), *event, targetForWindowEvents, this, ancestors); Nit: InspectorInstrumentationCookie cookie? > WebCore/inspector/InspectorInstrumentation.cpp:313 > + if (InspectorTimelineAgent* timelineAgent = inspectorController->m_timelineAgent.get()) { Nit: I think it'd be better to have overloaded function for retrieving timelineAgent. like retrieveTimelineAgent(InspectorController*) Created attachment 69953 [details]
Comments addressed.
Comment on attachment 69953 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=69953&action=review > WebCore/ChangeLog:27 > + (WebCore::InspectorInstrumentation::willInsertDOMNodeImpl): You should nuke these scary verbose lines. > WebCore/bindings/v8/V8Proxy.cpp:414 > + InspectorInstrumentation::didEvaluateScript(m_frame, cookie); I think cookie should be the first parameter in all the calls. > WebCore/inspector/InspectorInstrumentation.cpp:-67 > - if (!inspectorController->hasFrontend()) Why did these go? Please explain in order to get r+. > WebCore/inspector/InspectorInstrumentation.h:116 > + static InspectorInstrumentationCookie willCallFunctionImpl(InspectorController*, const String& scriptName, int scriptLine); This is crazy amount of methods, really easy to make a mistake here. Can we cut the number? >
> > WebCore/inspector/InspectorInstrumentation.cpp:-67
> > - if (!inspectorController->hasFrontend())
>
> Why did these go? Please explain in order to get r+.
This checks were moved to InspectorInstrumentation::inspectorControllerForPage method.
Comment on attachment 69953 [details] Comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=69953&action=review > WebCore/inspector/InspectorInstrumentation.h:61 > + static void willSendXMLHttpRequest(ScriptExecutionContext*, const String& url); LGTM given we migrate to a macro shortly. Committed r69283: <http://trac.webkit.org/changeset/69283> This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms. (In reply to comment #11) > This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms. Oops, copy&paste error. This change broke http/tests/inspector/console-xhr-logging.html on all platforms. (In reply to comment #12) > (In reply to comment #11) > > This change broke https://bugs.webkit.org/show_bug.cgi?id=47173 on all platforms. > > Oops, copy&paste error. > > This change broke http/tests/inspector/console-xhr-logging.html on all platforms. Fixed in r69286. |