Bug 187613 - Web Inspector: Basic blocks highlighting should use line/column locations instead of offsets
Summary: Web Inspector: Basic blocks highlighting should use line/column locations ins...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P1 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 186453
  Show dependency treegraph
 
Reported: 2018-07-12 12:06 PDT by Matt Baker
Modified: 2018-07-13 15:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.94 KB, patch)
2018-07-13 10:46 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (8.18 KB, patch)
2018-07-13 11:16 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (9.28 KB, patch)
2018-07-13 14:48 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.