Bug 153672 – Web Inspector: Timelines UI redesign: Provide a way to configure which instruments to use

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
Timelines UI redesign: Provide a way to configure which instruments to use.

[Radar WebKit Bug Importer]

<rdar://problem/24417575>

[Timothy Hatcher]

*** Bug 155335 has been marked as a duplicate of this bug. ***

[Matt Baker]

Created attachment 274573 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 274573 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=274573&action=review

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:39
> +        this._enabledInstrumentTypesSetting = new WebInspector.Setting("enabled-instrument-types", WebInspector.TimelineManager.defaultInstrumentTypes());

This default setting, or any picked instrument, could become stale if an instrument is no longer supported or supported in web but not JS contexts. We likely need to cross check the list later when the setting is read. (You might do this already.)

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:56
> +    static defaultInstrumentTypes()

Maybe rename to defailtTimelineTypes. See comment below.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:66
> +            WebInspector.TimelineRecord.Type.Network,

We might want separate identifiers here, since one instrument could support multiple timelines. It also makes the getter confusing, since it is default timeline types, not instrument types. I know right now it is 1-1.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:77
> +    static instrumentTypes()

Rename to availableTimelinesTypes?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:135
> +        return this._enabledInstrumentTypesSetting.value;

This is where you might need to cross check the setting with the current supported types, and remove types that are not support now, ideally without changing the setting.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:60
> +        this._editInstrumentsButton = new WebInspector.ActivateButtonNavigationItem("toggle-edit-instruments", "Edit Configuration", "Save Configuration");

Tooltips are not title case.

[Timothy Hatcher]

Comment on attachment 274573 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=274573&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:39
>> +        this._enabledInstrumentTypesSetting = new WebInspector.Setting("enabled-instrument-types", WebInspector.TimelineManager.defaultInstrumentTypes());
> 
> This default setting, or any picked instrument, could become stale if an instrument is no longer supported or supported in web but not JS contexts. We likely need to cross check the list later when the setting is read. (You might do this already.)

This default setting, or any picked instrument, could become stale if an instrument is no longer supported or supported in web but not JS contexts. We likely need to cross check the list later when the setting is read. (You might do this already.)

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:56
>> +    static defaultInstrumentTypes()
> 
> Maybe rename to defailtTimelineTypes. See comment below.

Maybe rename to defailtTimelineTypes. See comment below.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:66
>> +            WebInspector.TimelineRecord.Type.Network,
> 
> We might want separate identifiers here, since one instrument could support multiple timelines. It also makes the getter confusing, since it is default timeline types, not instrument types. I know right now it is 1-1.

We might want separate identifiers here, since one instrument could support multiple timelines. It also makes the getter confusing, since it is default timeline types, not instrument types. I know right now it is 1-1.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:77
>> +    static instrumentTypes()
> 
> Rename to availableTimelinesTypes?

Rename to availableTimelinesTypes?

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:135
>> +        return this._enabledInstrumentTypesSetting.value;
> 
> This is where you might need to cross check the setting with the current supported types, and remove types that are not support now, ideally without changing the setting.

This is where you might need to cross check the setting with the current supported types, and remove types that are not support now, ideally without changing the setting.

>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:60
>> +        this._editInstrumentsButton = new WebInspector.ActivateButtonNavigationItem("toggle-edit-instruments", "Edit Configuration", "Save Configuration");
> 
> Tooltips are not title case.

Tooltips are not title case.

[Matt Baker]

Created attachment 274603 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 274603 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 274603

Committed r198537: <http://trac.webkit.org/changeset/198537>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.