[webkit-reviews] review denied: [Bug 30135] Make InspectorTimelineAgent consistent with other components in Inspector. : [Attachment 41117] Removes TimelineAgent setting.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Kelly Norton
<knorton at google.com>'s request for review:
Bug 30135: Make InspectorTimelineAgent consistent with other components in
Inspector.
https://bugs.webkit.org/show_bug.cgi?id=30135

Attachment 41117: Removes TimelineAgent setting.
https://bugs.webkit.org/attachment.cgi?id=41117&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> -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.