Bug 14370 – Inspector's timeline should record when certain DOM events fired

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

[Adam Roben (:aroben)]

It would be nice to be able to see when certain DOM events fired during the load of the page. The logical place to put this is the Inspector's timeline view. For instance, it would be nice to see when the load event fires for each document in the page. This will be especially handy for deciphering why JS executed, once bug 14351 is resolved.

A possible visual representation for showing when these events occurred would be to have a tick mark on the resource's time bar. When you hover over the tick mark, it could expand to tell you what it represents.

[Adam Roben (:aroben)]

<rdar://problem/5712825>

[Brian Weinstein]

Created attachment 40466 [details]
Screenshot

[Brian Weinstein]

Created attachment 40467 [details]
Fix

[Timothy Hatcher]

Comment on attachment 40467 [details]
Fix


> +        InspectorController* controller = page()->inspectorController();
> +        if (controller) {
> +            if (controller->isMainResourceLoader(f->loader()->documentLoader(), url())) {
> +                controller->mainResourceFiredDOMContentEvent();
> +            }
> +        }

Better written as:

> +        InspectorController* controller = page()->inspectorController();
> +        if (controller && controller->isMainResourceLoader(f->loader()->documentLoader(), url()))
> +            controller->mainResourceFiredDOMContentEvent();

But I think it would be better to not expose isMainResourceLoader and just always call "firedDOMContentEvent", etc. And let InspectorController decide to keep it if isMainResourceLoader true. That way the call sites in Document.cpp and DOMWindow.cpp are as small as possible.

So:

> +        if (InspectorController* controller = page()->inspectorController())
> +            controller->firedDOMContentEvent(f->loader()->documentLoader());


> +    bool isMainResourceLoader(DocumentLoader* loader, const KURL& requestUrl);

See my previous comment about not exposing this a public.

> +            jsonObject.set("domContentTime", m_domContentEventTime);

This should be "domContentEventTime" to match "loadEventTime".

> +        
> +        
> +        if (this.calculator.startAtZero) {

Remove the extra line above the if.



> +        if (eventTime !== -1 && !this.startAtZero)
> +            var percent = ((eventTime - this.minimumBoundary) / this.boundarySpan) * 100;
> +        else
> +            var percent = 0;
> +    
> +        return percent;

Simplier as:

> +        if (eventTime !== -1 && !this.startAtZero)
> +            return ((eventTime - this.minimumBoundary) / this.boundarySpan) * 100;
> +        return 0;


> +        if (payload.loadEventTime) {
> +            // This loadEventTime is for the main resource, and we want to show it
> +            // for all resources on this page. This means we want to set it as a member
> +            // of the resource panel instead of the individual resource.
> +            if (this.panels.resources)
> +                this.panels.resources.mainResourceLoadTime = payload.loadEventTime;

> +        }
> +        if (payload.domContentTime) {

Add a line break between those lines.

> +            if (this.panels.resources)
> +                this.panels.resources.mainResourceDOMContentTime = payload.domContentTime;

Add a comment referencing the previous comment, so you know the reason this goes on ResourcesPanel.


> +    InspectorController* controller = frame()->page()->inspectorController();
> +    if (controller) {
> +        if (controller->isMainResourceLoader(frame()->loader()->documentLoader(), frame()->document()->url())) {
> +            controller->mainResourceFiredLoadEvent();
> +        }
> +    }

See my earlier comment about simplifying this call site.

[Brian Weinstein]

Created attachment 40540 [details]
Addresses Tim's Comments

[Brian Weinstein]

Committed in http://trac.webkit.org/changeset/49036, sorry for not closing this sooner.

[Pavel Feldman]

I think this should have been implemented using TimelineAgent. Adding Kelly

[Pavel Feldman]

I think this should have been implemented using TimelineAgent. Adding Kelly.

I would push these dom events along with the rest of timeline ones and would bind these events to the resource / timeline graph on the frontend side instead.

This however puts some constraints onto how TimelineAgent should be implemented. It seems that there are important events (such as the ones supported in this change) that we would like to get whenever inspector is enabled. Or to be more precise, whenever resources panel is available. In fact, resources panel is already perceived as a timeline in some sense.

Here is what I would do:
1) Make TimelineAgent exist at all times resource tracking is enabled (there is anyway bidirectional dependency between them as we can see).
2) Introduce TimelineAgent filter and make it configurable.
3) It also means that Timeline should be working in the frontend-less mode (i.e. collecting native objects for events and dumping them to ScriptObjects when the time comes).
4) Remove main resource / dom event correlation in the inspector controller and wire it in the frontend.

Kelly, for you that would mean that start/stopTimelineAgent would transform into setting up timeline agent filter. I wonder how much overhead this approach would introduce given the granularity of your events.

This also could affect the way we visualize things. It seems like one of the approaches would be to generalize resources panel and provide different perspectives on the timeline: resource-related, dom events-related, timer-related or overlaid. Tim, what do you think?

[Timothy Hatcher]

(In reply to comment #8)
> I think this should have been implemented using TimelineAgent. Adding Kelly.
> 
> I would push these dom events along with the rest of timeline ones and would
> bind these events to the resource / timeline graph on the frontend side
> instead.
> 
> This however puts some constraints onto how TimelineAgent should be
> implemented. It seems that there are important events (such as the ones
> supported in this change) that we would like to get whenever inspector is
> enabled. Or to be more precise, whenever resources panel is available. In fact,
> resources panel is already perceived as a timeline in some sense.
> 
> Here is what I would do:
> 1) Make TimelineAgent exist at all times resource tracking is enabled (there is
> anyway bidirectional dependency between them as we can see).
> 2) Introduce TimelineAgent filter and make it configurable.
> 3) It also means that Timeline should be working in the frontend-less mode
> (i.e. collecting native objects for events and dumping them to ScriptObjects
> when the time comes).
> 4) Remove main resource / dom event correlation in the inspector controller and
> wire it in the frontend.
> 
> Kelly, for you that would mean that start/stopTimelineAgent would transform
> into setting up timeline agent filter. I wonder how much overhead this approach
> would introduce given the granularity of your events.
> 
> This also could affect the way we visualize things. It seems like one of the
> approaches would be to generalize resources panel and provide different
> perspectives on the timeline: resource-related, dom events-related,
> timer-related or overlaid. Tim, what do you think?

I think building this feature off of TimelineAgent makes sense, as we talked about today. But I think the Resources panel timeline should stay simple and we should design a timeline UI into the Profiles panel, ala Instruments, also like we talked about today.

If I find some time, I will look at mocking something up. Or maybe you guys can take the ideas we talked about and see how it will fit with the data and the UI you areleady have in mind.

[Pavel Feldman]

(In reply to comment #9)
> (In reply to comment #8)
> > I think this should have been implemented using TimelineAgent. Adding Kelly.
> > 
> > I would push these dom events along with the rest of timeline ones and would
> > bind these events to the resource / timeline graph on the frontend side
> > instead.
> > 
> > This however puts some constraints onto how TimelineAgent should be
> > implemented. It seems that there are important events (such as the ones
> > supported in this change) that we would like to get whenever inspector is
> > enabled. Or to be more precise, whenever resources panel is available. In fact,
> > resources panel is already perceived as a timeline in some sense.
> > 
> > Here is what I would do:
> > 1) Make TimelineAgent exist at all times resource tracking is enabled (there is
> > anyway bidirectional dependency between them as we can see).
> > 2) Introduce TimelineAgent filter and make it configurable.
> > 3) It also means that Timeline should be working in the frontend-less mode
> > (i.e. collecting native objects for events and dumping them to ScriptObjects
> > when the time comes).
> > 4) Remove main resource / dom event correlation in the inspector controller and
> > wire it in the frontend.
> > 
> > Kelly, for you that would mean that start/stopTimelineAgent would transform
> > into setting up timeline agent filter. I wonder how much overhead this approach
> > would introduce given the granularity of your events.
> > 
> > This also could affect the way we visualize things. It seems like one of the
> > approaches would be to generalize resources panel and provide different
> > perspectives on the timeline: resource-related, dom events-related,
> > timer-related or overlaid. Tim, what do you think?
> 
> I think building this feature off of TimelineAgent makes sense, as we talked
> about today. But I think the Resources panel timeline should stay simple and we
> should design a timeline UI into the Profiles panel, ala Instruments, also like
> we talked about today.
> 
> If I find some time, I will look at mocking something up. Or maybe you guys can
> take the ideas we talked about and see how it will fit with the data and the UI
> you areleady have in mind.

See https://bugs.webkit.org/show_bug.cgi?id=30614