Summary: | Web Inspector: [experimental] add a mode to display timeline events aligned by the start time | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2012-02-21 09:54:43 PST
Created attachment 127990 [details]
Patch
Screenshot? Comment on attachment 127990 [details] Patch Attachment 127990 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11560177 New failing tests: inspector/network-status-non-http.html http/tests/inspector/network/network-sidebar-width.html http/tests/inspector/network/network-initiator-from-console.html Created attachment 128174 [details]
Aligned mode, few events (columns in the overview are wide and rare)
Created attachment 128175 [details]
Aligned mode, many events (overview columns are dense; selection window visible)
(In reply to comment #2) > Screenshot? Added. We did not decide on the best way to toggle it yet, and this is the primary reason it's behind the experimental switch. Currently, the mode is toggled by a button in the status bar, and when it's on, the Timeline/Memory toggle in the overview sidebar is hidden. We're considering to revamp the overview sidebar, so that the "normal" timeline mode, overview mode and performance graphs are controlled from there -- however, there are no particular ideas on how the control should look yet. Comment on attachment 127990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127990&action=review I don't think you should remove the Timeline/Memory buttons when your mode toggles. > Source/WebCore/inspector/front-end/TimelineGrid.js:118 > + dividerLabelBar._labelElement.textContent = calculator.formatTime(slice * i); why did this change? Calculator could be used for arranging rows by size or latency, etc. > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:40 > + this._alignedMode = false; this._startAtZero > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:76 > + this._alignedOverview = new WebInspector.TimelineAlignedModeOverview(this._presentationModel); lets create it lazily > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:138 > + setAlignedMode: function(enabled) setStartAtZero > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:144 > + this.element.addStyleClass("timeline-aligned-mode"); timeline-start-at-zero. > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:757 > + var calculator = new WebInspector.AlignedTimelineCalculator(); do you need to create calculator on each update? > Source/WebCore/inspector/front-end/TimelinePanel.js:687 > + if (!this._boundariesAreValid) why did this change? > Source/WebCore/inspector/front-end/TimelinePanel.js:713 > + var filter = this._alignMode ? new WebInspector.TimelineAlignedRecordFilter(this._presentationModel, this._rootRecord, this._showShortEvents) You don't really need to re-create filter each time? > Source/WebCore/inspector/front-end/TimelinePanel.js:-717 > - if (updateBoundaries) why did this change? > Source/WebCore/inspector/front-end/TimelinePanel.js:1718 > +WebInspector.TimelineRecordFilter = function(calculator, showShortEvents) Ok, we will hit 2K lines here soon. Time to extract some classes. Created attachment 128191 [details]
Patch
(In reply to comment #7) > (From update of attachment 127990 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127990&action=review > > I don't think you should remove the Timeline/Memory buttons when your mode toggles. I think the primary use case for memory mode is to correlate changes in heap size against timeline events -- so it does not look like it has much meaning when all events are aligned; on the other hand, we gain some real estate for the overview bars this way. I guess we can change this once we manage to fit all mode controls to the overview sidebar. > > > Source/WebCore/inspector/front-end/TimelineGrid.js:118 > > + dividerLabelBar._labelElement.textContent = calculator.formatTime(slice * i); > > why did this change? Calculator could be used for arranging rows by size or latency, etc. It's just that we only used time in timeline. Ok, I'm bringing this back to formatValue(). > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:40 > > + this._alignedMode = false; > > this._startAtZero Done. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:76 > > + this._alignedOverview = new WebInspector.TimelineAlignedModeOverview(this._presentationModel); > > lets create it lazily Done. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:138 > > + setAlignedMode: function(enabled) > > setStartAtZero Done. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:144 > > + this.element.addStyleClass("timeline-aligned-mode"); > > timeline-start-at-zero. Done. > > Source/WebCore/inspector/front-end/TimelineOverviewPane.js:757 > > + var calculator = new WebInspector.AlignedTimelineCalculator(); > > do you need to create calculator on each update? Fixed. > > Source/WebCore/inspector/front-end/TimelinePanel.js:687 > > + if (!this._boundariesAreValid) > > why did this change? It's a drive-by change, I'd like to take boundaries update logic out of refreshRecords(). It's a trivial change, refreshRecords() is only called from here. > > Source/WebCore/inspector/front-end/TimelinePanel.js:713 > > + var filter = this._alignMode ? new WebInspector.TimelineAlignedRecordFilter(this._presentationModel, this._rootRecord, this._showShortEvents) > > You don't really need to re-create filter each time? I do, since it maintains some state that needs to be reset when we start enumerating records from 0 (one filter counts records, another holds a calculator). > > Source/WebCore/inspector/front-end/TimelinePanel.js:-717 > > - if (updateBoundaries) > > why did this change? See above. > > Source/WebCore/inspector/front-end/TimelinePanel.js:1718 > > +WebInspector.TimelineRecordFilter = function(calculator, showShortEvents) > > Ok, we will hit 2K lines here soon. Time to extract some classes. I'm doing this gradually by extracting some stuff into presentation model, filters etc. We can move them to a separate file later. Committed r108502: <http://trac.webkit.org/changeset/108502> WebKit policy is to always have classes in seperate files. We should do the same in the Inspector. It would encourage less spaghetti code and private function/property access. (In reply to comment #11) > WebKit policy is to always have classes in seperate files. We should do the same in the Inspector. It would encourage less spaghetti code and private function/property access. +1, I'm going to push for < 1KLOC classes. Private classes are still fine to belong to the same unit as long as it does not grow > 1KLOC. |