Closed Bug 901138 Opened 7 years ago Closed 6 months ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: rjacob, Assigned: wartmanm)

References

(Regressed 1 open bug)

Details

Attachments

(2 files)

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).
Assignee: nobody → general
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Summary: Debugger.Script.prototype should provide a startColumn getter → [jsdbg2] Debugger.Script.prototype should provide a startColumn getter
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.
Assignee: general → nobody
Duplicate of this bug: 1056310
I tried, but the results are wrong and I'm not really sure what's going on there. Would appreciate any help.
(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.

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.

Assignee: nobody → mattheww
Keywords: checkin-needed
Keywords: checkin-needed

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

Flags: needinfo?(mattheww)

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

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.

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

Flags: needinfo?(mattheww)
Keywords: checkin-needed

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07052ce569c3
Add Debugger.Script.prototype.startColumn r=jimb

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1570881
You need to log in before you can comment on or make changes to this bug.