Some of the events that we want to display in Timeline panel are not convenient to plumb using present instrumentation interfaces. This adds support for handling trace events in Timeline panel and uses it to instrument deferred painting of layers.
Created attachment 180791 [details] Patch
Comment on attachment 180791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180791&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:122 > + static void didReportTraceEvent(Page*, char phase, const char* categoryName, const char* name, unsigned long long id, traceEvent ? > Source/WebCore/inspector/InspectorTimelineAgent.cpp:117 > + : m_size(numArguments), m_argumentNames(argumentNames), m_argumentTypes(argumentTypes), m_argumentValues(argumentValues) stack these please. who owns the argumentNames? > Source/WebCore/inspector/InspectorTimelineAgent.cpp:123 > + bool getBool(const char* name) const { return getParameter(name, TRACE_VALUE_TYPE_BOOL).m_bool; } Place block on its own lines. asBool(const char* name). > Source/WebCore/inspector/InspectorTimelineAgent.cpp:140 > + const TraceValueUnion& getParameter(const char* name, char expectedType) const parameter( > Source/WebCore/inspector/InspectorTimelineAgent.cpp:226 > + m_client->enableTraceEventsReporting(true); setReportTraceEvents > Source/WebCore/inspector/InspectorTimelineAgent.cpp:243 > + m_deferredPaintLayerId = 0; Lets split these. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:378 > + TraceEventHandler handler = m_traceEventHandlers.get(name); Lets make name+phase a key. > Source/WebCore/inspector/InspectorTimelineAgent.cpp:385 > +const char kLayerId[] = "layerId"; anonymous namespace for this one? > Source/WebCore/inspector/InspectorTimelineAgent.cpp:692 > + m_traceEventHandlers.set("Instrumentation::DeferPaint", &InspectorTimelineAgent::onDeferPaint); I would whitelist the ones we want to expose + handle specifics using switch. Or lets introduce generic fallback handler. Or rename to onTraceEventDeferPaint > Source/WebCore/inspector/InspectorTimelineAgent.cpp:725 > +void InspectorTimelineAgent::pushSkippedRecord(const String& type) mute? Can we also land this as a separate patch?
Created attachment 180876 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 180876 [details] Patch Attachment 180876 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15549833
Comment on attachment 180876 [details] Patch Attachment 180876 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15557772
Comment on attachment 180876 [details] Patch Attachment 180876 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15545874
Comment on attachment 180876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180876&action=review > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:200 > + ASSERT_NOT_REACHED(); That should be fine.
Comment on attachment 180876 [details] Patch Attachment 180876 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15549836
Created attachment 180881 [details] Patch Trying on bots..
Comment on attachment 180881 [details] Patch Attachment 180881 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15582050
Comment on attachment 180876 [details] Patch Attachment 180876 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15549854
Created attachment 180922 [details] Patch Another try on bots..
Created attachment 180932 [details] Patch Rebased, trying on bots again..
Created attachment 186000 [details] Patch
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16308579
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16305575
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16293592
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16301628
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16298575
Comment on attachment 186000 [details] Patch Attachment 186000 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16301636
Created attachment 186015 [details] Patch
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16301659
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16301660
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16297656
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16297661
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16297666
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16300602
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16301671
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16297675
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16305620
Comment on attachment 186015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186015&action=review > Source/WebCore/inspector/InspectorTimelineAgent.cpp:175 > + if (m_client && m_pageAgent) // Workers case. > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:2 > +* Copyright (C) 2012 Google Inc. All rights reserved. 2013
Comment on attachment 186015 [details] Patch Attachment 186015 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16293892
Comment on attachment 186015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186015&action=review > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:57 > + return &instance; Is it supposed to be called only on the main thread? If no, the initialization should be guarded with a mutex. > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:78 > + m_processors[index] = m_processors.last(); I don't think this place will be a bottleneck and would probably just call remove(index) here. > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142 > + HashMap<String, EventTypeEntry>::iterator it = m_handlersByType.find(name); How do you know that the processor is not destroyed when processEventOnAnyThread is called from another thread? > Source/WebCore/inspector/TimelineTraceEventProcessor.h:69 > + bool m_bool; Add default constructor. > Source/WebCore/inspector/TimelineTraceEventProcessor.h:90 > + : m_argumentCount(0) Initialize other parameters?
Comment on attachment 186015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186015&action=review > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:92 > + Vector<RefPtr<TimelineTraceEventProcessor> > processors; Consider passing inlineCapacity = 2 > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:123 > + static TraceValueUnion missingValue; const > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:190 > + Vector<TraceEvent> events; ASSERT(isMainThread())
Created attachment 186381 [details] Patch
Created attachment 186384 [details] Patch
Comment on attachment 186015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186015&action=review >> Source/WebCore/inspector/InspectorTimelineAgent.cpp:175 >> + if (m_client && m_pageAgent) > > // Workers case. done. >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:2 >> +* Copyright (C) 2012 Google Inc. All rights reserved. > > 2013 done >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:57 >> + return &instance; > > Is it supposed to be called only on the main thread? If no, the initialization should be guarded with a mutex. It's guaranteed to be _first_ called on the main thread (we would only subscribe from the main thread). >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:78 >> + m_processors[index] = m_processors.last(); > > I don't think this place will be a bottleneck and would probably just call remove(index) here. done. >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:92 >> + Vector<RefPtr<TimelineTraceEventProcessor> > processors; > > Consider passing inlineCapacity = 2 done. >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:123 >> + static TraceValueUnion missingValue; > > const done. >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142 >> + HashMap<String, EventTypeEntry>::iterator it = m_handlersByType.find(name); > > How do you know that the processor is not destroyed when processEventOnAnyThread is called from another thread? It's reference counted and one of the copies is held by the caller. >> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:190 >> + Vector<TraceEvent> events; > > ASSERT(isMainThread()) done >> Source/WebCore/inspector/TimelineTraceEventProcessor.h:69 >> + bool m_bool; > > Add default constructor. done >> Source/WebCore/inspector/TimelineTraceEventProcessor.h:90 >> + : m_argumentCount(0) > > Initialize other parameters? Also done for m_name.
Created attachment 186389 [details] Patch
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16370264
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16377159
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16371296
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16379161
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16375254
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16369290
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16378177
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16375262
Comment on attachment 186389 [details] Patch Attachment 186389 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16374283
Committed r144041: <http://trac.webkit.org/changeset/144041>
Reverted r144041 for reason: Broke build Committed r144053: <http://trac.webkit.org/changeset/144053>
Created attachment 190277 [details] EWS run
Committed r144062: <http://trac.webkit.org/changeset/144062>