Bug 187613

Summary: Web Inspector: Basic blocks highlighting should use line/column locations instead of offsets
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, joepeck, saam, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 186453    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Matt Baker 2018-07-12 12:06:50 PDT 
Basic blocks highlighting should use line/column locations instead of offsets.

BasicBlockAnnotator uses start and end offsets from the backend to calculate execution ranges to highlight. Once CodeMirror returns to auto-detecting line endings (see https://webkit.org/b/186453), these ranges won't correspond to the same ranges in CodeMirror.

This isn't a simple matter of using line/column locations from the AST instead of offsets. This may require sending line/column locations from the backend, in which case compatibility with older backends would be an issue.
Comment 1 Radar WebKit Bug Importer 2018-07-12 12:07:02 PDT 
<rdar://problem/42131808>
Comment 2 Matt Baker 2018-07-13 10:46:40 PDT 
Created attachment 344955 [details]
Patch
Comment 3 Matt Baker 2018-07-13 11:08:10 PDT 
Comment on attachment 344955 [details]
Patch

Found a small issue. Fixing.
Comment 4 Matt Baker 2018-07-13 11:16:19 PDT 
Created attachment 344959 [details]
Patch
Comment 5 Joseph Pecoraro 2018-07-13 12:35:07 PDT 
Comment on attachment 344959 [details]
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.
Comment 6 Matt Baker 2018-07-13 14:48:39 PDT 
Created attachment 344986 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-07-13 15:27:38 PDT 
Comment on attachment 344986 [details]
Patch for landing

Clearing flags on attachment: 344986

Committed r233820: <https://trac.webkit.org/changeset/233820>
Comment 8 WebKit Commit Bot 2018-07-13 15:27:39 PDT 
All reviewed patches have been landed.  Closing bug.