Bug 89584 – Web Inspector: Forward message loop instrumentation data to frontend.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

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.

Created attachment 148623 [details]
Patch

[Andrey Kosyakov]

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 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

Created attachment 148703 [details]
Patch

[Pavel Feldman]

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?

[Yury Semikhatsky]

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 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.

Created attachment 148842 [details]
Patch

[Andrey Kosyakov]

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 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.

Created attachment 148899 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 148899 [details]
Patch

LGTM

[Pavel Feldman]

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.

Created attachment 148936 [details]
Patch

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

[Pavel Feldman]

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 { }

Created attachment 149039 [details]
Patch

Rebased

Created attachment 150128 [details]
Patch

Hide-program-category-checkbox

Created attachment 150201 [details]
Patch

[Andrey Kosyakov]

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));

[Yury Semikhatsky]

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 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

Created attachment 150604 [details]
Patch

Rebased-and-fixed-nits

Created attachment 150611 [details]
Patch

Rebased-and-fixed-js-compilation-warnings

[Andrey Kosyakov]

Committed r121767: <http://trac.webkit.org/changeset/121767>

[Andrey Kosyakov]

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>

[Andrey Kosyakov]

Committed r122312: <http://trac.webkit.org/changeset/122312>