Bug 30135 – Make InspectorTimelineAgent consistent with other components in Inspector.

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

[Kelly Norton]

== From pfeldman (https://bugs.webkit.org/show_bug.cgi?id=30028) ==

Given that you initialize m_timelineAgent in the setFrontendProxyObject, it
will only collect the data while inspector is
opened (has been opened for the page at least once in case of WebKit). Is it so
by design?

I am asking because Inspector uses a bit different models for existing types of
inspected objects:
1) It tracks resources iff tracking is enabled, regardless of the frontend
state. It then dumps all the data collected
into the frontend upon its availability. When user enables this tracking for
the first time, page reload takes place in
order to gather the data.
2) There are explicit "start/stop profiling" buttons that start/stop collecting
data and dump results into the profiler page.
We are currently extending profiling tab so that it could gather different
types of profiles.

I think you should choose one of the models above to look consistent within
WebKit. If you think some users can have
timeline agent turned on constantly (it produces minimal overhead, there are
times when user would like to see what
has been happening without reloading the page), you should go (1). Otherwise,
if you consider your agent to be of a
profiling nature, you should choose (2). (2) implies that timeline agent
results are presented as a profile result on the
profiles page.

We can chat about it and discuss the ways you will push the gathered
information further into your framework. I'll
be happy to help you out with the WebKit frontend piece as well.

[Kelly Norton]

Created attachment 41117 [details]
Removes TimelineAgent setting.

This removes the setting for the TimelineAgent since there is no immediate need for it and it's shaping up to look more like profiling than resource tracking.

Pavel - Should I add anything else to the patch to make it consistent with the other components in InspectorController?

[Pavel Feldman]

Comment on attachment 41117 [details]
Removes TimelineAgent setting.

> -void InspectorBackend::enableTimeline(bool always)
> +void InspectorBackend::enableTimeline()
>  {

If you are implementing profiler-alike operation, you should consider naming these accordingly. In the profiler world start/stopProfiling are the methods controlling profiler. enable/disableProfiler methods are only making sure JS engine is ready for the operation as a whole. I think you should name yours start/stopTimelineProfiler or similar. Note that you won't find start/stopProfiling methods in the inspector controller API since they are not used anywhere other than Web Inspector frontend. That's why they only present in InspectorBackend interface. There are start/stopUserInitiatedProfiling methods that inspector controller exposes to the clients instead. It is up to you whether to expose start/stopTimelineProfiler methods in InspectorController API or limit their visibility to the frontend.

[Kelly Norton]

Created attachment 41125 [details]
Updates the method names appropriately.

[Pavel Feldman]

Note that you should be adding bug id into the ChangeLog. Unfortunately I noticed it only after landing.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	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/front-end/TimelineAgent.js
Committed r49565

[Eric Seidel (no email)]

Comment on attachment 41125 [details]
Updates the method names appropriately.

Removing cq? from this closed bug.