Closed Bug 863089 Opened 7 years ago Closed 4 years ago

Replace Debugger.Script.getOffsetLine with Debugger.Script.prototype.getOffsetLocation (which returns line and column numbers, not only the line number)

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: tromey)

References

Details

Attachments

(1 file, 2 obsolete files)

getOffsetColumn()

Returns the column number in the original source code file on which the specified bytecode offset occurs.

Number getOffsetColumn(
  offset
);

Parameters:

offset
    The offset to the bytecode for which to retrieve the corresponding source code column number.

Return value:

The column number on which the specified bytecode offset occurs.

Exceptions thrown:

TypeError
    The specified offset is invalid.
I thought the debugger API/spec/etc. deliberately didn't consider anything about bytecode, as bytecode is an implementation detail.  Am I wrong?  :-\
Waldo,

We already have |Debugger.Script.prototype.getOffsetLine| but we also need columns so we can properly set breakpoints in minified scripts. See my related work in bug 860035, which has a wip patch with a shim to get around the lack of this method.

Whether it was a good idea to expose bytecode offsets originally, well I trust the designers of the API, and since they're already out, I don't think I can contribute anything by discussing it more right now :)
After talking with Jim on IRC, we decided we should just change the existing method to return an object which has both line and column numbers. No need to do the search twice.
Assignee: general → nfitzgerald
Summary: Implement Debugger.Script.prototype.getOffsetColumn → Replace Debugger.Script.getOffsetLine with Debugger.Script.prototype.getOffsetLocation (which returns line and column numbers, not only the line number)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> I thought the debugger API/spec/etc. deliberately didn't consider anything
> about bytecode, as bytecode is an implementation detail.  Am I wrong?  :-\

That would have been nice, but Debugger does expose bytecode offsets, because the source relationship was too complicated for me to feel sure I'd covered all the cases. Instead, Debugger simply exposes the data we have, and makes the JS responsible for sorting out the details. :(
Unassigning myself because I am caught up in other things at the moment and I'm about to be gone for two weeks in Poland.
Assignee: nfitzgerald → general
Hey Eddy, you were asking me for places you could help out, how does this sound to you?

Jim and I were talking on IRC about having this return {lineNumber, columnNumber, offset} objects just like other jsdbg2 methods.
Flags: needinfo?(ejpbruel)
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Hey Eddy, you were asking me for places you could help out, how does this
> sound to you?
> 
> Jim and I were talking on IRC about having this return {lineNumber,
> columnNumber, offset} objects just like other jsdbg2 methods.

Hey Nick, sorry for not replying on this bug for so long. It kind of disappeared from my radar for a while. I'll assign this bug to me, and I'll take a look at it as soon as I can make time.
Assignee: general → ejpbruel
Flags: needinfo?(ejpbruel)
Assignee: ejpbruel → nobody
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Blocks: 1003554
Initial patch for safekeeping.
This still needs tests to exercise the new functionality.
Added a test case derived from the existing getAllColumnOffsets tests.
Attachment #8672123 - Attachment is obsolete: true
Attachment #8675839 - Flags: review?(nfitzgerald)
Duplicate of this bug: 1211906
Comment on attachment 8675839 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

Review of attachment 8675839 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much!
Attachment #8675839 - Flags: review?(nfitzgerald) → review+
Fix some bugs and tests.
Attachment #8675839 - Attachment is obsolete: true
Comment on attachment 8677545 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

A couple of notes on the changes.

One change was trivial -- changing a "let" to a "var" in a test.
I did this just for consistency, it isn't important.


The other change is a bit funnier.  I had to update test_blackboxing-01.js
to adapt to this patch.

The code here used to use PCToLineNumber, but now it uses BytecodeRangeWithPosition.
Previously I had thought that these did the same thing, but in fact they
differ in their handling of function prologue.  In particular, 
BytecodeRangeWithPosition skips the prologue, while PCToLineNumber does not.

What this means is that PCToLineNumber returns the "zeroth" line number of
the function when given an offset in the prologue, but BytecodeRangeWithPosition
gives the line number of the first "main" instruction instead.

I think the BytecodeRangeWithPosition behavior is better, but I'm re-requesting
review just in case.
Attachment #8677545 - Flags: review?(nfitzgerald)
It's worth noting:

https://bugzilla.mozilla.org/show_bug.cgi?id=969816#c7

Maybe the current behavior is intentional.

I still think the change is for the better.

However, if not, then it seems odd that the other Debugger.Script methods
skip the prologue.
Comment on attachment 8677545 [details] [diff] [review]
replace Debugger.Script.getOffsetLine with getOffsetLocation

Review of attachment 8677545 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8677545 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57714b421d53
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 969816
You need to log in before you can comment on or make changes to this bug.