Bug 30328 – Web Inspector: Migrate profiles to the injected script-based schema.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Mikhail Naganov]

Continuing Pavel's efforts on migrating Inspector interaction, I've migrated CPU profiles retrieval to use injected script-based schema.

What had been changed:
 - profile trees are encoded into JS objects, without 'parent' and 'head' references, for further JSONification (see InjectedScript.serializeProfile);
 - on the Inspector's side, 'parent' and 'head' properties are restored (see ProfileView._assignParentsInProfile);
 - when populating existing profiles on Inspector re-opening, they are loaded lazily; initially, only profile titles are loaded (see ProfilesPanel._populateProfiles), a full profile is retrieved when opening the view (see ProfileView constructor).

I also discovered that C++ side of profile objects and their C++/JS bridges contain unused methods and props, I will remove in the next change.

[Mikhail Naganov]

Created attachment 41095 [details]
proposed change

[Pavel Feldman]

Comment on attachment 41095 [details]
proposed change

We now are spending twice as much memory for the profile during serialization. Do you think we could tweak native object a bit so that it we don't need additional wrapping?

> +        ScriptFunctionCall function(m_scriptState, m_injectedScriptObj, "serializeProfile");
> +        function.appendArgument(toJS(m_scriptState, it->second.get()));
> +        ScriptValue serializedProfile = function.call();
> +        m_frontend->didGetProfile(callId, serializedProfile);

Could you extract wrapping into a separate method? (I see two usages).

Otherwise looks good.

[Mikhail Naganov]

Created attachment 41153 [details]
proposed change, comments addressed

OK, I removed "parent", "head", and other unneeded properties from the wrapper object, so additional serialization is no more needed. This also addresses the second comment, as there is no more call to a function from InjectedScript.

[Pavel Feldman]

Comment on attachment 41153 [details]
proposed change, comments addressed

> +    void getProfileHeaders(long callId);
> +    void getProfile(long callId, unsigned uid);

These should be private in InspectorController.

> +        function profileCallback(profile) {
> +            self.profile = profile;
> +            continueInitialization();
> +        }

{ on next line

> +        if (!this.profile._isStub)

Why do you end up with profiles in different states? I think you should resolve profile only here and only once.

Otherwise r+.

[Mikhail Naganov]

Created attachment 41155 [details]
proposed change, comments addressed

OK, now when a profile is added, only header is added actually. Thus, profiles are always loaded on demand when the view is about to be shown.

[WebKit Commit Bot]

Comment on attachment 41155 [details]
proposed change, comments addressed

Rejecting patch 41155 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49558

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49560

[Pavel Feldman]

Guess what - profiler tests rely upon the API you have changed...

fast/profiler/anonymous-event-handler.html -> failed
.
fast/profiler/anonymous-function-called-from-different-contexts.html -> failed
.
fast/profiler/anonymous-function-calls-built-in-functions.html -> failed
.
fast/profiler/anonymous-function-calls-eval.html -> failed
.
fast/profiler/anonymous-functions-with-display-names.html -> failed
.
fast/profiler/apply.html -> failed
.
fast/profiler/built-in-function-calls-anonymous.html -> failed
.
fast/profiler/built-in-function-calls-user-defined-function.html -> failed
.
fast/profiler/call-nodelist-as-function.html -> failed
..
fast/profiler/call.html -> failed
.
fast/profiler/calling-the-function-that-started-the-profiler-from-another-scope.html -> failed
.
fast/profiler/compare-multiple-profiles.html -> failed
.
fast/profiler/constructor.html -> failed
.
fast/profiler/dead-time.html -> failed
.
fast/profiler/document-dot-write.html -> failed
.
fast/profiler/event-handler.html -> failed
.
fast/profiler/execution-context-and-eval-on-same-line.html -> failed
.
fast/profiler/inline-event-handler.html -> failed
.
fast/profiler/many-calls-in-the-same-scope.html -> failed
.
fast/profiler/multiple-and-different-scoped-anonymous-function-calls.html -> failed
.
fast/profiler/multiple-and-different-scoped-function-calls.html -> failed
.
fast/profiler/multiple-anonymous-functions-called-from-the-same-function.html -> failed
.
fast/profiler/multiple-frames.html -> failed
.
fast/profiler/named-functions-with-display-names.html -> failed
.
fast/profiler/nested-anonymous-functon.html -> failed
.
fast/profiler/nested-start-and-stop-profiler.html -> failed
..
fast/profiler/one-execution-context.html -> failed
.
fast/profiler/profile-calls-in-included-file.html -> failed
.
fast/profiler/profile-with-no-title.html -> failed
.
fast/profiler/profiling-from-a-nested-location-but-stop-profiling-outside-the-nesting.html -> failed
.
fast/profiler/profiling-from-a-nested-location.html -> failed
.
fast/profiler/simple-event-call.html -> failed
.
fast/profiler/simple-no-level-change.html -> failed
.
fast/profiler/start-and-stop-profiler-multiple-times.html -> failed
.
fast/profiler/start-and-stop-profiling-in-the-same-function.html -> failed
..
fast/profiler/stop-profiling-after-setTimeout.html -> failed
.
fast/profiler/stop-then-function-call.html -> failed
.
fast/profiler/throw-exception-from-eval.html -> failed
.
fast/profiler/two-execution-contexts.html -> failed
.
fast/profiler/user-defined-function-calls-built-in-functions.html -> failed
.
fast/profiler/window-dot-eval.html -> failed

[Darin Adler]

Maybe llround is not available on Windows?

[Darin Adler]

Sorry, comment added to the wrong bug.

[Mikhail Naganov]

Created attachment 41189 [details]
proposed patch - now profiler tests pass

I'm really sorry for putting you into a trouble, Pavel.

It appears that I was too aggressive in removing properties from the profile node wrapper---"visible" property is what profiler tests were missing.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49615