Bug 149733 – Web Inspector: Simplify TimelineRecordBar a bit

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

[Joseph Pecoraro]

Simplify TimelineRecordBar a bit. Its setter is only ever used with multiple records. Require an array instead of dynamically converting inputs.

[Joseph Pecoraro]

Created attachment 262301 [details]
[PATCH] Proposed Fix

[Matt Baker]

Comment on attachment 262301 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:199
> +        console.assert(records instanceof Array, "records should be an array");

Should we return if !(records instance Array)? What's our policy toward this style of defensive coding?
Nit: I'd format the assert message as a complete sentence: "Records should be an array."

[Darin Adler]

Joe, are you waiting to set commit-queue+ until you think about Matt’s comment? Surprised this is still not landed.

[Joseph Pecoraro]

Comment on attachment 262301 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:199
>> +        console.assert(records instanceof Array, "records should be an array");
> 
> Should we return if !(records instance Array)? What's our policy toward this style of defensive coding?
> Nit: I'd format the assert message as a complete sentence: "Records should be an array."

In this case, our policy should be to use the API correctly. Adding an assert here would just help us catch any possible outliers that I didn't see on auditing the code. Ideally this assert can be removed entirely.

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 262301 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262301&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:199
> >> +        console.assert(records instanceof Array, "records should be an array");
> > 
> > Should we return if !(records instance Array)? What's our policy toward this style of defensive coding?
> > Nit: I'd format the assert message as a complete sentence: "Records should be an array."
> 
> In this case, our policy should be to use the API correctly. Adding an
> assert here would just help us catch any possible outliers that I didn't see
> on auditing the code. Ideally this assert can be removed entirely.

So we'll continue to assert that inputs are valid to catch improper API use, but not attempt to sanitize invalid input or early return. Sounds reasonable.

[WebKit Commit Bot]

Comment on attachment 262301 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 262301

Committed r190632: <http://trac.webkit.org/changeset/190632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.