Summary: | Web Inspector: Migrate profiles to the injected script-based schema. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Naganov <mnaganov> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, pfeldman, timothy | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Mikhail Naganov
2009-10-13 01:51:56 PDT
Created attachment 41095 [details]
proposed change
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. 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.
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+. 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.
Comment on attachment 41155 [details] proposed change, comments addressed 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. 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 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 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 Maybe llround is not available on Windows? Sorry, comment added to the wrong bug. 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.
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 |