Bug 52869

Summary: Web Inspector: switch page/Console implementation from InspectorController to InspectorInstrumentation
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, buildbot, bweinstein, eric, gustavo, joepeck, keishi, loislo, mnaganov, pfeldman, pmuellr, rik, timothy, webkit-ews, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 52875    
Bug Blocks:    
Attachments:
Description Flags
[patch] initial version
none
[patch] second iteration
yurys: review-
[patch] third iteration none

Description Ilya Tikhonovsky 2011-01-20 22:53:10 PST
%subj%
Comment 1 Ilya Tikhonovsky 2011-01-20 23:09:52 PST
Created attachment 79700 [details]
[patch] initial version
Comment 2 WebKit Review Bot 2011-01-20 23:16:54 PST
Attachment 79700 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7604260
Comment 3 Early Warning System Bot 2011-01-20 23:23:22 PST
Attachment 79700 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7522228
Comment 4 Ilya Tikhonovsky 2011-01-20 23:39:50 PST
Created attachment 79701 [details]
[patch] second iteration
Comment 5 Build Bot 2011-01-20 23:40:06 PST
Attachment 79700 [details] did not build on win:
Build output: http://queues.webkit.org/results/7602242
Comment 6 WebKit Review Bot 2011-01-21 00:09:37 PST
Attachment 79700 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7539257
Comment 7 Yury Semikhatsky 2011-01-21 00:20:25 PST
Comment on attachment 79701 [details]
[patch] second iteration

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

> Source/WebCore/inspector/InspectorInstrumentation.cpp:571
> +    return inspectorController->profilerAgent()->getCurrentUserInitiatedProfileName(incrementProfileNumber);

ProfilerAgent may be 0 here, r- for this.

> 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.

> 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.
Comment 8 Ilya Tikhonovsky 2011-01-21 01:06:45 PST
Created attachment 79707 [details]
[patch] third iteration

(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(incrementProfileNumber);
> 
> 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.
Comment 9 Ilya Tikhonovsky 2011-01-21 01:24:59 PST
Comment on attachment 79707 [details]
[patch] third iteration

Clearing flags on attachment: 79707

Committed r76335: <http://trac.webkit.org/changeset/76335>
Comment 10 Ilya Tikhonovsky 2011-01-21 01:25:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2011-01-21 01:58:13 PST
http://trac.webkit.org/changeset/76335 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/profiler/anonymous-event-handler.html
fast/profiler/anonymous-function-called-from-different-contexts.html
fast/profiler/anonymous-function-calls-built-in-functions.html
fast/profiler/anonymous-function-calls-eval.html
fast/profiler/anonymous-functions-with-display-names.html
fast/profiler/apply.html
fast/profiler/built-in-function-calls-anonymous.html
fast/profiler/built-in-function-calls-user-defined-function.html
fast/profiler/call-nodelist-as-function.html
fast/profiler/call.html
fast/profiler/calling-the-function-that-started-the-profiler-from-another-scope.html
fast/profiler/compare-multiple-profiles.html
fast/profiler/constructor.html
fast/profiler/dead-time.html
fast/profiler/document-dot-write.html
fast/profiler/event-handler.html
fast/profiler/execution-context-and-eval-on-same-line.html
fast/profiler/inline-event-handler.html
fast/profiler/many-calls-in-the-same-scope.html
fast/profiler/multiple-and-different-scoped-anonymous-function-calls.html
fast/profiler/multiple-and-different-scoped-function-calls.html
fast/profiler/multiple-anonymous-functions-called-from-the-same-function.html
fast/profiler/multiple-frames.html
fast/profiler/named-functions-with-display-names.html
fast/profiler/nested-anonymous-functon.html
fast/profiler/nested-start-and-stop-profiler.html
fast/profiler/one-execution-context.html
fast/profiler/profile-calls-in-included-file.html
fast/profiler/profile-with-no-title.html
fast/profiler/profiling-from-a-nested-location-but-stop-profiling-outside-the-nesting.html
fast/profiler/profiling-from-a-nested-location.html
fast/profiler/simple-event-call.html
fast/profiler/simple-no-level-change.html
fast/profiler/start-and-stop-profiler-multiple-times.html
fast/profiler/start-and-stop-profiling-in-the-same-function.html
fast/profiler/stop-profiling-after-setTimeout.html
fast/profiler/stop-then-function-call.html
fast/profiler/two-execution-contexts.html
fast/profiler/user-defined-function-calls-built-in-functions.html
fast/profiler/window-dot-eval.html
Comment 12 Ilya Tikhonovsky 2011-01-22 00:20:58 PST
actually landed as 

Committed r76344
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/page/Console.cpp
	M	Source/WebCore/inspector/InspectorController.cpp
	M	Source/WebCore/inspector/InspectorController.h
	M	Source/WebCore/inspector/InspectorInstrumentation.cpp
	M	Source/WebCore/inspector/InspectorInstrumentation.h
r76344 = 043f7d3c197b58cfab39413ffbdec0c6cf84955b (refs/remotes/trunk)