Closed Bug 1152992 Opened 5 years ago Closed 4 years ago

[marker] Timeline should handle markers without being explicitly defined

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

37 Branch
x86
macOS
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

To ease integration of timeline markers, a platform developer should not have to modify the front end code -- We should have a default marker group, color.

Subsequently after the platform markers have been added, we should decide on which group this marker should be in, localize it, pick a color, and whether or not this should be behind a pref.
Update https://wiki.mozilla.org/DevTools/OperationInstrument on completion
Assignee: nobody → jsantell
Summary: Timeline should handle markers without being explicitly defined → [marker] Timeline should handle markers without being explicitly defined
Duplicate of this bug: 1170712
Attached patch 1152992-othermarkers.patch (obsolete) — Splinter Review
This was harder than you would imagine
Attachment #8614865 - Flags: review?(vporof)
Comment on attachment 8614865 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8614865 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/markers.js
@@ +57,5 @@
>  const TIMELINE_BLUEPRINT = {
> +  /* Default definition used for markers that occur but
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {

Nit: how about UNKNOWN instead of OTHER?

@@ +58,5 @@
> +  /* Default definition used for markers that occur but
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {
> +    group: 1,

Why group 1? I'd put it either in the first or the last group.

@@ +59,5 @@
> +   * are not defined here. Should ultimately be defined, but this gives
> +   * us room to work on the front end separately from the platform. */
> +  "OTHER": {
> +    group: 1,
> +    colorName: "graphs-blue",

A bleak color is probably nicer and signifies that we have no idea what this is.

::: browser/devtools/performance/modules/widgets/marker-view.js
@@ +172,5 @@
>  
> +    // If this marker type has been marked hidden via marker filtering,
> +    // then don't display it. This is so we can also handle "Other" markers
> +    // that do not have a blueprint, but we want to show those anyway unless "Other"
> +    // markers are filtered.

If we rename "Other" to UNKNOWN, then update this comment.

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +74,5 @@
>    setBlueprint: function(blueprint) {
>      this._paintBatches = new Map();
>      this._lastGroup = 0;
>  
> +    for (let type of Object.keys(blueprint).filter(e => !blueprint[e].hidden)) {

If this graph is passed the filtered blueprint without the hidden markers, then why would this be necessary?

If this graph isn't passed the filtered blueprint, then extra rows won't be hidden will they?

@@ +109,1 @@
>        if (markerType) {

This conditional is useless now, right?
Attachment #8614865 - Flags: review?(vporof) → review+
+1 on color, naming and row. Can use grey for it, and change console time colors. 

We can't remove filtered markers from the blueprint; then they become UNKNOWN. So we just tag a "hidden" prop on there to truly filter.
Attached patch 1152992-othermarkers.patch (obsolete) — Splinter Review
Attachment #8614865 - Attachment is obsolete: true
Attachment #8617535 - Flags: review+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Whiteboard: [fixed-in-fx-team]
This one is fascinating
Flags: needinfo?(jsantell)
Crash has something to do with the Cu.cloneInto stuff for copying the blueprint. Changed to always use the same blueprint, but use a filter array of marker types that should not be displayed. r? again for those changes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=134ba2b35cad
Attachment #8617535 - Attachment is obsolete: true
Attachment #8617725 - Flags: review?(vporof)
Comment on attachment 8617725 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8617725 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +26,5 @@
> + * Takes a marker, blueprint, and filter list and
> + * determines if this marker should be filtered or not.
> + */
> +function isMarkerValid (marker, blueprint, filter) {
> +  let isUnknown = !blueprint[marker.name];

Nit: !(marker.name in blueprint) is probably more accurate. Maybe someday we want to handle null or undefineds differently?

::: browser/devtools/performance/modules/markers.js
@@ +135,5 @@
>      }],
>    },
>    "TimeStamp": {
>      group: 2,
> +    colorName: "graphs-yellow",

Can this stay blue? It might be confusing to have it yellow, just like the reflow/js markers.

::: browser/devtools/performance/modules/widgets/marker-view.js
@@ +10,5 @@
>  
>  const { Cc, Ci, Cu, Cr } = require("chrome");
>  const { Heritage } = require("resource:///modules/devtools/ViewHelpers.jsm");
>  const { AbstractTreeItem } = require("resource:///modules/devtools/AbstractTreeItem.jsm");
> +const { getBlueprintFor } = require("devtools/performance/markers");

This should be lazy required?

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +80,4 @@
>  
> +    let observedGroups = new Set();
> +
> +    for (let type of Object.keys(this._blueprint)) {

No need for Object.keys here. You could either use a for..in loop, or more nicely:

for (let [type, details] of Iterator(this._blueprint)) {
}

@@ +98,1 @@
>      }

Nice

@@ +124,2 @@
>      for (let marker of markers) {
> +

Nit: extra newline.

::: browser/devtools/performance/performance-controller.js
@@ +403,5 @@
>    /**
>     * Gets the current timeline blueprint without the hidden markers.
>     * @return object
>     */
>    getTimelineBlueprint: function() {

Well this function is useless now. Remove it.

@@ +410,5 @@
> +
> +  /**
> +   * Returns the currently hidden marker types as an array.
> +   */
> +  getHiddenMarkerTypes: function () {

Is this really necessary?
Attachment #8617725 - Flags: review?(vporof) → review+
Comment on attachment 8617725 [details] [diff] [review]
1152992-othermarkers.patch

Review of attachment 8617725 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/markers.js
@@ +135,5 @@
>      }],
>    },
>    "TimeStamp": {
>      group: 2,
> +    colorName: "graphs-yellow",

Sure -- the yellow seemed to make sense due to always being triggered by JS, and able to be seen ontop the blue-grey of the ConsoleTime

::: browser/devtools/performance/modules/widgets/markers-overview.js
@@ +98,1 @@
>      }

Yeah i was pretty proud of this one.

::: browser/devtools/performance/performance-controller.js
@@ +411,5 @@
> +  /**
> +   * Returns the currently hidden marker types as an array.
> +   */
> +  getHiddenMarkerTypes: function () {
> +    return this.getPref("hidden-markers");

both getHiddenMarkerTypes and getTimelineBlueprint could be done wherever they're needed -- probably can get rid of the blueprint one, since we no longer have the notion of multiple blueprints.

For the hidden markers, wanted the controller to be the arbiter of info that gets distributed to the views... but we have other prefs accessible there. I think for the extensibility story, this'll need to be changed up anyway.

Removing these two functions
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e4738a23d0c4
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.