From 1bcee49f8ade15e32b61af2b05f5df1d5b5e4a2d Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Mon, 26 Sep 2022 11:47:56 +0200 Subject: [PATCH 1/7] Squashed more types commits --- src/components/sidebar/MarkerSidebar.js | 1 + src/components/tooltip/Marker.js | 15 +- src/profile-logic/marker-schema.js | 122 +++++- .../__snapshots__/marker-schema.test.js.snap | 372 ++++++++++++++++++ src/test/unit/marker-schema.test.js | 52 +++ src/types/markers.js | 15 +- 6 files changed, 572 insertions(+), 5 deletions(-) diff --git a/src/components/sidebar/MarkerSidebar.js b/src/components/sidebar/MarkerSidebar.js index 3117c24952..099ec09010 100644 --- a/src/components/sidebar/MarkerSidebar.js +++ b/src/components/sidebar/MarkerSidebar.js @@ -47,6 +47,7 @@ class MarkerSidebarImpl extends React.PureComponent { marker={marker} threadsKey={selectedThreadsKey} restrictHeightWidth={false} + supportsInteraction={true} /> diff --git a/src/components/tooltip/Marker.js b/src/components/tooltip/Marker.js index f7b49ce383..0587f44c9e 100644 --- a/src/components/tooltip/Marker.js +++ b/src/components/tooltip/Marker.js @@ -35,7 +35,7 @@ import { import { Backtrace } from 'firefox-profiler/components/shared/Backtrace'; import { - formatFromMarkerSchema, + formatDOMFromMarkerSchema, getSchemaFromMarker, } from 'firefox-profiler/profile-logic/marker-schema'; import { computeScreenshotSize } from 'firefox-profiler/profile-logic/marker-data'; @@ -81,6 +81,7 @@ type OwnProps = {| // the layout to be huge. This option when set to true will restrict the // height of things like stacks, and the width of long things like URLs. +restrictHeightWidth: boolean, + +supportsInteraction?: boolean, |}; type StateProps = {| @@ -94,6 +95,7 @@ type StateProps = {| +markerSchemaByName: MarkerSchemaByName, +getMarkerLabel: (MarkerIndex) => string, +categories: CategoryList, + +supportsInteraction: boolean, |}; type Props = ConnectedProps; @@ -222,7 +224,8 @@ class MarkerTooltipContents extends React.PureComponent { * properties that are difficult to represent with the Schema. */ _renderMarkerDetails(): TooltipDetailComponent[] { - const { marker, markerSchemaByName, thread } = this.props; + const { marker, markerSchemaByName, thread, supportsInteraction } = + this.props; const data = marker.data; const details: TooltipDetailComponent[] = []; @@ -245,7 +248,12 @@ class MarkerTooltipContents extends React.PureComponent { key={schema.name + '-' + key} label={label || key} > - {formatFromMarkerSchema(schema.name, format, value)} + {formatDOMFromMarkerSchema( + schema.name, + format, + value, + supportsInteraction + )} ); } @@ -493,6 +501,7 @@ export const TooltipMarker = explicitConnect({ markerSchemaByName: getMarkerSchemaByName(state), getMarkerLabel: selectors.getMarkerTooltipLabelGetter(state), categories: getCategories(state), + supportsInteraction: props.supportsInteraction || false, }; }, component: MarkerTooltipContents, diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 3c28eaa4d0..92af32ea2e 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // @flow +import React from 'react'; import { oneLine } from 'common-tags'; import { formatNumber, @@ -364,6 +365,15 @@ export function formatFromMarkerSchema( format: MarkerFormatType, value: any ): string { + if ( + (value === undefined || value === null) && + format !== 'string' && + format !== 'url' && + format !== 'file-path' + ) { + throw new Error("Can't format undefined or null"); + } + switch (format) { case 'url': case 'file-path': @@ -394,8 +404,118 @@ export function formatFromMarkerSchema( return formatPercent(value); default: console.error( - `A marker schema of type "${markerType}" had an unknown format "${(format: empty)}"` + `A marker schema of type "${markerType}" had an unknown format ${JSON.stringify( + format + )}` ); return value; } } + +export function isDOMRenderingFormat(format: MarkerFormatType): boolean { + return format === 'url' || typeof format === 'object' || format === 'list'; +} + +function _formatDOMFromMarkerSchema( + markerType: string, + format: MarkerFormatType, + value: any, + // add elements that allow interaction (like clickable urls) + supportsInteraction: boolean +) { + if (!isDOMRenderingFormat(format)) { + return <>{formatFromMarkerSchema(markerType, format, value)}; + } + if (typeof format === 'object') { + switch (format.type) { + case 'table': { + const { columns } = format; + if (!(value instanceof Array)) { + throw new Error('Expected an array for table type'); + } + const hasHeader = columns.some((column) => column.label); + return ( + + {hasHeader ? ( + + {columns.map((col, i) => ( + + ))} + + ) : null} + {value.map((row, i) => { + if (!(row instanceof Array)) { + throw new Error('Expected an array for table row'); + } + + if (row.length !== columns.length) { + throw new Error( + "Row length doesn't match column count (row: " + + row.length + + ', cols: ' + + columns.length + + ')' + ); + } + return ( + + {row.map((cell, i) => { + return ( + + ); + })} + + ); + })} +
{col.label || ''}
+ {_formatDOMFromMarkerSchema( + markerType, + columns[i].type || 'string', + cell, + supportsInteraction + )} +
+ ); + } + default: + throw new Error(`Unknown format type ${JSON.stringify(format)}`); + } + } + switch (format) { + case 'list': + if (!(value instanceof Array)) { + throw new Error('Expected an array for list format'); + } + return value.map((entry, i) => ( +
  • + {formatFromMarkerSchema(markerType, 'string', value[i])} +
    • + )); + case 'url': + if (supportsInteraction) { + return ( + + {formatFromMarkerSchema(markerType, 'string', value)} + + ); + } + return <>value; + default: + throw new Error(`Unknown format type ${JSON.stringify(format)}`); + } +} + +export function formatDOMFromMarkerSchema( + markerType: string, + format: MarkerFormatType, + value: any, + // add elements that allow interaction (like clickable urls) + supportsInteraction: boolean +) { + return _formatDOMFromMarkerSchema( + markerType, + format, + value, + supportsInteraction + ); +} diff --git a/src/test/unit/__snapshots__/marker-schema.test.js.snap b/src/test/unit/__snapshots__/marker-schema.test.js.snap index 760592ac67..3a3b589323 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.js.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.js.snap @@ -1,5 +1,377 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`marker schema formatting supports complex formats 1`] = ` +Array [ + Array [ + "url", + "http://example.com", + + value + , + + http://example.com + , + ], + Array [ + "file-path", + "/Users/me/gecko", + + /Users/me/gecko + , + + /Users/me/gecko + , + ], + Array [ + "file-path", + null, + + (empty) + , + + (empty) + , + ], + Array [ + "file-path", + undefined, + + (empty) + , + + (empty) + , + ], + Array [ + "duration", + 0, + + 0s + , + + 0s + , + ], + Array [ + "duration", + 10, + + 10ms + , + + 10ms + , + ], + Array [ + "duration", + 12.3456789, + + 12.346ms + , + + 12.346ms + , + ], + Array [ + Object { + "columns": Array [ + Object { + "type": "string", + }, + Object { + "type": "integer", + }, + ], + "type": "table", + }, + Array [ + Array [ + "a", + 1, + ], + Array [ + "b", + 2, + ], + ], + + + + + + + + + +
    + + a + + + + 1 + +
    + + b + + + + 2 + +
    , + + + + + + + + + +
    + + a + + + + 1 + +
    + + b + + + + 2 + +
    , + ], + Array [ + Object { + "columns": Array [ + Object { + "label": "a", + "type": "string", + }, + Object { + "label": "b", + "type": "integer", + }, + ], + "type": "table", + }, + Array [ + Array [ + "b", + 2, + ], + ], + + + + + + + + + +
    + a + + b +
    + + b + + + + 2 + +
    , + + + + + + + + + +
    + a + + b +
    + + b + + + + 2 + +
    , + ], + Array [ + Object { + "columns": Array [ + Object { + "label": "a", + "type": "string", + }, + Object { + "type": "integer", + }, + ], + "type": "table", + }, + Array [ + Array [ + "b", + 2, + ], + ], + + + + + + + + + +
    + a + + +
    + + b + + + + 2 + +
    , + + + + + + + + + +
    + a + + +
    + + b + + + + 2 + +
    , + ], + Array [ + Object { + "columns": Array [ + Object { + "label": "a", + "type": "string", + }, + Object {}, + ], + "type": "table", + }, + Array [ + Array [ + "b", + 2, + ], + ], + + + + + + + + + +
    + a + + +
    + + b + + + + 2 + +
    , + + + + + + + + + +
    + a + + +
    + + b + + + + 2 + +
    , + ], + Array [ + "list", + Array [], + Array [], + Array [], + ], + Array [ + "list", + Array [ + "a", + "b", + ], + Array [ +
  • + a +
    • , +
  • + b +
    • , + ], + Array [ +
  • + a +
    • , +
  • + b +
    • , + ], + ], +] +`; + exports[`marker schema labels parseErrors errors if looking up into a part of the marker that does not exist 1`] = ` Array [ Array [ diff --git a/src/test/unit/marker-schema.test.js b/src/test/unit/marker-schema.test.js index f92974a982..ec06d1278e 100644 --- a/src/test/unit/marker-schema.test.js +++ b/src/test/unit/marker-schema.test.js @@ -4,6 +4,7 @@ // @flow import { formatFromMarkerSchema, + formatDOMFromMarkerSchema, parseLabel, markerSchemaFrontEndOnly, } from '../../profile-logic/marker-schema'; @@ -291,6 +292,57 @@ describe('marker schema formatting', function () { ] `); }); + + it('supports complex formats', function () { + const entries = [ + ['url', 'http://example.com'], + ['file-path', '/Users/me/gecko'], + ['file-path', null], + ['file-path', undefined], + ['duration', 0], + ['duration', 10], + ['duration', 12.3456789], + [ + { type: 'table', columns: [{ type: 'string' }, { type: 'integer' }] }, + [ + ['a', 1], + ['b', 2], + ], + ], + [ + { + type: 'table', + columns: [ + { type: 'string', label: 'a' }, + { type: 'integer', label: 'b' }, + ], + }, + [['b', 2]], + ], + [ + { + type: 'table', + columns: [{ type: 'string', label: 'a' }, { type: 'integer' }], + }, + [['b', 2]], + ], + [ + { type: 'table', columns: [{ type: 'string', label: 'a' }, {}] }, + [['b', 2]], + ], + ['list', []], + ['list', ['a', 'b']], + ]; + + expect( + entries.map(([format, value]) => [ + format, + value, + formatDOMFromMarkerSchema('none', format, value, false), + formatDOMFromMarkerSchema('none', format, value, true), + ]) + ).toMatchSnapshot(); + }); }); describe('getMarkerSchema', function () { diff --git a/src/types/markers.js b/src/types/markers.js index ef50242eeb..f137af7c27 100644 --- a/src/types/markers.js +++ b/src/types/markers.js @@ -14,7 +14,9 @@ import type { import type { ObjectMap } from './utils'; // Provide different formatting options for strings. -export type MarkerFormatType = +export type MarkerFormatType = BasicMarkerFormatType | ComplexMarkerFormatType; + +export type BasicMarkerFormatType = // ---------------------------------------------------- // String types. @@ -60,6 +62,17 @@ export type MarkerFormatType = // "Label: 52.23, 0.0054, 123,456.78" | 'decimal'; +type ComplexMarkerFormatType = + | 'list' + | {| type: 'table', columns: TableColumnFormat[] |}; + +type TableColumnFormat = {| + // type for formatting, default is string + type?: BasicMarkerFormatType, + // header column label + label?: string, +|}; + // A list of all the valid locations to surface this marker. // We can be free to add more UI areas. export type MarkerDisplayLocation = From 15918211e65e91e643bf3e1cc8332269dd1e22e6 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Mon, 26 Sep 2022 12:48:21 +0200 Subject: [PATCH 2/7] Improve formatting new types --- src/components/sidebar/MarkerSidebar.js | 1 - src/components/sidebar/sidebar.css | 10 + src/components/tooltip/Marker.js | 15 +- src/profile-logic/marker-schema.js | 153 ++++----- .../__snapshots__/marker-schema.test.js.snap | 319 ++++++------------ src/test/unit/marker-schema.test.js | 5 +- 6 files changed, 183 insertions(+), 320 deletions(-) diff --git a/src/components/sidebar/MarkerSidebar.js b/src/components/sidebar/MarkerSidebar.js index 099ec09010..3117c24952 100644 --- a/src/components/sidebar/MarkerSidebar.js +++ b/src/components/sidebar/MarkerSidebar.js @@ -47,7 +47,6 @@ class MarkerSidebarImpl extends React.PureComponent { marker={marker} threadsKey={selectedThreadsKey} restrictHeightWidth={false} - supportsInteraction={true} /> diff --git a/src/components/sidebar/sidebar.css b/src/components/sidebar/sidebar.css index c86c8fecc1..3589ee1bb7 100644 --- a/src/components/sidebar/sidebar.css +++ b/src/components/sidebar/sidebar.css @@ -187,3 +187,13 @@ margin-top: 3px; margin-inline-start: -12px; } + +.sidebar ul.marker-value { + padding-left: 25px; + margin-top: 5px; + margin-bottom: 5px; +} + +.sidebar table.marker-value th { + text-align: left; +} diff --git a/src/components/tooltip/Marker.js b/src/components/tooltip/Marker.js index 0587f44c9e..28dd93e3e0 100644 --- a/src/components/tooltip/Marker.js +++ b/src/components/tooltip/Marker.js @@ -35,7 +35,7 @@ import { import { Backtrace } from 'firefox-profiler/components/shared/Backtrace'; import { - formatDOMFromMarkerSchema, + formatMarkupFromMarkerSchema, getSchemaFromMarker, } from 'firefox-profiler/profile-logic/marker-schema'; import { computeScreenshotSize } from 'firefox-profiler/profile-logic/marker-data'; @@ -81,7 +81,6 @@ type OwnProps = {| // the layout to be huge. This option when set to true will restrict the // height of things like stacks, and the width of long things like URLs. +restrictHeightWidth: boolean, - +supportsInteraction?: boolean, |}; type StateProps = {| @@ -95,7 +94,6 @@ type StateProps = {| +markerSchemaByName: MarkerSchemaByName, +getMarkerLabel: (MarkerIndex) => string, +categories: CategoryList, - +supportsInteraction: boolean, |}; type Props = ConnectedProps; @@ -224,8 +222,7 @@ class MarkerTooltipContents extends React.PureComponent { * properties that are difficult to represent with the Schema. */ _renderMarkerDetails(): TooltipDetailComponent[] { - const { marker, markerSchemaByName, thread, supportsInteraction } = - this.props; + const { marker, markerSchemaByName, thread } = this.props; const data = marker.data; const details: TooltipDetailComponent[] = []; @@ -248,12 +245,7 @@ class MarkerTooltipContents extends React.PureComponent { key={schema.name + '-' + key} label={label || key} > - {formatDOMFromMarkerSchema( - schema.name, - format, - value, - supportsInteraction - )} + {formatMarkupFromMarkerSchema(schema.name, format, value)} ); } @@ -501,7 +493,6 @@ export const TooltipMarker = explicitConnect({ markerSchemaByName: getMarkerSchemaByName(state), getMarkerLabel: selectors.getMarkerTooltipLabelGetter(state), categories: getCategories(state), - supportsInteraction: props.supportsInteraction || false, }; }, component: MarkerTooltipContents, diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 92af32ea2e..61427b3614 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // @flow -import React from 'react'; +import * as React from 'react'; import { oneLine } from 'common-tags'; import { formatNumber, @@ -365,23 +365,16 @@ export function formatFromMarkerSchema( format: MarkerFormatType, value: any ): string { - if ( - (value === undefined || value === null) && - format !== 'string' && - format !== 'url' && - format !== 'file-path' - ) { - throw new Error("Can't format undefined or null"); + if (value === undefined || value === null) { + console.warn(`Formatting ${value} for ${markerType}`); + return '(empty)'; } - switch (format) { case 'url': case 'file-path': case 'string': // Make sure a non-empty string is returned here. - if (value === undefined || value === null) { - return '(empty)'; - } + return String(value) || '(empty)'; case 'duration': case 'time': @@ -416,15 +409,17 @@ export function isDOMRenderingFormat(format: MarkerFormatType): boolean { return format === 'url' || typeof format === 'object' || format === 'list'; } -function _formatDOMFromMarkerSchema( +export function formatMarkupFromMarkerSchema( markerType: string, format: MarkerFormatType, - value: any, - // add elements that allow interaction (like clickable urls) - supportsInteraction: boolean -) { + value: any +): React.Element | string { + if (value === undefined || value === null) { + console.warn(`Formatting ${value} for ${JSON.stringify(markerType)}`); + return '(empty)'; + } if (!isDOMRenderingFormat(format)) { - return <>{formatFromMarkerSchema(markerType, format, value)}; + return formatFromMarkerSchema(markerType, format, value); } if (typeof format === 'object') { switch (format.type) { @@ -435,45 +430,48 @@ function _formatDOMFromMarkerSchema( } const hasHeader = columns.some((column) => column.label); return ( - +
    {hasHeader ? ( - - {columns.map((col, i) => ( - - ))} - + + + {columns.map((col, i) => ( + + ))} + + ) : null} - {value.map((row, i) => { - if (!(row instanceof Array)) { - throw new Error('Expected an array for table row'); - } - - if (row.length !== columns.length) { - throw new Error( - "Row length doesn't match column count (row: " + - row.length + - ', cols: ' + - columns.length + - ')' + + {value.map((row, i) => { + if (!(row instanceof Array)) { + throw new Error('Expected an array for table row'); + } + + if (row.length !== columns.length) { + throw new Error( + "Row length doesn't match column count (row: " + + row.length + + ', cols: ' + + columns.length + + ')' + ); + } + return ( + + {row.map((cell, i) => { + return ( + + ); + })} + ); - } - return ( - - {row.map((cell, i) => { - return ( - - ); - })} - - ); - })} + })} +
    {col.label || ''}
    {col.label || ''}
    + {formatMarkupFromMarkerSchema( + markerType, + columns[i].type || 'string', + cell + )} +
    - {_formatDOMFromMarkerSchema( - markerType, - columns[i].type || 'string', - cell, - supportsInteraction - )} -
    ); } @@ -486,36 +484,27 @@ function _formatDOMFromMarkerSchema( if (!(value instanceof Array)) { throw new Error('Expected an array for list format'); } - return value.map((entry, i) => ( -
  • - {formatFromMarkerSchema(markerType, 'string', value[i])} -
    • - )); + return ( +
      + {value.map((entry, i) => ( +
    • + {formatFromMarkerSchema(markerType, 'string', value[i])} +
      • + ))} +
    + ); case 'url': - if (supportsInteraction) { - return ( - - {formatFromMarkerSchema(markerType, 'string', value)} - - ); - } - return <>value; + return ( + + {formatFromMarkerSchema(markerType, 'string', value)} + + ); default: throw new Error(`Unknown format type ${JSON.stringify(format)}`); } } - -export function formatDOMFromMarkerSchema( - markerType: string, - format: MarkerFormatType, - value: any, - // add elements that allow interaction (like clickable urls) - supportsInteraction: boolean -) { - return _formatDOMFromMarkerSchema( - markerType, - format, - value, - supportsInteraction - ); -} diff --git a/src/test/unit/__snapshots__/marker-schema.test.js.snap b/src/test/unit/__snapshots__/marker-schema.test.js.snap index 3a3b589323..c4032411f2 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.js.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.js.snap @@ -5,11 +5,11 @@ Array [ Array [ "url", "http://example.com", - - value - , http://example.com , @@ -17,62 +17,32 @@ Array [ Array [ "file-path", "/Users/me/gecko", - - /Users/me/gecko - , - - /Users/me/gecko - , + "/Users/me/gecko", ], Array [ "file-path", null, - - (empty) - , - - (empty) - , + "(empty)", ], Array [ "file-path", undefined, - - (empty) - , - - (empty) - , + "(empty)", ], Array [ "duration", 0, - - 0s - , - - 0s - , + "0s", ], Array [ "duration", 10, - - 10ms - , - - 10ms - , + "10ms", ], Array [ "duration", 12.3456789, - - 12.346ms - , - - 12.346ms - , + "12.346ms", ], Array [ Object { @@ -96,57 +66,27 @@ Array [ 2, ], ], - - - - - - - - - -
    - - a - - - - 1 - -
    - - b - - - - 2 - -
    , - - -
    - + + + + - + - - - + + + - + - + + +
    a - - - + 1 - -
    - +
    b - - - + 2 - -
    , ], Array [ @@ -169,49 +109,29 @@ Array [ 2, ], ], - - - - - - -
    - a - - b -
    - + + + + + - -
    + a + b - - - - - 2 - -
    , - - - - - - - + + + + - + - + + +
    - a - - b -
    - + +
    b - - - + 2 - -
    , ], Array [ @@ -233,49 +153,29 @@ Array [ 2, ], ], - - - - - - - - - -
    - a - - -
    - - b - - - - 2 - -
    , - - - - - - -
    - a - - -
    - + + + + + + + + + + - + - + + +
    + a + + +
    b - - - + 2 - -
    , ], Array [ @@ -295,56 +195,37 @@ Array [ 2, ], ], - - - - - - - - - -
    - a - - -
    - - b - - - - 2 - -
    , - - - - - - -
    - a - - -
    - + + + + + + + + + + - + - + + +
    + a + + +
    b - - - + 2 - -
    , ], Array [ "list", Array [], - Array [], - Array [], +
      , ], Array [ "list", @@ -352,22 +233,16 @@ Array [ "a", "b", ], - Array [ -
    • - a -
      • , -
    • - b -
      • , - ], - Array [ +
      • a -
        • , +
      • b -
        • , - ], + +
      , ], ] `; diff --git a/src/test/unit/marker-schema.test.js b/src/test/unit/marker-schema.test.js index ec06d1278e..4a34cfb459 100644 --- a/src/test/unit/marker-schema.test.js +++ b/src/test/unit/marker-schema.test.js @@ -4,7 +4,7 @@ // @flow import { formatFromMarkerSchema, - formatDOMFromMarkerSchema, + formatMarkupFromMarkerSchema, parseLabel, markerSchemaFrontEndOnly, } from '../../profile-logic/marker-schema'; @@ -338,8 +338,7 @@ describe('marker schema formatting', function () { entries.map(([format, value]) => [ format, value, - formatDOMFromMarkerSchema('none', format, value, false), - formatDOMFromMarkerSchema('none', format, value, true), + formatMarkupFromMarkerSchema('none', format, value), ]) ).toMatchSnapshot(); }); From 66974c73cb2a3911d10bb3c119abce9dadb2e0ef Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Mon, 26 Sep 2022 13:06:17 +0200 Subject: [PATCH 3/7] Small fixes --- src/profile-logic/marker-schema.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 61427b3614..b2d4811c5d 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -405,10 +405,6 @@ export function formatFromMarkerSchema( } } -export function isDOMRenderingFormat(format: MarkerFormatType): boolean { - return format === 'url' || typeof format === 'object' || format === 'list'; -} - export function formatMarkupFromMarkerSchema( markerType: string, format: MarkerFormatType, @@ -418,7 +414,7 @@ export function formatMarkupFromMarkerSchema( console.warn(`Formatting ${value} for ${JSON.stringify(markerType)}`); return '(empty)'; } - if (!isDOMRenderingFormat(format)) { + if (format !== 'url' && typeof format !== 'object' && format !== 'list') { return formatFromMarkerSchema(markerType, format, value); } if (typeof format === 'object') { From dc47a7472c8756f517aa9e91a7285b5fafed9276 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Mon, 26 Sep 2022 18:36:38 +0200 Subject: [PATCH 4/7] Support list and table in formatFromMarkerSchema --- src/profile-logic/marker-schema.js | 53 +++++++++++++++++-- .../__snapshots__/marker-schema.test.js.snap | 13 +++++ src/test/unit/marker-schema.test.js | 6 ++- src/types/markers.js | 9 ++-- 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index b2d4811c5d..27a894631e 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -366,15 +366,55 @@ export function formatFromMarkerSchema( value: any ): string { if (value === undefined || value === null) { - console.warn(`Formatting ${value} for ${markerType}`); + console.warn( + `Formatting ${value} for ${markerType} with format ${JSON.stringify( + format + )}` + ); return '(empty)'; } + if (typeof format === 'object') { + switch (format.type) { + case 'table': { + const { columns } = format; + if (!(value instanceof Array)) { + throw new Error('Expected an array for table type'); + } + const hasHeader = columns.some((column) => column.label); + const headerRows = hasHeader + ? [columns.map((x) => x.label || '(empty)')] + : []; + const cellRows = value.map((row) => { + if (!(row instanceof Array)) { + throw new Error('Expected an array for table row'); + } + + if (row.length !== columns.length) { + throw new Error( + "Row length doesn't match column count (row: " + + row.length + + ', cols: ' + + columns.length + + ')' + ); + } + return row.map((cell, j) => { + const { format } = columns[j]; + return formatFromMarkerSchema(markerType, format || 'string', cell); + }); + }); + const rows = headerRows.concat(cellRows); + return rows.map((r) => `(${r.join(', ')})`).join(','); + } + default: + throw new Error(`Unknown format type ${JSON.stringify(format)}`); + } + } switch (format) { case 'url': case 'file-path': case 'string': // Make sure a non-empty string is returned here. - return String(value) || '(empty)'; case 'duration': case 'time': @@ -395,8 +435,15 @@ export function formatFromMarkerSchema( return formatNumber(value); case 'percentage': return formatPercent(value); + case 'list': + if (!(value instanceof Array)) { + throw new Error('Expected an array for list format'); + } + return value + .map((v) => formatFromMarkerSchema(markerType, 'string', v)) + .join(', '); default: - console.error( + console.warn( `A marker schema of type "${markerType}" had an unknown format ${JSON.stringify( format )}` diff --git a/src/test/unit/__snapshots__/marker-schema.test.js.snap b/src/test/unit/__snapshots__/marker-schema.test.js.snap index c4032411f2..ac36ad9651 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.js.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.js.snap @@ -13,36 +13,43 @@ Array [ > http://example.com , + "http://example.com", ], Array [ "file-path", "/Users/me/gecko", "/Users/me/gecko", + "/Users/me/gecko", ], Array [ "file-path", null, "(empty)", + "(empty)", ], Array [ "file-path", undefined, "(empty)", + "(empty)", ], Array [ "duration", 0, "0s", + "0s", ], Array [ "duration", 10, "10ms", + "10ms", ], Array [ "duration", 12.3456789, "12.346ms", + "12.346ms", ], Array [ Object { @@ -88,6 +95,7 @@ Array [
    , + "(a, 1),(b, 2)", ], Array [ Object { @@ -133,6 +141,7 @@ Array [
    , + "(a, b),(b, 2)", ], Array [ Object { @@ -177,6 +186,7 @@ Array [
    , + "(a, (empty)),(b, 2)", ], Array [ Object { @@ -219,6 +229,7 @@ Array [
    , + "(a, (empty)),(b, 2)", ], Array [ "list", @@ -226,6 +237,7 @@ Array [
      , + "", ], Array [ "list", @@ -243,6 +255,7 @@ Array [ b
    , + "a, b", ], ] `; diff --git a/src/test/unit/marker-schema.test.js b/src/test/unit/marker-schema.test.js index 4a34cfb459..a2721048d1 100644 --- a/src/test/unit/marker-schema.test.js +++ b/src/test/unit/marker-schema.test.js @@ -178,6 +178,10 @@ describe('marker schema labels', function () { }); describe('marker schema formatting', function () { + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(() => {}); + }); + it('can apply a variety of formats', function () { const entries = [ ['url', 'http://example.com'], @@ -333,12 +337,12 @@ describe('marker schema formatting', function () { ['list', []], ['list', ['a', 'b']], ]; - expect( entries.map(([format, value]) => [ format, value, formatMarkupFromMarkerSchema('none', format, value), + formatFromMarkerSchema('none', format, value), ]) ).toMatchSnapshot(); }); diff --git a/src/types/markers.js b/src/types/markers.js index f137af7c27..0fc0c48185 100644 --- a/src/types/markers.js +++ b/src/types/markers.js @@ -14,9 +14,8 @@ import type { import type { ObjectMap } from './utils'; // Provide different formatting options for strings. -export type MarkerFormatType = BasicMarkerFormatType | ComplexMarkerFormatType; -export type BasicMarkerFormatType = +export type MarkerFormatType = // ---------------------------------------------------- // String types. @@ -60,15 +59,13 @@ export type BasicMarkerFormatType = // The decimal should be used for generic representations of numbers. Do not // use it for time information. // "Label: 52.23, 0.0054, 123,456.78" - | 'decimal'; - -type ComplexMarkerFormatType = + | 'decimal' | 'list' | {| type: 'table', columns: TableColumnFormat[] |}; type TableColumnFormat = {| // type for formatting, default is string - type?: BasicMarkerFormatType, + type?: MarkerFormatType, // header column label label?: string, |}; From e1ee179d981da800e4192cc896b72aacd5f225bd Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 29 Sep 2022 17:16:21 +0200 Subject: [PATCH 5/7] Small changes to make the code clearer and more maintainable --- src/profile-logic/marker-schema.js | 47 ++++++++++++++++++------------ 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index 27a894631e..e9eeee2d0e 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -360,6 +360,11 @@ export function getLabelGetter( }; } +/** + * This function formats a string from a marker type and a value. + * If you wish to get markup instead, have a look at + * formatMarkupFromMarkerSchema below. + */ export function formatFromMarkerSchema( markerType: string, format: MarkerFormatType, @@ -381,21 +386,17 @@ export function formatFromMarkerSchema( throw new Error('Expected an array for table type'); } const hasHeader = columns.some((column) => column.label); - const headerRows = hasHeader + const rows = hasHeader ? [columns.map((x) => x.label || '(empty)')] : []; - const cellRows = value.map((row) => { + const cellRows = value.map((row, i) => { if (!(row instanceof Array)) { throw new Error('Expected an array for table row'); } if (row.length !== columns.length) { throw new Error( - "Row length doesn't match column count (row: " + - row.length + - ', cols: ' + - columns.length + - ')' + `Row ${i} length doesn't match column count (row: ${row.length}, cols: ${columns.length})` ); } return row.map((cell, j) => { @@ -403,11 +404,13 @@ export function formatFromMarkerSchema( return formatFromMarkerSchema(markerType, format || 'string', cell); }); }); - const rows = headerRows.concat(cellRows); - return rows.map((r) => `(${r.join(', ')})`).join(','); + rows.push(...cellRows); + return rows.map((row) => `(${row.join(', ')})`).join(','); } default: - throw new Error(`Unknown format type ${JSON.stringify(format)}`); + throw new Error( + `Unknown format type ${JSON.stringify((format.type: empty))}` + ); } } switch (format) { @@ -445,13 +448,18 @@ export function formatFromMarkerSchema( default: console.warn( `A marker schema of type "${markerType}" had an unknown format ${JSON.stringify( - format + (format: empty) )}` ); return value; } } +/** + * This function may return structured markup for some types suchs as table, + * list, or urls. For other types this falls back to formatFromMarkerSchema + * above. + */ export function formatMarkupFromMarkerSchema( markerType: string, format: MarkerFormatType, @@ -491,11 +499,7 @@ export function formatMarkupFromMarkerSchema( if (row.length !== columns.length) { throw new Error( - "Row length doesn't match column count (row: " + - row.length + - ', cols: ' + - columns.length + - ')' + `Row ${i} length doesn't match column count (row: ${row.length}, cols: ${columns.length})` ); } return ( @@ -519,7 +523,9 @@ export function formatMarkupFromMarkerSchema( ); } default: - throw new Error(`Unknown format type ${JSON.stringify(format)}`); + throw new Error( + `Unknown format type ${JSON.stringify((format: empty))}` + ); } } switch (format) { @@ -537,6 +543,9 @@ export function formatMarkupFromMarkerSchema( ); case 'url': + if (!value.startsWith('http:') && !value.startsWith('https:')) { + return value; + } return ( - {formatFromMarkerSchema(markerType, 'string', value)} + {value} ); default: - throw new Error(`Unknown format type ${JSON.stringify(format)}`); + throw new Error(`Unknown format type ${JSON.stringify((format: empty))}`); } } From c8a30d360796bb8a493cc854e59b72dfbcc058f8 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 29 Sep 2022 17:35:45 +0200 Subject: [PATCH 6/7] Adjust CSS to make it more maintainable, RTL-friendly, and apply to the tooltip as well as the sidebar --- src/components/sidebar/sidebar.css | 10 ---------- src/components/tooltip/Marker.css | 18 ++++++++++++++++++ src/components/tooltip/Marker.js | 2 ++ src/profile-logic/marker-schema.js | 6 +++--- .../__snapshots__/marker-schema.test.js.snap | 14 +++++++------- 5 files changed, 30 insertions(+), 20 deletions(-) create mode 100644 src/components/tooltip/Marker.css diff --git a/src/components/sidebar/sidebar.css b/src/components/sidebar/sidebar.css index 3589ee1bb7..c86c8fecc1 100644 --- a/src/components/sidebar/sidebar.css +++ b/src/components/sidebar/sidebar.css @@ -187,13 +187,3 @@ margin-top: 3px; margin-inline-start: -12px; } - -.sidebar ul.marker-value { - padding-left: 25px; - margin-top: 5px; - margin-bottom: 5px; -} - -.sidebar table.marker-value th { - text-align: left; -} diff --git a/src/components/tooltip/Marker.css b/src/components/tooltip/Marker.css new file mode 100644 index 0000000000..ccbcfad6ad --- /dev/null +++ b/src/components/tooltip/Marker.css @@ -0,0 +1,18 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +.marker-list-value { + margin: 0; + padding-inline-start: 25px; +} + +/* These styles are useful to make the table well-aligned with its labels in tooltips. */ +.marker-table-value { + border-collapse: collapse; + margin-block-start: -1px; +} + +.marker-table-value th { + text-align: start; +} diff --git a/src/components/tooltip/Marker.js b/src/components/tooltip/Marker.js index 28dd93e3e0..5a769a7a97 100644 --- a/src/components/tooltip/Marker.js +++ b/src/components/tooltip/Marker.js @@ -62,6 +62,8 @@ import { getGCSliceDetails, } from './GCMarker'; +import './Marker.css'; + function _maybeFormatDuration( start: number | void, end: number | void diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index e9eeee2d0e..9966b60f08 100644 --- a/src/profile-logic/marker-schema.js +++ b/src/profile-logic/marker-schema.js @@ -481,7 +481,7 @@ export function formatMarkupFromMarkerSchema( } const hasHeader = columns.some((column) => column.label); return ( - +
    {hasHeader ? ( @@ -534,7 +534,7 @@ export function formatMarkupFromMarkerSchema( throw new Error('Expected an array for list format'); } return ( -
      +
        {value.map((entry, i) => (
      • {formatFromMarkerSchema(markerType, 'string', value[i])} @@ -551,7 +551,7 @@ export function formatMarkupFromMarkerSchema( href={value} target="_blank" rel="noreferrer" - className="marker-value" + className="marker-link-value" > {value} diff --git a/src/test/unit/__snapshots__/marker-schema.test.js.snap b/src/test/unit/__snapshots__/marker-schema.test.js.snap index ac36ad9651..786ecf3c6c 100644 --- a/src/test/unit/__snapshots__/marker-schema.test.js.snap +++ b/src/test/unit/__snapshots__/marker-schema.test.js.snap @@ -6,7 +6,7 @@ Array [ "url", "http://example.com",
    @@ -118,7 +118,7 @@ Array [ ], ],
    @@ -163,7 +163,7 @@ Array [ ], ],
    @@ -206,7 +206,7 @@ Array [ ], ],
    @@ -235,7 +235,7 @@ Array [ "list", Array [],