[DevTools] Move suspended generator location to internal properties

[DevTools] Move suspended generator location to internal properties

BUG=623763
R=dgozman@chromium.org

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

[kozy, 2016-06-29 22:15:52]

Dmitry, please take a look.

[dgozman, 2016-06-30 00:47:38]

This need screenshot and more detailed description. Why do we have internal
location and not a raw location? Why don't we send all the information
immediately?

[kozy, 2016-07-06 18:43:46]

- internal locations are sent as primitive value, so we don't need additional
round trip to request it,
- internal locations are shown as a link in ObjectPropertySection.

Screenshot:
https://drive.google.com/file/d/0B7RLEApsOKBLTUphbmgxLTdfZGc/view?usp=sharing

Please take a look!

[dgozman, 2016-07-06 19:25:12]

Let's leave the only place for location: in properties section.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:219:
this._linkifier = new WebInspector.Linkifier();
Let's pass linkifier from the user of ObjectPropertiesSection.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:1051:
_showGeneratorLocation: function(remoteObject)
Let's remove this and rely on ObjectPropertiesSection.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/DebuggerScript.js (left):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/DebuggerScript.js:112: var
location = generatorMirror.sourceLocation() || funcMirror.sourceLocation();
Let's revert back to what we had, and remove all extra handling on frontend.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:728: if
(injectedScript.isInternalObject(object)) {
shouldPassByValue

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:730:
this.subtype = injectedScript._subtype(object);
Let's add description?

[kozy, 2016-07-06 22:38:42]

All done. Please take a look!

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:219:
this._linkifier = new WebInspector.Linkifier();
On 2016/07/06 19:25:12, dgozman wrote:
> Let's pass linkifier from the user of ObjectPropertiesSection.

Done.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:1051:
_showGeneratorLocation: function(remoteObject)
On 2016/07/06 19:25:12, dgozman wrote:
> Let's remove this and rely on ObjectPropertiesSection.

Done.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/DebuggerScript.js (left):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/DebuggerScript.js:112: var
location = generatorMirror.sourceLocation() || funcMirror.sourceLocation();
On 2016/07/06 19:25:12, dgozman wrote:
> Let's revert back to what we had, and remove all extra handling on frontend.

Done.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js
(right):

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:728: if
(injectedScript.isInternalObject(object)) {
On 2016/07/06 19:25:12, dgozman wrote:
> shouldPassByValue

Done.

https://codereview.chromium.org/2112673003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:730:
this.subtype = injectedScript._subtype(object);
On 2016/07/06 19:25:12, dgozman wrote:
> Let's add description?

Done.

[dgozman, 2016-07-07 00:44:34]

Almost there! Moar linkifiers!

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:223:

nit: extra blank line?

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:571:
treeNode.appendChild(new
WebInspector.ObjectPropertyTreeElement(getterProperty));
We should pass linkifier through everywhere.

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

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:720:
generatorObjectLocation: function(remoteObject)
I think we don't use this.

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/ScopeChainSidebarPane.js
(right):

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/ScopeChainSidebarPane.js:123:
var section = new
WebInspector.ObjectPropertiesSection(WebInspector.SourceMapNamesResolver.resolveScopeInObject(scope),
titleElement, null, emptyPlaceholder, true, extraProperties);
Should have a linkifier here.

[kozy, 2016-07-07 17:58:03]

All done, please take a look!

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:223:

On 2016/07/07 00:44:34, dgozman wrote:
> nit: extra blank line?

Done.

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:571:
treeNode.appendChild(new
WebInspector.ObjectPropertyTreeElement(getterProperty));
On 2016/07/07 00:44:34, dgozman wrote:
> We should pass linkifier through everywhere.

Done.

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

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:720:
generatorObjectLocation: function(remoteObject)
On 2016/07/07 00:44:34, dgozman wrote:
> I think we don't use this.

Done.

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/sources/ScopeChainSidebarPane.js
(right):

https://codereview.chromium.org/2112673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/ScopeChainSidebarPane.js:123:
var section = new
WebInspector.ObjectPropertiesSection(WebInspector.SourceMapNamesResolver.resolveScopeInObject(scope),
titleElement, null, emptyPlaceholder, true, extraProperties);
On 2016/07/07 00:44:34, dgozman wrote:
> Should have a linkifier here.

Done.

[dgozman, 2016-07-07 19:34:26]

lgtm

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:32:
* @param {?WebInspector.Linkifier=} linkifier
Let's make it !WebInspector.Linkifier= and pass undefined instead of null. Two
types of nullability don't really make sense.

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:37:
WebInspector.ObjectPropertiesSection = function(object, title, linkifier,
emptyPlaceholder, ignoreHasOwnProperty, extraProperties)
Unfortunately I cannot check that you updated all the calls to this method, and
it's quite dangerous due to all the optional parameters. Can you please
double-check?

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:1196:
var loc = value.value;
location

[kozy, 2016-07-07 21:32:38]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-07-07 21:32:38]

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/2112673003/#ps80001
(title: "addressed comments")

[kozy, 2016-07-07 21:33:28]

all done

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:32:
* @param {?WebInspector.Linkifier=} linkifier
On 2016/07/07 19:34:25, dgozman wrote:
> Let's make it !WebInspector.Linkifier= and pass undefined instead of null. Two
> types of nullability don't really make sense.

Done.

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:37:
WebInspector.ObjectPropertiesSection = function(object, title, linkifier,
emptyPlaceholder, ignoreHasOwnProperty, extraProperties)
On 2016/07/07 19:34:25, dgozman wrote:
> Unfortunately I cannot check that you updated all the calls to this method,
and
> it's quite dangerous due to all the optional parameters. Can you please
> double-check?

I checked it with search over code base while adding and I think that closure
compiler will catch me if I miss something.

https://codereview.chromium.org/2112673003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:1196:
var loc = value.value;
On 2016/07/07 19:34:26, dgozman wrote:
> location

Done.

[commit-bot: I haz the power, 2016-07-07 21:33:49]

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-07-07 23:36:16]

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

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

Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[kozy, 2016-07-07 23:36:50]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2016-07-07 23:37:34]

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-07-08 01:15:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-08 01:17:06]

Description was changed from

==========
[DevTools] Move suspended generator location to internal properties

BUG=623763
R=dgozman@chromium.org
==========

to

==========
[DevTools] Move suspended generator location to internal properties

BUG=623763
R=dgozman@chromium.org

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

[commit-bot: I haz the power, 2016-07-08 01:17:07]

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