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.
Created attachment 41715 [details] WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes
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); } }
Your ChangeLog should have url of this bug. This is a requirement (see other entries).
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); > } > }
Created attachment 41761 [details] Addresses issues raised by pfeldman.
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
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
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.
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
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).