Bug 30328 - Web Inspector: Migrate profiles to the injected script-based schema.
: Web Inspector: Migrate profiles to the injected script-based schema.
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-10-13 01:51 PST by
Modified: 2009-10-15 02:32 PST (History)


Attachments
proposed change (17.49 KB, patch)
2009-10-13 01:59 PST, Mikhail Naganov
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
proposed change, comments addressed (19.27 KB, patch)
2009-10-14 01:25 PST, Mikhail Naganov
pfeldman: review-
Review Patch | Details | Formatted Diff | Diff
proposed change, comments addressed (20.49 KB, patch)
2009-10-14 03:07 PST, Mikhail Naganov
commit-queue: review-
Review Patch | Details | Formatted Diff | Diff
proposed patch - now profiler tests pass (20.07 KB, patch)
2009-10-14 14:20 PST, Mikhail Naganov
pfeldman: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-13 01:51:56 PST
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.
------- Comment #1 From 2009-10-13 01:59:42 PST -------
Created an attachment (id=41095) [details]
proposed change
------- Comment #2 From 2009-10-13 12:58:04 PST -------
(From update of attachment 41095 [details])
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.
------- Comment #3 From 2009-10-14 01:25:51 PST -------
Created an attachment (id=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.
------- Comment #4 From 2009-10-14 01:41:58 PST -------
(From update of attachment 41153 [details])
> +    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+.
------- Comment #5 From 2009-10-14 03:07:36 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-10-14 06:30:21 PST -------
(From update of attachment 41155 [details])
Rejecting patch 41155 from review queue.

pfeldman@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
------- Comment #7 From 2009-10-14 06:32:55 PST -------
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
------- Comment #8 From 2009-10-14 07:55:20 PST -------
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
------- Comment #9 From 2009-10-14 07:58:15 PST -------
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
------- Comment #10 From 2009-10-14 08:21:43 PST -------
Maybe llround is not available on Windows?
------- Comment #11 From 2009-10-14 08:23:46 PST -------
Sorry, comment added to the wrong bug.
------- Comment #12 From 2009-10-14 14:20:38 PST -------
Created an attachment (id=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.
------- Comment #13 From 2009-10-15 02:32:26 PST -------
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