Bug 30707 – WebInspector: Refactors InspectorTimelineAgent to eliminate TimelineItem classes

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

[Eric Ayers]

This patch is in response to comments in issues 30578, 30467 that prescribe removing the Timeline item classes in favor of creating a ScriptObject directly.

[Eric Ayers]

Created attachment 41715 [details]
WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes

[Pavel Feldman]

Comment on attachment 41715 [details]
WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes

I see how lack of getters hurts you, sorry about that - we should probably implement those. It would be great if you made the stack explicit other than encapuslating it into your new class. It would look really short then, no heap allocations, brought back assert. What do I miss, why the additional complexity?

struct TimelineEventEntry {
    public TimelineEvent(object, children, type) : object(object), children(children), type(type) {}
    ScriptObject object;
    ScriptArray children;
    int type;
}

Vector< TimelineEventEntry > m_eventStack;


InspectorTimelineAgent::willRecalculateStyle()
{
    pushTimelineItem(TimelineItemFactory::createGenericTimelineItem(m_frontend, currentTimeInMilliseconds()), RecalculateStylesTimelineItemType);
}

InspectorTimelineAgent::didRecalculateStyle()
{
    popTimelineItem(RecalculateStylesTimelineItemType);
}

InspectorTimelineAgent::pushTimelineItem(ScriptObject event, int type)
{
    ScriptArray children = m_frontend->newScriptArray();
    event.set("type", type);
    event.set("children", children);
    m_eventStack.append(TimelineEventEntry(event, children, type));
}

InspectorTimelineAgent::popTimelineItem(int type)
{
    TimelineEventEntry event = m_eventStack.last();
    m_eventStack.removeLast();
    ASSERT(event.type == type);
    event.object.set("endTime", currentTimeInMilliseconds());

    if (m_eventStack.size()) {
        TimelineEventEntry parent = m_eventStack.last();
        parent.children.set(parent.children.length(), event.object);
    } else {
        m_frontend->addItemToTimeline(event.object);
    }
}

[Pavel Feldman]

Your ChangeLog should have url of this bug. This is a requirement (see other entries).

[Kelly Norton]

A Vector will work just fine, here's the reason I used a singly linked list:

Without getters on ScriptObject's there is no getting around having a C++ wrapper object because you need a place to keep the m_children ScriptArray. Since you have to create a C++ object anyway, it would be better to have constant-time push/pop of a linked list rather than the aggregated constant-time of a sensibly growing Vector (I haven't even looked to see if WTF::Vector uses a growth strategy that will give us aggregated constant time).

So, a Vector will work just fine here but it's actually too complex a data structure for our needs and adds some unfortunate runtime complexity. But I think it's a pretty minor detail overall, so I'll defer to your judgement.


(In reply to comment #2)
> (From update of attachment 41715 [details])
> I see how lack of getters hurts you, sorry about that - we should probably
> implement those. It would be great if you made the stack explicit other than
> encapuslating it into your new class. It would look really short then, no heap
> allocations, brought back assert. What do I miss, why the additional
> complexity?
> 
> struct TimelineEventEntry {
>     public TimelineEvent(object, children, type) : object(object),
> children(children), type(type) {}
>     ScriptObject object;
>     ScriptArray children;
>     int type;
> }
> 
> Vector< TimelineEventEntry > m_eventStack;
> 
> 
> InspectorTimelineAgent::willRecalculateStyle()
> {
>     pushTimelineItem(TimelineItemFactory::createGenericTimelineItem(m_frontend,
> currentTimeInMilliseconds()), RecalculateStylesTimelineItemType);
> }
> 
> InspectorTimelineAgent::didRecalculateStyle()
> {
>     popTimelineItem(RecalculateStylesTimelineItemType);
> }
> 
> InspectorTimelineAgent::pushTimelineItem(ScriptObject event, int type)
> {
>     ScriptArray children = m_frontend->newScriptArray();
>     event.set("type", type);
>     event.set("children", children);
>     m_eventStack.append(TimelineEventEntry(event, children, type));
> }
> 
> InspectorTimelineAgent::popTimelineItem(int type)
> {
>     TimelineEventEntry event = m_eventStack.last();
>     m_eventStack.removeLast();
>     ASSERT(event.type == type);
>     event.object.set("endTime", currentTimeInMilliseconds());
> 
>     if (m_eventStack.size()) {
>         TimelineEventEntry parent = m_eventStack.last();
>         parent.children.set(parent.children.length(), event.object);
>     } else {
>         m_frontend->addItemToTimeline(event.object);
>     }
> }

[Kelly Norton]

Created attachment 41761 [details]
Addresses issues raised by pfeldman.

[Pavel Feldman]

Comment on attachment 41761 [details]
Addresses issues raised by pfeldman.

Couple of  nits that can be addressed in further patches.

+     TimelineItemEntry entry = m_itemStack.last();

ASSERT(m_itemStack.size()); ?

+     if (m_itemStack.isEmpty()) {


no { here

[WebKit Commit Bot]

Comment on attachment 41761 [details]
Addresses issues raised by pfeldman.

Rejecting patch 41761 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Pavel Feldman', '--force']" exit_code: 1
Last 500 characters of output:
ng file WebCore/inspector/DOMDispatchTimelineItem.h
rm 'WebCore/inspector/DOMDispatchTimelineItem.h'
patching file WebCore/inspector/InspectorTimelineAgent.cpp
patching file WebCore/inspector/InspectorTimelineAgent.h
patching file WebCore/inspector/TimelineItem.cpp
rm 'WebCore/inspector/TimelineItem.cpp'
patching file WebCore/inspector/TimelineItem.h
rm 'WebCore/inspector/TimelineItem.h'
patching file WebCore/inspector/TimelineItemFactory.cpp
patching file WebCore/inspector/TimelineItemFactory.h

[Eric Seidel (no email)]

Hum... the last 500 characters was not enough to show the failure here.  In short, the patch failed to apply, probably just out of date.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/inspector/DOMDispatchTimelineItem.cpp
	M	WebCore/inspector/DOMDispatchTimelineItem.h
	M	WebCore/inspector/InspectorTimelineAgent.cpp
	M	WebCore/inspector/InspectorTimelineAgent.h
	M	WebCore/inspector/TimelineItem.cpp
	M	WebCore/inspector/TimelineItem.h
	A	WebCore/inspector/TimelineItemFactory.cpp
	A	WebCore/inspector/TimelineItemFactory.h
Committed r50013

[Eric Seidel (no email)]

Looks like the commit-queue's rejection was real.

http://trac.webkit.org/changeset/50150
http://trac.webkit.org/changeset/50152

Were clean-up commits to remove the files which didn't get removed during pavel's manual landing.  I'm not yet sure what was "wrong" with this patch yet such that svn-apply failed on the commit-queue (or what the failure was for Pavel).