149733 2015-10-01 17:08:06 -0700 Web Inspector: Simplify TimelineRecordBar a bit 2015-10-06 12:14:21 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED DoNotImportToRadar P2 Normal --- 1 joepeck joepeck bburg commit-queue darin graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1130096 0 joepeck 2015-10-01 17:08:06 -0700 Simplify TimelineRecordBar a bit. Its setter is only ever used with multiple records. Require an array instead of dynamically converting inputs. 1130098 1 262301 joepeck 2015-10-01 17:08:45 -0700 Created attachment 262301 [PATCH] Proposed Fix 1130102 2 262301 mattbaker 2015-10-01 17:17:57 -0700 Comment on attachment 262301 [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." 1130974 3 darin 2015-10-06 09:16:02 -0700 Joe, are you waiting to set commit-queue+ until you think about Matt’s comment? Surprised this is still not landed. 1131046 4 262301 joepeck 2015-10-06 11:27:59 -0700 Comment on attachment 262301 [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. 1131055 5 mattbaker 2015-10-06 11:42:30 -0700 (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. 1131068 6 262301 commit-queue 2015-10-06 12:14:16 -0700 Comment on attachment 262301 [PATCH] Proposed Fix Clearing flags on attachment: 262301 Committed r190632: <http://trac.webkit.org/changeset/190632> 1131069 7 commit-queue 2015-10-06 12:14:21 -0700 All reviewed patches have been landed. Closing bug. 262301 2015-10-01 17:08:45 -0700 2015-10-06 12:14:16 -0700 [PATCH] Proposed Fix simplify-2.patch text/plain 1342 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDc2NmRhM2EuLmZmMDA2NjUgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE1IEBACiAyMDE1LTEwLTAxICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKKyAgICAgICAgV2ViIEluc3BlY3RvcjogU2ltcGxp ZnkgVGltZWxpbmVSZWNvcmRCYXIgYSBiaXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE0OTczMworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9WaWV3cy9UaW1lbGluZVJlY29yZEJh ci5qczoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5UaW1lbGluZVJlY29yZEJhci5wcm90b3R5cGUu c2V0IHJlY29yZHMpOgorCisyMDE1LTEwLTAxICBKb3NlcGggUGVjb3Jhcm8gIDxwZWNvcmFyb0Bh cHBsZS5jb20+CisKICAgICAgICAgV2ViIEluc3BlY3RvcjogU2ltcGxpZnkgZmlsdGVyYWJsZURh dGEgcHJvY2Vzc2luZwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTQ5NzMwCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50 ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lUmVjb3JkQmFyLmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJ L1VzZXJJbnRlcmZhY2UvVmlld3MvVGltZWxpbmVSZWNvcmRCYXIuanMKaW5kZXggZWQ3YjA0OC4u NTU2MGVhOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2Uv Vmlld3MvVGltZWxpbmVSZWNvcmRCYXIuanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1Vz ZXJJbnRlcmZhY2UvVmlld3MvVGltZWxpbmVSZWNvcmRCYXIuanMKQEAgLTE5Niw4ICsxOTYsNyBA QCBXZWJJbnNwZWN0b3IuVGltZWxpbmVSZWNvcmRCYXIgPSBjbGFzcyBUaW1lbGluZVJlY29yZEJh ciBleHRlbmRzIFdlYkluc3BlY3Rvci5PYgogCiAgICAgICAgIHJlY29yZHMgPSByZWNvcmRzIHx8 IFtdOwogCi0gICAgICAgIGlmICghKHJlY29yZHMgaW5zdGFuY2VvZiBBcnJheSkpCi0gICAgICAg ICAgICByZWNvcmRzID0gW3JlY29yZHNdOworICAgICAgICBjb25zb2xlLmFzc2VydChyZWNvcmRz IGluc3RhbmNlb2YgQXJyYXksICJyZWNvcmRzIHNob3VsZCBiZSBhbiBhcnJheSIpOwogCiAgICAg ICAgIHRoaXMuX3JlY29yZHMgPSByZWNvcmRzOwogCg==