[webkit-reviews] review requested: [Bug 52869] Web Inspector: switch page/Console implementation from InspectorController to InspectorInstrumentation : [Attachment 79707] [patch] third iteration

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 52869: Web Inspector: switch page/Console implementation from
InspectorController to InspectorInstrumentation
https://bugs.webkit.org/show_bug.cgi?id=52869

Attachment 79707: [patch] third iteration
https://bugs.webkit.org/attachment.cgi?id=79707&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(In reply to comment #7)
> (From update of attachment 79701 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=79701&action=review
> 
> > Source/WebCore/inspector/InspectorInstrumentation.cpp:571
> > +	 return
inspectorController->profilerAgent()->getCurrentUserInitiatedProfileName(increm
entProfileNumber);
> 
> ProfilerAgent may be 0 here, r- for this.

fixed.


> > Source/WebCore/inspector/InspectorInstrumentation.h:142
> > +	 static void addProfile(Page*, RefPtr<ScriptProfile>,
ScriptCallStack*);
> 
> ScriptCallStack should be passed as PassOwnPtr since the profiler agent may
want to store the callstack in a console message.

There are 4 such functions which receiving ScriptCallStack by pointer.
These functions use script name and line number of top frame at the moment. 
This can be implemented in another patch if you want.

> 
> > Source/WebCore/inspector/InspectorInstrumentation.h:902
> > +	 if (InspectorController* inspectorController =
inspectorControllerForPage(page))
> 
> There was no check for InspectorController, I don't think it can be 0 here.
It is a common practice in InspectorInstrumentation code.