[webkit-reviews] review granted: [Bug 187613] Web Inspector: Basic blocks highlighting should use line/column locations instead of offsets : [Attachment 344959] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 187613: Web Inspector: Basic blocks highlighting should use line/column
locations instead of offsets
https://bugs.webkit.org/show_bug.cgi?id=187613

Attachment 344959: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 344959
  --> https://bugs.webkit.org/attachment.cgi?id=344959
Patch

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

r=me if the formatted case continues to behave as we expect (which I suspect it
will given

> Source/WebInspectorUI/UserInterface/Controllers/BasicBlockAnnotator.js:62
> +	   let lineEndings = [];
> +	   let lineEndingLengths = [];
> +	   let pattern = /\r\n?|\n/g;
> +	   let match = pattern.exec(content);

This reminds me that we have `String.prototype.lineEndings` in
FormatterUtilities.js, which only uses "\n" and not "\r\n". Lets double check
that things behave as expected in a pretty printed resource.

My test for that is typically:
1. Inspect http://bogojoker.com/shell/
2. Resources > easySlider.min.js
3. Set a breakpoint on Line 56:21 (the first line inside $next.click callback
4. Click the down arrow on the page
5. Step through code (step all around the code include the calling code which
is jquery and ensure ranges are correct)

Shouldn't matter for the majority of cases (\r\n and \n) end in \n. It seems we
might only have differences in \r only eliminated content.