This string can be used by developers to provide a summary (*cough* explanation *cough*) as to why the result level was chosen.
Created attachment 355918 [details] Patch
Comment on attachment 355918 [details] Patch Attachment 355918 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10187711 New failing tests: inspector/audit/data-domNodes.html inspector/audit/basic.html
Created attachment 355933 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 355918 [details] Patch Attachment 355918 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10187735 New failing tests: inspector/audit/data-domNodes.html inspector/audit/basic.html
Created attachment 355938 [details] Archive of layout-test-results from ews105 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 355939 [details] Patch
Created attachment 355944 [details] Patch
Comment on attachment 355944 [details] Patch Attachment 355944 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10189804 New failing tests: inspector/audit/data-explanation.html
Created attachment 355952 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 355957 [details] Patch
<rdar://problem/46423563>
Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review r- just so that we can better test the quoting function in AuditTestCase.js > Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 > + for (let i = 0; i < this._test.length; ++i) { > + let c = this._test[i]; > + > + let backslashes = 0; > + while (this._test[i - 1 - backslashes] === "\\") > + ++backslashes; 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. Is this substantially different then something like: let escaped = JSON.stringify(str); let test = escaped.substr(1, escaped.length - 2); I suspect it is (especially around template literals) but I just want to make sure. > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:130 > + const linkRegExp = /\[(.+?)\]\((.+?)\)/g; This is a poor-mans Markdown, right? We should have a comment and maybe consider using a markdown library eventually. > Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:145 > + if (backslashes % 2 === 0) { Again with the backslashes, without a high level comment I don't know if this is actually doing what it wants to do, because this is complex.
Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review Considering the number of edge cases I can imagine would arise from escaping things, I think I might want to table this for a bit to rethink how it should be done. >> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 >> + ++backslashes; > > 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. > 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. > 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. > > Is this substantially different then something like: > > let escaped = JSON.stringify(str); > let test = escaped.substr(1, escaped.length - 2); > > I suspect it is (especially around template literals) but I just want to make sure. Due to the fact that we insert `_test` inside an `eval(`(${this._test})`)` when calling `Runtime.evaluate`, I wanted to make sure that all escaped characters remained in their escaped state after being eval'd (e.g. `eval('("foo\"bar")')` vs `eval('("foo\\\"bar")')`). As a side note, the `eval(${test})` should be rewritten as `eval(\`(${test})\`)` so that the test code is treated as a string, not actual code (e.g. someone could have their test code be `false); (function() { window.location = "bad";}` as a form of exploit). This was also addressed in <https://webkit.org/b/193149>. >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:130 >> + const linkRegExp = /\[(.+?)\]\((.+?)\)/g; > > This is a poor-mans Markdown, right? We should have a comment and maybe consider using a markdown library eventually. Yes, very much so. It's just the link syntax from Markdown. >> Source/WebInspectorUI/UserInterface/Views/AuditTestCaseContentView.js:145 >> + if (backslashes % 2 === 0) { > > Again with the backslashes, without a high level comment I don't know if this is actually doing what it wants to do, because this is complex. It's trying to determine whether the character is escaped or not. "\[" is escaped (odd number of '\'), while "\\[" is not (even number of '\').
Comment on attachment 355957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355957&action=review >>> Source/WebInspectorUI/UserInterface/Models/AuditTestCase.js:117 >>> + ++backslashes; >> >> 1. Its unclear to me exactly what this is trying to do and that is not covered in the ChangeLog or with comments. It should probably be in a well named function that can (and should) be tested. >> 2. This seems to affect any test, and just becomes apparent with explanation text. So it sounds separate from adding explanation text an could probably be broken out into its own patch, which would be great cause it could be testable separately. >> 3. This backtracking is potentially slow. If there are 10 in a row for example this code would cause ( N + ((N*(N+1)/2) ) 65 reads. Even though in practice this shouldn't be a problem this would be ingesting user data so we try to protect against a malicious user that just provides a lot of backslashes in a row. Also, the backtracking seems unnecessary. >> >> Is this substantially different then something like: >> >> let escaped = JSON.stringify(str); >> let test = escaped.substr(1, escaped.length - 2); >> >> I suspect it is (especially around template literals) but I just want to make sure. > > Due to the fact that we insert `_test` inside an `eval(`(${this._test})`)` when calling `Runtime.evaluate`, I wanted to make sure that all escaped characters remained in their escaped state after being eval'd (e.g. `eval('("foo\"bar")')` vs `eval('("foo\\\"bar")')`). > > As a side note, the `eval(${test})` should be rewritten as `eval(\`(${test})\`)` so that the test code is treated as a string, not actual code (e.g. someone could have their test code be `false); (function() { window.location = "bad";}` as a form of exploit). This was also addressed in <https://webkit.org/b/193149>. Something I forgot to mention: the code below only tries to add extra escapes for characters that would require an escape due to the characters around them (e.g. "a\"b" should add an extra escape to the middle \" since it's within a "-type group, whereas "a'b" wouldn't need to worry about that). This logic somewhat fails for ` however, in that ${...} code blocks can add other characters that would also need extra escaping (e.g. `a${"b\"c"}d`).