Refactor script position calculation

Refactor script position calculation

Script position calculation logic (i.e. line & column numbers for a
given code position) is now based on a single method
Script::GetPositionInfo(). Refactored related code in isolate.cc and
js/messages.js to use the new method and removed the line_ends JS
accessor.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/c04d547298ce4fd425ef1eaa9b02ad1e177918dc
Cr-Commit-Position: refs/heads/master@{#36359}

[jgruber1, 2016-05-17 13:27:47]

The CQ bit was checked by jgruber@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 13:27:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986173002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986173002/1

[commit-bot: I haz the power, 2016-05-17 13:59:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 13:59:09]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-05-18 13:23:09]

LGTM!

Please consider the comments for a separate CL.

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc#newcode555
src/isolate.cc:555: bool valid_pos = Script::GetPositionInfo(script, position,
&info, true);
Can we pass an enum value instead of true/false? That would be a lot easier to
read at the call site.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:246: return new SourceLocation(this, position, info.line,
info.column,
Can we simply return 'info'? Do we need line_start and line_end for anything
other than formatting the string in SourceLocationSourceText? Is
SourceLocation.start and SourceLocation.end used anywhere? If not, we can simply
omit line_start/line_end and simply have %ScriptPositionInfo store the string
slice for the source text on 'info'. Substrings are fairly cheap to create
anyways.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:266: function ScriptLocationFromLine(opt_line, opt_column,
opt_offset_position) {
This function is also only used inside V8. Let's move this thing to the runtime
entirely and only call the runtime function in V8. Let's not expose it on the
Script object anymore.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:332: return new SourceSlice(this,
The SourceSlice object seems not to be used anywhere in Chrome, and in V8 only
in a test case. Let's remove that test case and this whole SourceSlice thing.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:339: function ScriptSourceLine(opt_line) {
Seems like this function is only used inside V8 (please verify). Can we turn
this whole thing into a runtime function and and use that runtime function at
call sites and not expose this on the Script object anymore?

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:366: return %ScriptLineCount(this);
This also doesn't seem to be used outside of V8. Let's not expose this anymore.

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12383: InitLineEnds(script);
You can avoid this wrapper if you do something like this:

bool Script::GetPositionInfo(int position,
                             PositionInfo* info, bool with_offset) {
  Handle<Script> script(this);
  InitLineEnds(script);
  DisallowHeapAllocation no_gc;
  ...
}

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12417: if (right < left) {
Let's just DCHECK_GE(right, left);

https://codereview.chromium.org/1986173002/diff/40001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1537: } else {  // 0 < line <= line_count
Let's simply make this a DCHECK.

[jgruber1, 2016-05-19 08:05:24]

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc#newcode555
src/isolate.cc:555: bool valid_pos = Script::GetPositionInfo(script, position,
&info, true);
On 2016/05/18 13:23:08, Yang wrote:
> Can we pass an enum value instead of true/false? That would be a lot easier to
> read at the call site.

Good point. Is there an easy way to define an enum visible from both JS and C++
sides? Messages seems to have something like this, but they define it in
include/v8.h.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:246: return new SourceLocation(this, position, info.line,
info.column,
On 2016/05/18 13:23:08, Yang wrote:
> Can we simply return 'info'? Do we need line_start and line_end for anything
> other than formatting the string in SourceLocationSourceText? Is
> SourceLocation.start and SourceLocation.end used anywhere? If not, we can
simply
> omit line_start/line_end and simply have %ScriptPositionInfo store the string
> slice for the source text on 'info'. Substrings are fairly cheap to create
> anyways.

Done. I'll remove ScripLocationFromPosition itself in an upcoming CL.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:266: function ScriptLocationFromLine(opt_line, opt_column,
opt_offset_position) {
On 2016/05/18 13:23:08, Yang wrote:
> This function is also only used inside V8. Let's move this thing to the
runtime
> entirely and only call the runtime function in V8. Let's not expose it on the
> Script object anymore.

Will do in an upcoming commit unless you'd prefer it in this one.

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:332: return new SourceSlice(this,
On 2016/05/18 13:23:08, Yang wrote:
> The SourceSlice object seems not to be used anywhere in Chrome, and in V8 only
> in a test case. Let's remove that test case and this whole SourceSlice thing.

What about DebugCommandProcessor.prototype.sourceRequest_?

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:339: function ScriptSourceLine(opt_line) {
On 2016/05/18 13:23:08, Yang wrote:
> Seems like this function is only used inside V8 (please verify). Can we turn
> this whole thing into a runtime function and and use that runtime function at
> call sites and not expose this on the Script object anymore?

Ditto (upcoming commit).

https://codereview.chromium.org/1986173002/diff/40001/src/js/messages.js#newc...
src/js/messages.js:366: return %ScriptLineCount(this);
On 2016/05/18 13:23:09, Yang wrote:
> This also doesn't seem to be used outside of V8. Let's not expose this
anymore.

Ditto.

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12383: InitLineEnds(script);
On 2016/05/18 13:23:09, Yang wrote:
> You can avoid this wrapper if you do something like this:
> 
> bool Script::GetPositionInfo(int position,
>                              PositionInfo* info, bool with_offset) {
>   Handle<Script> script(this);
>   InitLineEnds(script);
>   DisallowHeapAllocation no_gc;
>   ...
> }

I used the wrapper in order to keep Script::GetLineNumber(int) allocation-free.
Not sure how essential that is, do you know more about it?

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12417: if (right < left) {
On 2016/05/18 13:23:09, Yang wrote:
> Let's just DCHECK_GE(right, left);

Done (DCHECK_LT(left, right)).

https://codereview.chromium.org/1986173002/diff/40001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1537: } else {  // 0 < line <= line_count
On 2016/05/18 13:23:09, Yang wrote:
> Let's simply make this a DCHECK.

Done.

[jgruber1, 2016-05-19 08:56:02]

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/isolate.cc#newcode555
src/isolate.cc:555: bool valid_pos = Script::GetPositionInfo(script, position,
&info, true);
On 2016/05/19 08:05:24, jgruber wrote:
> On 2016/05/18 13:23:08, Yang wrote:
> > Can we pass an enum value instead of true/false? That would be a lot easier
to
> > read at the call site.
> 
> Good point. Is there an easy way to define an enum visible from both JS and
C++
> sides? Messages seems to have something like this, but they define it in
> include/v8.h.

And done, using the enum on C++ side only.

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1986173002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12383: InitLineEnds(script);
On 2016/05/19 08:05:24, jgruber wrote:
> On 2016/05/18 13:23:09, Yang wrote:
> > You can avoid this wrapper if you do something like this:
> > 
> > bool Script::GetPositionInfo(int position,
> >                              PositionInfo* info, bool with_offset) {
> >   Handle<Script> script(this);
> >   InitLineEnds(script);
> >   DisallowHeapAllocation no_gc;
> >   ...
> > }
> 
> I used the wrapper in order to keep Script::GetLineNumber(int)
allocation-free.
> Not sure how essential that is, do you know more about it?

Done, removed the static wrapper.

[jgruber1, 2016-05-19 08:58:17]

The CQ bit was checked by jgruber@google.com

[jgruber1, 2016-05-19 08:58:17]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1986173002/#ps80001
(title: "enum instead of bool, remove static wrapper")

[commit-bot: I haz the power, 2016-05-19 08:58:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986173002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986173002/80001

[commit-bot: I haz the power, 2016-05-19 09:22:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-19 09:22:06]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[jgruber1, 2016-05-19 11:46:16]

The CQ bit was checked by jgruber@google.com

[jgruber1, 2016-05-19 11:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1986173002/#ps100001
(title: "Fix DCHECK logic")

[commit-bot: I haz the power, 2016-05-19 11:46:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1986173002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1986173002/100001

[commit-bot: I haz the power, 2016-05-19 12:22:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-19 12:23:41]

Description was changed from

==========
Refactor script position calculation

Script position calculation logic (i.e. line & column numbers for a
given code position) is now based on a single method
Script::GetPositionInfo(). Refactored related code in isolate.cc and
js/messages.js to use the new method and removed the line_ends JS
accessor.

R=yangguo@chromium.org
BUG=
==========

to

==========
Refactor script position calculation

Script position calculation logic (i.e. line & column numbers for a
given code position) is now based on a single method
Script::GetPositionInfo(). Refactored related code in isolate.cc and
js/messages.js to use the new method and removed the line_ends JS
accessor.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/c04d547298ce4fd425ef1eaa9b02ad1e177918dc
Cr-Commit-Position: refs/heads/master@{#36359}
==========

[commit-bot: I haz the power, 2016-05-19 12:23:42]

Patchset 6 (id:??) landed as
https://crrev.com/c04d547298ce4fd425ef1eaa9b02ad1e177918dc
Cr-Commit-Position: refs/heads/master@{#36359}

[Michael Achenbach, 2016-05-19 13:54:13]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1994973002/ by machenbach@chromium.org.

The reason for reverting is: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui....