[webkit-reviews] review denied: [Bug 192102] Web Inspector: Audit: add "explanation" result value support : [Attachment 355957] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 192102: Web Inspector: Audit: add "explanation" result value support
https://bugs.webkit.org/show_bug.cgi?id=192102

Attachment 355957: Patch

https://bugs.webkit.org/attachment.cgi?id=355957&action=review




--- Comment #12 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 355957
  --> https://bugs.webkit.org/attachment.cgi?id=355957
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.