Bug 89584

Summary: Web Inspector: Forward message loop instrumentation data to frontend.
Product: WebKit Reporter: eustas.bug
Component: Web Inspector (Deprecated)Assignee: eustas.bug
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, eustas.bug, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 91041    
Bug Blocks: 88325    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
yurys: review+
Patch
none
Patch none

Description eustas.bug 2012-06-20 11:26:35 PDT
Transmit collected message loop tasks to inspector frontend.
Now "Program" should be a top-level event where appropriate.
Frontend was changed so that user will not see any changes.
Comment 1 eustas.bug 2012-06-20 12:06:22 PDT
Created attachment 148623 [details]
Patch
Comment 2 Andrey Kosyakov 2012-06-20 16:59:41 PDT
Comment on attachment 148623 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:99
> +static const char Program[] = "Program";

We do seem to order these semantically, so I'd make Program the top one.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:457
> +    if (record->getString("type", &type)) {
> +        if (type == program)

if (record->getString("type", &type) && type == program)
   return;

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:54
> +        program: new WebInspector.TimelineCategory("program", WebInspector.UIString("Program"), -1, "transparent", "transparent", "transparent")

This is not supposed to ever be painted, so passing concrete values for colors seems strange. Can we just omit them?
The category may also be named "Hidden", I think.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:214
> +        var records = [record];
> +        if (record.type === recordTypes.Program)
> +            records = record.children;

var records = record.type === recordTypes.Program ? record.children : [record];
or if (...) records = ... else records = ...;

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:223
> +    _addRecord: function(record, parentRecord)

innerAddRecord?
Comment 3 eustas.bug 2012-06-20 17:51:16 PDT
Comment on attachment 148623 [details]
Patch

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:99
>> +static const char Program[] = "Program";
> 
> We do seem to order these semantically, so I'd make Program the top one.

Done

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:457
>> +        if (type == program)
> 
> if (record->getString("type", &type) && type == program)
>    return;

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:54
>> +        program: new WebInspector.TimelineCategory("program", WebInspector.UIString("Program"), -1, "transparent", "transparent", "transparent")
> 
> This is not supposed to ever be painted, so passing concrete values for colors seems strange. Can we just omit them?
> The category may also be named "Hidden", I think.

Done.
I think "hidden" is not very good, because it doesn't reflect the nature of activity.

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:214
>> +            records = record.children;
> 
> var records = record.type === recordTypes.Program ? record.children : [record];
> or if (...) records = ... else records = ...;

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:223
>> +    _addRecord: function(record, parentRecord)
> 
> innerAddRecord?

Done
Comment 4 eustas.bug 2012-06-20 18:41:18 PDT
Created attachment 148703 [details]
Patch
Comment 5 Pavel Feldman 2012-06-20 19:14:02 PDT
Comment on attachment 148703 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:451
>  void InspectorTimelineAgent::setHeapSizeStatistic(InspectorObject* record)

setHeapSizeStatistics

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:456
> +    if (record->getString("type", &type) && type == program)

I'd rather not call setHeapSizeStatistics for Program type.

> Source/WebCore/inspector/front-end/MemoryStatistics.js:260
> +        var record = event.data;

We should not expect counters in the second-level records only. When we fix the GC to be synchronous, the counters should only stay in Parse, Script and GC records (maybe the whitelist is longer).

> Source/WebCore/inspector/front-end/TimelineFrameController.js:69
> +        records.forEach(this._innerAddRecord, this);

DFS?
Comment 6 Yury Semikhatsky 2012-06-21 01:16:02 PDT
Comment on attachment 148703 [details]
Patch

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

>> Source/WebCore/inspector/front-end/MemoryStatistics.js:260
>> +        var record = event.data;
> 
> We should not expect counters in the second-level records only. When we fix the GC to be synchronous, the counters should only stay in Parse, Script and GC records (maybe the whitelist is longer).

What do you mean by "When we fix the GC to be synchronous" ?
Comment 7 eustas.bug 2012-06-21 11:01:23 PDT
Comment on attachment 148703 [details]
Patch

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:451
>>  void InspectorTimelineAgent::setHeapSizeStatistic(InspectorObject* record)
> 
> setHeapSizeStatistics

Done

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:456
>> +    if (record->getString("type", &type) && type == program)
> 
> I'd rather not call setHeapSizeStatistics for Program type.

Done

>>> Source/WebCore/inspector/front-end/MemoryStatistics.js:260
>>> +        var record = event.data;
>> 
>> We should not expect counters in the second-level records only. When we fix the GC to be synchronous, the counters should only stay in Parse, Script and GC records (maybe the whitelist is longer).
> 
> What do you mean by "When we fix the GC to be synchronous" ?

Done

>> Source/WebCore/inspector/front-end/TimelineFrameController.js:69
>> +        records.forEach(this._innerAddRecord, this);
> 
> DFS?

Unfortunately, no. This would require severe changes to time aggregation methods.
Comment 8 eustas.bug 2012-06-21 11:03:13 PDT
Created attachment 148842 [details]
Patch
Comment 9 Andrey Kosyakov 2012-06-21 13:44:56 PDT
Comment on attachment 148842 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePanel.js:492
> +        this._scheduleRefresh(false);

indent

> Source/WebCore/inspector/front-end/TimelinePanel.js:509
> +        function isAttachedRecord(record) {

isAdoptedRecord()?
style: { => next line

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:50
> +

no need for extra ws here

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:121
> +WebInspector.TimelinePresentationModel.forAllRecords = function(recordsArray, preOrderCallback, postOrderCallback)

Can we have a test for this?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:228
> +    _innerAddRecord: function(record, parentRecord)

so how about a recursive call this used to have?
Comment 10 eustas.bug 2012-06-21 14:40:17 PDT
Comment on attachment 148842 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePanel.js:492
>> +        this._scheduleRefresh(false);
> 
> indent

fixed

>> Source/WebCore/inspector/front-end/TimelinePanel.js:509
>> +        function isAttachedRecord(record) {
> 
> isAdoptedRecord()?
> style: { => next line

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:50
>> +
> 
> no need for extra ws here

fixed

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:121
>> +WebInspector.TimelinePresentationModel.forAllRecords = function(recordsArray, preOrderCallback, postOrderCallback)
> 
> Can we have a test for this?

Of sure.

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:228
>> +    _innerAddRecord: function(record, parentRecord)
> 
> so how about a recursive call this used to have?

Fixed.
Comment 11 eustas.bug 2012-06-21 15:20:41 PDT
Created attachment 148899 [details]
Patch
Comment 12 Andrey Kosyakov 2012-06-21 15:26:15 PDT
Comment on attachment 148899 [details]
Patch

LGTM
Comment 13 Pavel Feldman 2012-06-21 18:07:34 PDT
Comment on attachment 148899 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:133
> +                 stack.push({array: [], index: 0, record: record});

record: record should move into the same entry below.
Comment 14 eustas.bug 2012-06-21 18:30:50 PDT
Created attachment 148936 [details]
Patch
Comment 15 eustas.bug 2012-06-21 18:31:50 PDT
Comment on attachment 148899 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:133
>> +                 stack.push({array: [], index: 0, record: record});
> 
> record: record should move into the same entry below.

Done
Comment 16 Pavel Feldman 2012-06-21 20:19:24 PDT
Comment on attachment 148936 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:132
> +             if (record.children) {

nit: no need for { }
Comment 17 eustas.bug 2012-06-22 09:29:15 PDT
Created attachment 149039 [details]
Patch

Rebased
Comment 18 eustas.bug 2012-06-29 03:33:05 PDT
Created attachment 150128 [details]
Patch

Hide-program-category-checkbox
Comment 19 eustas.bug 2012-06-29 09:57:01 PDT
Created attachment 150201 [details]
Patch
Comment 20 Andrey Kosyakov 2012-07-03 05:23:01 PDT
Comment on attachment 150201 [details]
Patch

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

LGTM

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:227
> +        var formattedRecords = [];
> +        var recordsCount = records.length;
> +        for (var i = 0; i < recordsCount; ++i)
> +            formattedRecords.push(this._innerAddRecord(records[i], parentRecord));
> +        return formattedRecords;

nit: swap arguments to innerAddRecord, return records.map(this._innerAddRecord.bind(this, parentRecord));
Comment 21 Yury Semikhatsky 2012-07-03 05:56:37 PDT
Comment on attachment 150201 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Now "Program" should be a top-level event where appropriate.

Could you please elaborate on what "appropriate" means.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:212
>          var recordTypes = WebInspector.TimelineModel.RecordType;

There is only one usage, inline this?
Comment 22 eustas.bug 2012-07-03 07:15:36 PDT
Comment on attachment 150201 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Now "Program" should be a top-level event where appropriate.
> 
> Could you please elaborate on what "appropriate" means.

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:212
>>          var recordTypes = WebInspector.TimelineModel.RecordType;
> 
> There is only one usage, inline this?

Done
Comment 23 eustas.bug 2012-07-03 07:21:06 PDT
Created attachment 150604 [details]
Patch

Rebased-and-fixed-nits
Comment 24 eustas.bug 2012-07-03 08:12:28 PDT
Created attachment 150611 [details]
Patch

Rebased-and-fixed-js-compilation-warnings
Comment 25 Andrey Kosyakov 2012-07-03 08:27:31 PDT
Committed r121767: <http://trac.webkit.org/changeset/121767>
Comment 26 Andrey Kosyakov 2012-07-04 10:40:03 PDT
Reverted r121767 for reason:

Crashes inspected page while recording timeline due to conflict with BeginFrame in record stack

Committed r121865: <http://trac.webkit.org/changeset/121865>
Comment 27 Andrey Kosyakov 2012-07-11 01:57:37 PDT
Committed r122312: <http://trac.webkit.org/changeset/122312>