[DevTools] added BreakpointManager.possibleBreakpoints method

[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org

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

[kozy, 2016-11-09 02:28:38]

Andrey and Dmitry, please take a look!

[kozy, 2016-11-09 17:25:23]

Description was changed from

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISource code and TextRange and returns UILocations of
possible breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

to

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISource code and TextRange and returns UILocations of
possible breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

[dgozman, 2016-11-09 20:01:46]

Description was changed from

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISource code and TextRange and returns UILocations of
possible breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

to

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible
breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

[dgozman, 2016-11-09 20:44:03]

The patch is doing more than description claims :-)

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/use-possible-breakpoints-to-resolve-breakpoint.html
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/use-possible-breakpoints-to-resolve-breakpoint.html:64:
<p></p>
Please add test description.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/click-gutter-breakpoint.html
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/click-gutter-breakpoint.html:22:
return new Promise(resolve => InspectorTest.addSniffer(sourceFrame.__proto__,
"_setBreakpoint", resolve, false));
Introduce a special method |_breakpointWasSetForTest| and use it in sniffer
instead.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:298:
{
{ on the previous line

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:299:
var sortedLocations = locations.map(location =>
this._debuggerWorkspaceBinding.rawLocationToUILocation(location));
Could rawLocationToUILocation return null?

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:307:
if (sortedLocations[i].id() === lastLocation.id())
How could this happen?

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:320:
function compareLocations(first, second)
Don't we have a comparator on UILocation? If not, let's add one.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:288: var
resolveCallback;
fulfill

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:300:
console.error('Failed to get possible breakpoints: ' + (error || 'empty output
locations'));
Don't spam the console. I expect this to happen frequently due to scriptId
mismatch from sourcemaps or something.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1026:
for (let i = 0; i < linesToCheck; ++i)
let! Oh my!

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1027:
promise = promise.then(locations => locations ? locations :
findPossibleBreakpoints.call(this, lineNumber + i));
From my tests on DevTools engineers;
- one failed to understand this;
- another one understood it in 90 seconds;
- one knew in advance what's happening.
Compiler failed to understand this as well.
I think we should make this more explicit.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1033:
* @return {!Promise<!Array<!WebInspector.UILocation>>}
?Array

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1045:
* @param {!Array<!WebInspector.UILocation>} locations
?Array

[kozy, 2016-11-10 03:17:37]

All done, please take a look!

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/use-possible-breakpoints-to-resolve-breakpoint.html
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-breakpoints/use-possible-breakpoints-to-resolve-breakpoint.html:64:
<p></p>
On 2016/11/09 20:44:03, dgozman wrote:
> Please add test description.

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/click-gutter-breakpoint.html
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/sources/debugger-ui/click-gutter-breakpoint.html:22:
return new Promise(resolve => InspectorTest.addSniffer(sourceFrame.__proto__,
"_setBreakpoint", resolve, false));
On 2016/11/09 20:44:03, dgozman wrote:
> Introduce a special method |_breakpointWasSetForTest| and use it in sniffer
> instead.

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:298:
{
On 2016/11/09 20:44:03, dgozman wrote:
> { on the previous line

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:299:
var sortedLocations = locations.map(location =>
this._debuggerWorkspaceBinding.rawLocationToUILocation(location));
On 2016/11/09 20:44:03, dgozman wrote:
> Could rawLocationToUILocation return null?

No, there are asserts in our code base that it should return something. I'll add
a check in filter to be sure.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:307:
if (sortedLocations[i].id() === lastLocation.id())
On 2016/11/09 20:44:03, dgozman wrote:
> How could this happen?

SourceMap maps two different raw location to one UILocation.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/BreakpointManager.js:320:
function compareLocations(first, second)
On 2016/11/09 20:44:03, dgozman wrote:
> Don't we have a comparator on UILocation? If not, let's add one.

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:288: var
resolveCallback;
On 2016/11/09 20:44:03, dgozman wrote:
> fulfill

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:300:
console.error('Failed to get possible breakpoints: ' + (error || 'empty output
locations'));
On 2016/11/09 20:44:03, dgozman wrote:
> Don't spam the console. I expect this to happen frequently due to scriptId
> mismatch from sourcemaps or something.

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1026:
for (let i = 0; i < linesToCheck; ++i)
On 2016/11/09 20:44:03, dgozman wrote:
> let! Oh my!

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1027:
promise = promise.then(locations => locations ? locations :
findPossibleBreakpoints.call(this, lineNumber + i));
On 2016/11/09 20:44:03, dgozman wrote:
> From my tests on DevTools engineers;
> - one failed to understand this;
> - another one understood it in 90 seconds;
> - one knew in advance what's happening.
> Compiler failed to understand this as well.
> I think we should make this more explicit.

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1033:
* @return {!Promise<!Array<!WebInspector.UILocation>>}
On 2016/11/09 20:44:03, dgozman wrote:
> ?Array

Done.

https://codereview.chromium.org/2484283004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1045:
* @param {!Array<!WebInspector.UILocation>} locations
On 2016/11/09 20:44:03, dgozman wrote:
> ?Array

Done.

[kozy, 2016-11-10 15:42:26]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 15:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2016-11-10 16:02:42]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 16:03:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 18:36:06]

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

[commit-bot: I haz the power, 2016-11-10 18:36:07]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-11-10 19:06:52]

Overall looks good, but let's take a look at the usage before committing.

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1061:
if (locations)
if (locations || !linesToCheck)

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1062:
return Promise.resolve(/** @type {?Array<!WebInspector.UILocation>}
*/(locations));
No need to cast.

[dgozman, 2016-11-10 19:11:48]

lgtm

[kozy, 2016-11-10 19:49:53]

all done, thanks!

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js
(right):

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1061:
if (locations)
On 2016/11/10 19:06:52, dgozman wrote:
> if (locations || !linesToCheck)

Done.

https://codereview.chromium.org/2484283004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:1062:
return Promise.resolve(/** @type {?Array<!WebInspector.UILocation>}
*/(locations));
On 2016/11/10 19:06:52, dgozman wrote:
> No need to cast.

Done.

[kozy, 2016-11-10 19:49:58]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-11-10 19:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2484283004/#ps120001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-11-10 19:50:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 21:09:22]

Description was changed from

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible
breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

to

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible
breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

[commit-bot: I haz the power, 2016-11-10 21:09:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-10 21:23:32]

Description was changed from

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible
breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org
==========

to

==========
[DevTools] added BreakpointManager.possibleBreakpoints method

This method takes UISourceCode and TextRange and returns UILocations of possible
breakpoints in this range.
It uses first target with JS capability that was able to resolve startUILocation
and endUILocation to call getPossibleBreakpoints on.
JavaScriptSourceFrame uses this method to get more accurate breakpoint location.

BUG=chromium:566801
R=lushnikov@chromium.org,dgozman@chromium.org

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

[commit-bot: I haz the power, 2016-11-10 21:23:33]

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