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
eustas.bug
2012-06-20 11:26:35 PDT
Created attachment 148623 [details]
Patch
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
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 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
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
Comment on attachment 148899 [details]
Patch
LGTM
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 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
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 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
Committed r121767: <http://trac.webkit.org/changeset/121767> 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> Committed r122312: <http://trac.webkit.org/changeset/122312> |