Simplify TimelineRecordBar a bit. Its setter is only ever used with multiple records. Require an array instead of dynamically converting inputs.
Created attachment 262301 [details] [PATCH] Proposed Fix
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."
Joe, are you waiting to set commit-queue+ until you think about Matt’s comment? Surprised this is still not landed.
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.
(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.
Comment on attachment 262301 [details] [PATCH] Proposed Fix Clearing flags on attachment: 262301 Committed r190632: <http://trac.webkit.org/changeset/190632>
All reviewed patches have been landed. Closing bug.