[webkit-reviews] review canceled: [Bug 195202] Web Inspector: CPU Usage Timeline - Statistics and Sources sections : [Attachment 363404] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 5 18:54:21 PST 2019
Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195202: Web Inspector: CPU Usage Timeline - Statistics and Sources sections
https://bugs.webkit.org/show_bug.cgi?id=195202
Attachment 363404: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=363404&action=review
--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 363404
--> https://bugs.webkit.org/attachment.cgi?id=363404
[PATCH] Proposed Fix
View in context: https://bugs.webkit.org/attachment.cgi?id=363404&action=review
>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:121
>> +Object.defineProperty(Map.prototype, "getOrInitialize",
>
> Is this something we care about testing? It seems pretty trivial...
Yeah, I thought about it and passed just because it was so simple. But I added
one.
>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:321
>>> +.timeline-view.cpu > .content > .overview > .chart > .container.stats {
>>
>> I suggest to allow copying text here by adding:
>>
>> -webkit-user-select: text;
>>
>> I can see myself:
>> - copying and sharing this data.
>> - copying method names to find them in my IDE.
>
> I agree with both, but I think I'd be a bit more restrictive as to what we
allow to be copied. As an example, the filter "bubbles" don't (and probably
shouldn't) need to be copied. I think we should limit it to the Sources counts
and names/locations.
Text selection works great. The filter bubbles select great as well since they
are just text.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:38
>> + this._sectionLimit = 5;
>
> The `5` would be great as a `static get`.
Done.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:327
>> + observer: new Set,
>
> Rather than having these keys be something you have to remember, we could
leverage computed keys and a static object.
> ```
> this._sourcesFilter = {};
> for (let type in Object.values(WI.CPUTimelineView.Source))
> this._sourceFilter[type] = new Set;
>
> WI.CPUTimelineView.Source = {
> Timer: "timer",
> Event: "event",
> Observer: "observer",
> };
> ```
This seems rather overkill. Is there any advantage?
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:366
>> + filterClearElement.textContent = "\u00D7";
>
> Can you make this into a variable (or put it in Utilities.js) so we know what
it is without having to look it up?
Heh, it already was: `multiplicationSign`. I missed it because of a case
sensitive search.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:757
>> + span.className = "filter";
>
> NIT: `classList.add`.
className is faster for a newly created element with a known list. I believe
that is our preferred style, at least that is what I have always seen and done.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:759
>> + span.addEventListener("mouseup", (event) => {
>
> Why not use `"click"`?
We do "mouseup" instead of "click" because during a recording the elements
change. So the one you mousedown'd on is different from the one you mouseup'd
on and click doesn't work, but mouseup does.
We do have a flicker issue with mouse hovering these during an active recording
already.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:769
>> + let sectionLimit = this._sectionLimit;
>
> Why did you save this to a local?
This matches the style of the other section generation, which doubled the
sectionLimit when there was a filter. It would be useful in case we wanted to
do something similar in this section, but I've removed it for now.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:771
>> + let expandAllSections = () => {
>
> This function could be inlined, as it's only used once. Alternatively, you
could make this into a member function and share it between
`_layoutStatisticsSection` and `_layoutSourcesSection`.
It requires saving the `statistics` into a member variable, but I may do that
to convert some `updateLayout(Internal)` to `updateStatisiticsAndSources`.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:839
>> + function firstNonNativeCallFrame(callFrames) {
>
> This looks like it does the same thing as
`WI.TimelineRecord.prototype.initiatorCallFrame`. Can you use that instead?
Yep, nice!
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:852
>> + return sourceCodeLocation.sourceCode.url + ":" +
sourceCodeLocation.lineNumber + ":" + sourceCodeLocation.columnNumber;
>
>
`sourceCodeLocation.originalLocationString(WI.SourceCodeLocation.ColumnStyle.Sh
own, WI.SourceCodeLocation.NameStyle.Short)`?
This does a lot more work, this is getting called potentially thousands of
times so I want it to be simple.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:919
>> + entry.count += count;
>
> Just making sure I understand; is it guaranteed that a repeating timer will
only appear in `repeatingTimers` once (e.g. I'm worried about double counting
because of the `+=`).
There are two lists here:
• timerMap shows timers installations in the selected time range
- this will only ever happen once per timer id, and we are ignoring
timer ids overflowing
• repeatingTimers only shows timers have fired multiple times in the
selected time range
- this will only ever have values for a timer that fired multiple
times, not a timer that fired once
If we saw a repeating timer, then its fire count is at least 2 and we try to
use that to count as the repeating timer firing N times.
Since Sources is about installations, we aggreate repeated timers to an
installation point where possible.
If we have the following:
setInterval fire fire
time: | * * * |
range 1: |----------------------------------| => Timer Installed, Repeat
of 2 => ~2 (the number of fires of this installed timer in range, with
install location)
range 2: |----------------------| => Timer Installed, No
Repeat seen => ~1 (the number of fires of this installed timer in range, with
install location)
range 3: |------| => Timer Installed, No
Repeat seen => ~1 (the number of fires of this installed timer in range, with
install location)
range 4: |---------------------| => No Installed, Repeat of
2 => ~2 (the number of fires of this installed timer in range, with fire
location)
range 5: |------| => No Installed, Fire
happened once => nothing (we don't know where it was installed, or where if it
is repeating)
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:920
>> + entry.repeating = record.details.repeating;
>
> Should we only be setting this in the `true` case? IIUC, it's wouldn't be
possible for a repeating and non-repeating timer to both be fired from the same
location, so this would be "consistent" between iterations. Should we be
asserting as such?
Good point. Apparently it is possible to get `setInterval` and `setTimeout` to
have the same location:
<script>
function doThing(f) {
f(function() { console.log("fire") }, 1000);
}
doThing(setTimeout);
doThing(setInterval);
doThing(setTimeout);
</script>
In this case they would all be grouped at the same place in `doThing` and not
indicated as non-repeating depending on your range.
I'll change this to only go to true and not reset to false:
if (record.details.repeating)
entry.repeating = true;
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:971
>> + let number = entry.repeating ? "~" + entry.count :
entry.count;
>
> Should this be localized?
Good idea.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:975
>> + let row;
>
> Style: this should be initialized to some value (e.g. `null`).
It is initialized to undefined and the two branches of the if/else below will
initialize it. This is not uncommon style.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:984
>> + row.querySelector(".label").append(` ${enDash}
${entry.functionName}`);
>
> Rather than querying for an element, you could just add this at the end of
`label` before you pass it around.
> ```
> if (entry.functionName)
> label += ` ${enDash} ${entry.functionName}`;
> ```
Label is not always a string, it is commonly a source code location link. If it
was an element I'd want to append a sibling and need to get its parent anyways.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1370
>> +
this._sourcesFilterLabelElement.appendChild(createActiveFilterElement("observer
", name));
>
> Does this do any wrapping or allow scrolling? If a lot of filters are
selected, does that mean it extends past the edge of the window?
Scrolling is allowed.
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1417
>> + row.remove();
>
> Is this the equivalent of `this._statisticsTable.removeChildren`?
No, the statisticsTable has a static row that doesn't get regenerated ("Script
Entries").
More information about the webkit-reviews
mailing list