Bug 30707

Summary: WebInspector: Refactors InspectorTimelineAgent to eliminate TimelineItem classes
Product: WebKit Reporter: Eric Ayers <zundel>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, knorton, pfeldman, timothy, zundel
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes
pfeldman: review-
Addresses issues raised by pfeldman. pfeldman: review+, commit-queue: commit-queue-

Eric Ayers
Reported 2009-10-22 23:55:25 PDT
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.
Attachments
WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes (38.20 KB, patch)
2009-10-22 23:57 PDT, Eric Ayers
pfeldman: review-
Addresses issues raised by pfeldman. (38.69 KB, patch)
2009-10-23 16:12 PDT, Kelly Norton
pfeldman: review+
commit-queue: commit-queue-
Eric Ayers
Comment 1 2009-10-22 23:57:24 PDT
Created attachment 41715 [details] WebInspector: Refactors InspectorTimelineAgent to remove TimelineItem classes
Pavel Feldman
Comment 2 2009-10-23 07:37:54 PDT
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
Comment 3 2009-10-23 07:45:50 PDT
Your ChangeLog should have url of this bug. This is a requirement (see other entries).
Kelly Norton
Comment 4 2009-10-23 10:10:44 PDT
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
Comment 5 2009-10-23 16:12:03 PDT
Created attachment 41761 [details] Addresses issues raised by pfeldman.
Pavel Feldman
Comment 6 2009-10-23 16:19:45 PDT
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 7 2009-10-23 16:21:45 PDT
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)
Comment 8 2009-10-23 16:40:37 PDT
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
Comment 9 2009-10-23 17:25:08 PDT
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)
Comment 10 2009-10-27 13:56:06 PDT
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).
Note You need to log in before you can comment on or make changes to this bug.