Remove the concept of static fields from the marker schema.

Use an optional description field instead.

This causes less trouble with our Flow types. It's also a clearer API.
And it will make it easier to switch marker.data from being an object
to it being an array, in field order, because we'll be able to say that
there should be one array element per field.

Author: mstange

docs-developer/CHANGELOG-formats.md

@@ -6,6 +6,16 @@ Note that this is not an exhaustive list. Processed profile format upgraders can
 
 ## Processed profile format
 
+### Version 55
+
+Changes to the `MarkerSchema` type which is used for the elements of the array at `profile.meta.markerSchema`:
+
+- A new `description` field was added. This field is optional.
+- The `data` property was renamed to `fields`.
+- Every field must have a `key` and a `format` property now. There are no static fields any more.
+
+Concretely, this means that if you have a `{ "label": "Description", value: "..." }` entry in your marker schema's `data` array, this entry needs to be removed and the description needs to be put into the `description` field instead, and the `data` property needs to be renamed to `fields`. If you have any other static fields, i.e. fields with `label` and `value` properties rather than `key` and `format` properties, then they need to be removed without replacement.
+
 ### Version 54
 
 The `implementation` column was removed from the frameTable. Modern profiles from Firefox use subcategories to represent the information about the JIT type of a JS frame.

src/app-logic/constants.js

@@ -14,7 +14,7 @@ export const GECKO_PROFILE_VERSION = 31;
 // The current version of the "processed" profile format.
 // Please don't forget to update the processed profile format changelog in
 // `docs-developer/CHANGELOG-formats.md`.
-export const PROCESSED_PROFILE_VERSION = 54;
+export const PROCESSED_PROFILE_VERSION = 55;
 
 // The following are the margin sizes for the left and right of the timeline. Independent
 // components need to share these values.

src/components/tooltip/Marker.js

@@ -241,52 +241,43 @@ class MarkerTooltipContents extends React.PureComponent<Props> {
       // Add the details for the markers based on their Marker schema.
       const schema = getSchemaFromMarker(markerSchemaByName, marker.data);
       if (schema) {
-        for (const schemaData of schema.data) {
-          if (schemaData.hidden) {
-            // Do not include the data field if it's marked as hidden.
+        for (const field of schema.fields) {
+          if (field.hidden) {
+            // Do not include hidden fields.
             continue;
           }
 
-          // Check for a schema that is looking up and formatting a value from
-          // the payload.
-          if (schemaData.value === undefined) {
-            const { key, label, format } = schemaData;
-            if (key in data) {
-              const value = data[key];
-
-              // Don't add undefined values, as values are optional.
-              if (value !== undefined && value !== null) {
-                details.push(
-                  <TooltipDetail
-                    key={schema.name + '-' + key}
-                    label={label || key}
-                  >
-                    {formatMarkupFromMarkerSchema(
-                      schema.name,
-                      format,
-                      value,
-                      thread.stringTable,
-                      threadIdToNameMap,
-                      processIdToNameMap
-                    )}
-                  </TooltipDetail>
-                );
-              }
-            }
-          }
+          const { key, label, format } = field;
 
-          // Do a check to see if there is no key. This means this is a simple
-          // label that is applied to every marker of this type, with no data
-          // lookup. For some reason Flow as not able to refine this.
-          if (schemaData.key === undefined) {
-            const { label, value } = schemaData;
-            const key = label + '-' + value;
-            details.push(
-              <TooltipDetail key={key} label={label}>
-                <div className="tooltipDetailsDescription">{value}</div>
-              </TooltipDetail>
-            );
+          const value = data[key];
+          if (value === undefined || value === null) {
+            // This marker doesn't have a value for this field. Values are optional.
+            continue;
           }
+
+          details.push(
+            <TooltipDetail key={schema.name + '-' + key} label={label || key}>
+              {formatMarkupFromMarkerSchema(
+                schema.name,
+                format,
+                value,
+                thread.stringTable,
+                threadIdToNameMap,
+                processIdToNameMap
+              )}
+            </TooltipDetail>
+          );
+        }
+
+        if (schema.description) {
+          const key = schema.name + '-description';
+          details.push(
+            <TooltipDetail key={key} label="Description">
+              <div className="tooltipDetailsDescription">
+                {schema.description}
+              </div>
+            </TooltipDetail>
+          );
         }
       }
 

src/profile-logic/import/chrome.js

@@ -939,7 +939,7 @@ function extractMarkers(
       tooltipLabel: '{marker.data.type2} - EventDispatch',
       tableLabel: '{marker.data.type2}',
       display: ['marker-chart', 'marker-table', 'timeline-overview'],
-      data: [
+      fields: [
         {
           // In the original chrome profile, the key is `type`, but we rename it
           // so that it doesn't clash with our internal `type` property.

src/profile-logic/marker-data.js

@@ -1434,36 +1434,29 @@ export function sanitizeFromMarkerSchema(
   markerSchema: MarkerSchema,
   markerPayload: MarkerPayload
 ): MarkerPayload {
-  for (const propertyDescription of markerSchema.data) {
-    if (
-      propertyDescription.key !== undefined &&
-      propertyDescription.format !== undefined
-    ) {
-      const key = propertyDescription.key;
-      const format = propertyDescription.format;
-      if (!(key in markerPayload)) {
-        continue;
-      }
+  for (const { key, format } of markerSchema.fields) {
+    if (!(key in markerPayload)) {
+      continue;
+    }
 
-      // We're typing the result of the sanitization with `any` because Flow
-      // doesn't like much our enormous enum of non-exact objects that's used as
-      // MarkerPayload type, and this code is too generic for Flow in this context.
-      if (format === 'url') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: removeURLs(markerPayload[key]),
-        }: any);
-      } else if (format === 'file-path') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: removeFilePath(markerPayload[key]),
-        }: any);
-      } else if (format === 'sanitized-string') {
-        markerPayload = ({
-          ...markerPayload,
-          [key]: '<sanitized>',
-        }: any);
-      }
+    // We're typing the result of the sanitization with `any` because Flow
+    // doesn't like much our enormous enum of non-exact objects that's used as
+    // MarkerPayload type, and this code is too generic for Flow in this context.
+    if (format === 'url') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: removeURLs(markerPayload[key]),
+      }: any);
+    } else if (format === 'file-path') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: removeFilePath(markerPayload[key]),
+      }: any);
+    } else if (format === 'sanitized-string') {
+      markerPayload = ({
+        ...markerPayload,
+        [key]: '<sanitized>',
+      }: any);
     }
   }
 

src/profile-logic/marker-schema.js

@@ -39,16 +39,12 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     display: ['marker-table', 'marker-chart'],
     tooltipLabel: 'Jank – event processing delay',
     tableLabel: 'Event processing delay',
-    data: [
-      {
-        label: 'Description',
-        value: oneLine`
-          Jank markers show when the main event loop of a thread has been busy. It is
-          a good indicator that there may be some kind of performance problem that
-          is worth investigating.
-        `,
-      },
-    ],
+    fields: [],
+    description: oneLine`
+      Jank markers show when the main event loop of a thread has been busy. It is
+      a good indicator that there may be some kind of performance problem that
+      is worth investigating.
+    `,
   },
   // Note, these can also come from Gecko, but since we have lots of special handling
   // for IPC, the Gecko ones get overwritten by this definition.
@@ -58,17 +54,12 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     tableLabel: '{marker.data.messageType} — {marker.data.niceDirection}',
     chartLabel: '{marker.data.messageType}',
     display: ['marker-chart', 'marker-table', 'timeline-ipc'],
-    data: [
+    fields: [
       { key: 'messageType', label: 'Type', format: 'string', searchable: true },
       { key: 'sync', label: 'Sync', format: 'string' },
       { key: 'sendThreadName', label: 'From', format: 'string' },
       { key: 'recvThreadName', label: 'To', format: 'string' },
-      {
-        key: 'otherPid',
-        label: 'Other Pid',
-        format: 'pid',
-        searchable: true,
-      },
+      { key: 'otherPid', label: 'Other Pid', format: 'pid', searchable: true },
     ],
   },
   {
@@ -78,7 +69,7 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
     name: 'Network',
     display: ['marker-table', 'marker-chart'],
     chartLabel: '{marker.data.URI}',
-    data: [
+    fields: [
       {
         format: 'string',
         key: 'contentType',
@@ -212,11 +203,9 @@ export function parseLabel(
       // Handle:                                      ^^^^^^^^^^^^^^^^
 
       let format = null;
-      for (const rule of markerSchema.data) {
-        // The rule.value === undefined line is odd mainly because Flow was having trouble
-        // refining the type.
-        if (rule.value === undefined && rule.key === payloadKey) {
-          format = rule.format;
+      for (const field of markerSchema.fields) {
+        if (field.key === payloadKey) {
+          format = field.format;
           break;
         }
       }
@@ -634,19 +623,19 @@ export function markerPayloadMatchesSearch(
   }
 
   // Check if searchable fields match the search regular expression.
-  for (const payloadField of markerSchema.data) {
+  for (const payloadField of markerSchema.fields) {
     if (payloadField.searchable) {
       let value = data[payloadField.key];
+      if (value === undefined || value === null) {
+        // The value is missing, but this is OK, values are optional.
+        continue;
+      }
+
       if (
         payloadField.format === 'unique-string' ||
         payloadField.format === 'flow-id' ||
         payloadField.format === 'terminating-flow-id'
       ) {
-        if (value === undefined) {
-          // The value is missing, but this is OK, values are optional.
-          continue;
-        }
-
         if (typeof value !== 'number') {
           console.warn(
             `In marker ${marker.name}, the key ${payloadField.key} has an invalid value "${value}" as a unique string, it isn't a number.`
@@ -663,11 +652,7 @@ export function markerPayloadMatchesSearch(
         value = stringTable.getString(value);
       }
 
-      if (value === undefined || value === null || value === '') {
-        continue;
-      }
-
-      if (testFun(value, payloadField.key)) {
+      if (value !== '' && testFun(value, payloadField.key)) {
         return true;
       }
     }
@@ -691,9 +676,9 @@ export function computeStringIndexMarkerFieldsByDataType(
   stringIndexMarkerFieldsByDataType.set('CompositorScreenshot', ['url']);
 
   for (const schema of markerSchemas) {
-    const { name, data } = schema;
+    const { name, fields } = schema;
     const stringIndexFields = [];
-    for (const field of data) {
+    for (const field of fields) {
       if (field.format === 'unique-string' && field.key) {
         stringIndexFields.push(field.key);
       }