901138 - [jsdbg2] Debugger.Script.prototype should provide a startColumn getter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jake Bailey]

Debugger.Script.prototype.startLine provides the line at which a script's code starts (e.g. a function definition), but there is no accessor for the column. This would allow actors to provide complete source locations for function definitions (especially important in minified JS).

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

In bug 917583, I am hardcoding a column of 0 for function definition sites, since we don't have this yet. When we fix this bug, we should also replace that hard coding with startColumn.

Comment 3

[Jakub Jurovych]

Attachment: bug-901138-wip.patch

I tried, but the results are wrong and I'm not really sure what's going on there. Would appreciate any help.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to Jakub Jurovych from comment #3)
> Created attachment 8479340 [details] [diff] [review]
> bug-901138-wip.patch
> 
> I tried, but the results are wrong and I'm not really sure what's going on
> there. Would appreciate any help.

I'm willing to mentor you on this bug if you'd like.

Comment 5

[mattheww]

Attachment: Bug 901138: Add Debugger.Script.prototype.startColumn

I copy-pasted the implementation of Debugger.Script.prototype.startLine, and added a test and documenation. To make it work, I made JSScript::column_ mandatory, like lineno_. The only place where lineno_ was set and it was not was in JSScript::fullyInitFromEmitter, which copies the line number from BytecodeEmitter::firstLine, which is itself set in BytecodeEmitter's constructors. I followed the easiest path and added a new column field to BytecodeEmitter and all of its constructors.

Comment 6

[Cristina Coroiu [:ccoroiu]]

Failed to land:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmp7drD1q js/src/frontend/BytecodeEmitter.cpp Hunk #2 succeeded at 119 with fuzz 1 (offset -1 lines). Hunk #5 FAILED at 5749. 1 out of 5 hunks FAILED -- saving rejects to file js/src/frontend/BytecodeEmitter.cpp.rej abort: patch command failed: exited with status 256

Comment 7

[Jason Laster [:jlast]]

Mattheww, i'd be happy to land your patch once it is updated and tests are green

Comment 8

[Jim Blandy :jimb]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5035f1ec7067e2fc8a32341d8b83ee5620ce73

Comment 9

[Jim Blandy :jimb]

That try push is a version of the patch rebased around the BytecodeEmitter conflict, which is a simple variable rename. I've asked Matthew to update the patch on Phabricator.

Comment 10

[mattheww]

The patch is updated now, sorry it took so long.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07052ce569c3
Add Debugger.Script.prototype.startColumn r=jimb

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/07052ce569c3