Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

WebAssembly text generation on debugger side#3238

Merged
jasonLaster merged 4 commits into
firefox-devtools:nextfrom
yurydelendik:wasm-sourcemaps
Jul 20, 2017
Merged

WebAssembly text generation on debugger side#3238
jasonLaster merged 4 commits into
firefox-devtools:nextfrom
yurydelendik:wasm-sourcemaps

Conversation

@yurydelendik

Copy link
Copy Markdown
Contributor

Associated Issue: n/a

Summary of Changes

  • adds code generation for editor
  • better source-to-editor text line mapping

Test Plan

Example test plan (for original source):

  1. Open https://devtools-html.github.io/debugger-examples/examples/wasm/fib/fib.index.html
  2. Open debugger and refresh fib.index.html page (the latter is to enable wasm debugging)
  3. Find/open "fib.c"
  4. Set breakpoint at line 4
  5. Refresh refresh fib.index.html page

Alternative test plan (for wasm):

3a. Find/open "wasm:https://devtools-html.github.io/debugger-examples/examples/wasm/fib/fib.index.html:d65901927dbe8357"
4a. Set breakpoint at the beginnig of $func5 @0000027D
5a. Refresh refresh fib.index.html page

Expected results: stop at set breakpoint(s).

Some other defects found:

  • unable to set breakpoints in step 4a
  • when trying to step through in original source (after step 5), it take multiple step commands to pass to the next line.

@codecov

codecov Bot commented Jul 11, 2017

Copy link
Copy Markdown

Codecov Report

Merging #3238 into next will decrease coverage by 0.38%.
The diff coverage is 28.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #3238      +/-   ##
==========================================
- Coverage   50.41%   50.03%   -0.39%     
==========================================
  Files         106      107       +1     
  Lines        4336     4457     +121     
  Branches      897      920      +23     
==========================================
+ Hits         2186     2230      +44     
- Misses       2150     2227      +77
Impacted Files Coverage Δ
src/client/firefox/create.js 13.33% <ø> (ø) ⬆️
assets/images/Svg.js 84.61% <ø> (ø) ⬆️
src/utils/breakpoint/index.js 78.46% <0%> (ø) ⬆️
src/utils/editor/index.js 10.63% <0%> (-4%) ⬇️
src/components/Editor/Breakpoint.js 5.55% <0%> (-0.22%) ⬇️
src/utils/editor/source-documents.js 6.25% <0%> (-6.8%) ⬇️
src/actions/navigation.js 11.76% <0%> (-0.74%) ⬇️
src/components/Editor/CallSites.js 2.08% <0%> (-0.05%) ⬇️
src/components/Editor/CallSite.js 4.44% <0%> (-0.21%) ⬇️
src/client/firefox.js 88.23% <100%> (+0.73%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e650039...76487fd. Read the comment docs.

const { line, column } = getTokenLocation(editor.codeMirror, target);

this.toggleBreakpoint(line + 1, column - 2);
this.toggleBreakpoint(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suppose we can't use a similar toSourceLocation method here because of the strange column offset...

Comment thread src/components/Editor/CallSites.js Outdated
key: index,
callSite,
editor,
sourceId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets pass the selectedSource down if possible. I think that would be a little more useful

Comment thread src/components/Editor/index.js Outdated
let sourceLine = toSourceLine(selectedSource.get("id"), line);
const bp = breakpointAtLocation(breakpoints, selectedLocation, {
line: sourceLine
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to move after a rebase

Comment thread src/selectors/linesInScope.js Outdated
const sourceText = source.get("text");
const sourceNumLines = typeof sourceText === "string"
? sourceText.split("\n").length
: sourceText.get("binary").length;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • can we look at source.isWasm here instead of typeof sourceText === "string" 
  • perhaps move this to a helper utils/source

Comment thread src/utils/editor/index.js Outdated
updateLineNumberFormat(editor, sourceId);
}

function renderWasmText(editor, sourceId: string, wasm: any) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move much of this to utils/editor/wasm

function renderWasmText(editor, sourceId: string, wasm: any) {
  const lines = getWasmLines(souceId, wasm)
  let text = { split: () => lines, match: () => false };
  editor.setText(text);
}

Comment thread src/utils/editor/index.js Outdated
editor.setText(text);
}
updateLineNumberFormat(editor, sourceId);
editor.setMode(isSourceWasm ? { name: "text" } : getMode(source.toJS()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move isSourceWasm ? { name: "text" } to getMode?

@jasonLaster jasonLaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of small comments, but otherwise looks good.

Comment thread src/components/Editor/index.js Outdated
if (prevProps.selectedLocation !== selectedLocation) {
if (selectedLocation && selectedLocation.line != undefined) {
this.pendingJumpLine = selectedLocation.line;
this.pendingJumpLine = selectedLocation;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets change pendingJumpLine to pendingJumpLocation

Comment thread src/components/Editor/index.js Outdated

if (gutter !== "CodeMirror-foldgutter") {
toggleBreakpoint(line + 1);
const sourceId = selectedSource ? selectedSource.get("id") : "";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move the selectedSource higher up.

if (! selectedSource) {
return 
}

we can do the same thing in onToggleBreakpoint

@yurydelendik yurydelendik force-pushed the wasm-sourcemaps branch 4 times, most recently from 7f78c27 to 412d1a7 Compare July 20, 2017 15:14
codehag and others added 4 commits July 20, 2017 12:43
…s#3387)

* move editor to state to ensure that re-layouts are still ok

* make flow happy / fix tests

* remove editor state
…html/fix-shortcut

Fix source search shortcut
@jasonLaster jasonLaster changed the base branch from master to next July 20, 2017 20:23
@jasonLaster jasonLaster merged commit 17b3be6 into firefox-devtools:next Jul 20, 2017
jasonLaster pushed a commit to jasonLaster/debugger.html that referenced this pull request Aug 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants