Bug 112172 - Web Inspector: do not aggregate non-main thread timeline events, handle them a generic fashion
Summary: Web Inspector: do not aggregate non-main thread timeline events, handle them ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on: 112168
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-12 11:24 PDT by Andrey Kosyakov
Modified: 2013-03-14 07:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (24.43 KB, patch)
2013-03-12 11:34 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (24.45 KB, patch)
2013-03-13 01:21 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (24.45 KB, patch)
2013-03-13 10:01 PDT, Andrey Kosyakov
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2013-03-12 11:24:22 PDT
This removes the logic to aggregate rasterize events in timeline agent and instead sends them, along with potential nested events, to front-end, similar to how main thread events are handled. These are later supposed to be collated with front-end event collation logic from bug 112168.
Comment 1 Andrey Kosyakov 2013-03-12 11:34:14 PDT
Created attachment 192777 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-12 12:05:54 PDT
Comment on attachment 192777 [details]
Patch

Attachment 192777 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17173227
Comment 3 Andrey Kosyakov 2013-03-13 01:21:38 PDT
Created attachment 192879 [details]
Patch
Comment 4 Vsevolod Vlasov 2013-03-13 08:26:07 PDT
Comment on attachment 192879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192879&action=review

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142
> +    return m_stack.isEmpty() || (m_stack.last().record->getString("type", &lastRecordType) && type == lastRecordType);

return !m_stack.isEmpty() && (m_stack.last().record->getString("type", &lastRecordType) && type == lastRecordType);

> Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:245
> +    ASSERT(!state.rasterizeStartStackLevel);

Looks like rasterizeStartStackLevel could be a boolean (isInRasterize).
Comment 5 Andrey Kosyakov 2013-03-13 10:01:13 PDT
Created attachment 192941 [details]
Patch
Comment 6 Andrey Kosyakov 2013-03-13 10:05:10 PDT
(In reply to comment #4)
> (From update of attachment 192879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192879&action=review
> 
> > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:142
> > +    return m_stack.isEmpty() || (m_stack.last().record->getString("type", &lastRecordType) && type == lastRecordType);
> 
> return !m_stack.isEmpty() && (m_stack.last().record->getString("type", &lastRecordType) && type == lastRecordType);

This is written as intended (though may be the method name is unfortunate), not having the top event is considered ok for the purpose of validation -- we hit this case if we started recording in the middle of the event.

> > Source/WebCore/inspector/TimelineTraceEventProcessor.cpp:245
> > +    ASSERT(!state.rasterizeStartStackLevel);
> 
> Looks like rasterizeStartStackLevel could be a boolean (isInRasterize).

Fair, this is atavism of an earlier implementation. Fixed.
Comment 7 Andrey Kosyakov 2013-03-14 07:07:27 PDT
Committed r145809: <http://trac.webkit.org/changeset/145809>