Web Inspector: CPU Usage Timeline - the right edge of each column should align with...
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 20:13:03 +0000 (20:13 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Mar 2019 20:13:03 +0000 (20:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195789
<rdar://problem/48915271>

Reviewed by Joseph Pecoraro.

Right now, each column is rendered such that the middle of the column is aligned with the
time of the CPU measurement. This could potentially be misleading, as the width/position of
the bar implies that there was a period of time after the actual time of the CPU measurement
that should be "attributed" to that same CPU measurement.

   1      2      3
       _______
       [  *  ]
       [  *  ]_______
       [  *  ][  *  ]
_______[  *  ][  *  ]
[  *  ][  *  ][  *  ]
[__*__][__*__][__*__]
 A   B  C   D  E   F

In this example, one might "attribute" any work done at time B to record 1, when in reality,
it should be "attributed" to record 2, since the CPU measurement had already been taken by
the time B was captured, meaning that the work for B hadn't yet been done and could
therefore not have affected the CPU measurement for record 1.

We should be rendering the columns such that the CPU measurement aligns with the trailing
edge of the column, so that all of the work that could be "attributed" to a given CPU
measurement comes before it.

  1      2      3
    _______       ___
    [    *]       [
    [    *]_______[
    [    *][    *][
____[    *][    *][
  *][    *][    *][
__*][____*][____*][__
 A   B  C   D  E   F

        NOTE: this "rendering" isn't exactly accurate, as the `*` should overlap the `]`.

Legend:
 - `[     ]` represents a column for a CPU measurement
 - `*` represents the time when the measurement actually takes place

* UserInterface/Views/CPUTimelineOverviewGraph.js:
(WI.CPUTimelineOverviewGraph.prototype.layout):
(WI.CPUTimelineOverviewGraph.prototype._handleChartClick):
* UserInterface/Views/TimelineOverview.js:
(WI.TimelineOverview.prototype._recordSelected):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243166 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js
Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js

index 57c75c5..d7ddc8c 100644 (file)
@@ -1,5 +1,59 @@
 2019-03-19  Devin Rousso  <drousso@apple.com>
 
+        Web Inspector: CPU Usage Timeline - the right edge of each column should align with a CPU measurement
+        https://bugs.webkit.org/show_bug.cgi?id=195789
+        <rdar://problem/48915271>
+
+        Reviewed by Joseph Pecoraro.
+
+        Right now, each column is rendered such that the middle of the column is aligned with the
+        time of the CPU measurement. This could potentially be misleading, as the width/position of
+        the bar implies that there was a period of time after the actual time of the CPU measurement
+        that should be "attributed" to that same CPU measurement.
+
+           1      2      3
+               _______
+               [  *  ]
+               [  *  ]_______
+               [  *  ][  *  ]
+        _______[  *  ][  *  ]
+        [  *  ][  *  ][  *  ]
+        [__*__][__*__][__*__]
+         A   B  C   D  E   F
+
+        In this example, one might "attribute" any work done at time B to record 1, when in reality,
+        it should be "attributed" to record 2, since the CPU measurement had already been taken by
+        the time B was captured, meaning that the work for B hadn't yet been done and could
+        therefore not have affected the CPU measurement for record 1.
+
+        We should be rendering the columns such that the CPU measurement aligns with the trailing
+        edge of the column, so that all of the work that could be "attributed" to a given CPU
+        measurement comes before it.
+
+          1      2      3
+            _______       ___
+            [    *]       [
+            [    *]_______[
+            [    *][    *][
+        ____[    *][    *][
+          *][    *][    *][
+        __*][____*][____*][__
+         A   B  C   D  E   F
+
+                NOTE: this "rendering" isn't exactly accurate, as the `*` should overlap the `]`.
+
+        Legend:
+         - `[     ]` represents a column for a CPU measurement
+         - `*` represents the time when the measurement actually takes place
+
+        * UserInterface/Views/CPUTimelineOverviewGraph.js:
+        (WI.CPUTimelineOverviewGraph.prototype.layout):
+        (WI.CPUTimelineOverviewGraph.prototype._handleChartClick):
+        * UserInterface/Views/TimelineOverview.js:
+        (WI.TimelineOverview.prototype._recordSelected):
+
+2019-03-19  Devin Rousso  <drousso@apple.com>
+
         Web Inspector: Provide $event in the console when paused on an event listener
         https://bugs.webkit.org/show_bug.cgi?id=188672
 
index ef1337f..087a4a6 100644 (file)
@@ -116,7 +116,7 @@ WI.CPUTimelineOverviewGraph = class CPUTimelineOverviewGraph extends WI.Timeline
         }
 
         const includeRecordBeforeStart = true;
-        let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (CPUTimelineOverviewGraph.samplingRatePerSecond / 2), includeRecordBeforeStart);
+        let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime, includeRecordBeforeStart);
         if (!visibleRecords.length)
             return;
 
@@ -124,14 +124,13 @@ WI.CPUTimelineOverviewGraph = class CPUTimelineOverviewGraph extends WI.Timeline
             return yScale(record.usage);
         }
 
-        let intervalWidth = (CPUTimelineOverviewGraph.samplingRatePerSecond / secondsPerPixel);
+        let intervalWidth = CPUTimelineOverviewGraph.samplingRatePerSecond / secondsPerPixel;
         const minimumDisplayHeight = 4;
 
-        // Bars for each record.
         for (let record of visibleRecords) {
             let additionalClass = record === this.selectedRecord ? "selected" : undefined;
             let w = intervalWidth;
-            let x = xScale(record.startTime - (CPUTimelineOverviewGraph.samplingRatePerSecond / 2));            
+            let x = xScale(record.startTime - CPUTimelineOverviewGraph.samplingRatePerSecond);
             let h1 = Math.max(minimumDisplayHeight, yScale(record.mainThreadUsage));
             let h2 = Math.max(minimumDisplayHeight, yScale(record.mainThreadUsage + record.workerThreadUsage));
             let h3 = Math.max(minimumDisplayHeight, yScale(record.usage));
@@ -200,7 +199,7 @@ WI.CPUTimelineOverviewGraph = class CPUTimelineOverviewGraph extends WI.Timeline
         let graphStartTime = this.startTime;
 
         let clickTime = graphStartTime + graphClickTime;
-        let record = this._cpuTimeline.closestRecordTo(clickTime);
+        let record = this._cpuTimeline.closestRecordTo(clickTime + (CPUTimelineOverviewGraph.samplingRatePerSecond / 2));
         if (!record)
             return;
 
index aa6a3a4..a1560e9 100644 (file)
@@ -788,7 +788,7 @@ WI.TimelineOverview = class TimelineOverview extends WI.View
 
             if (firstRecord instanceof WI.CPUTimelineRecord) {
                 let selectionPadding = WI.CPUTimelineOverviewGraph.samplingRatePerSecond * 2.25;
-                this.selectionStartTime = startTime - selectionPadding;
+                this.selectionStartTime = startTime - selectionPadding - (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2);
                 this.selectionDuration = endTime - startTime + (selectionPadding * 2);
             } else if (startTime < this.selectionStartTime || endTime > this.selectionStartTime + this.selectionDuration) {
                 let selectionPadding = this.secondsPerPixel * 10;