DevTools: introduce object previews experiment

DevTools: introduce object previews experiment

BUG=405845
Review-Url: https://codereview.chromium.org/2605693003
Cr-Commit-Position: refs/heads/master@{#445814}
Committed: https://chromium.googlesource.com/chromium/src/+/f5966465d96bf16d576399c85d0199944d7b420d

[luoe, 2016-12-27 23:38:29]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[luoe, 2016-12-27 23:38:31]

Please take a look

[luoe, 2016-12-28 22:22:51]

Description was changed from

==========
DevTools: install object previews experiment

BUG=405845
==========

to

==========
DevTools: install object previews experiment

BUG=405845
==========

[luoe, 2016-12-28 22:22:51]

luoe@chromium.org changed reviewers:
- dgozman@chromium.org, lushnikov@chromium.org

[luoe, 2017-01-04 04:10:26]

luoe@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[luoe, 2017-01-04 04:10:27]

Please take a look.  I wasn't sure if the tests should be updated now or later,
so I've included them only in the latest patch.

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

https://codereview.chromium.org/2605693003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js:247:
span.setTextContentTruncatedIfNeeded(description);
With regexp and error types are going through here (see line 29), this will
be needed.

[luoe, 2017-01-04 23:33:47]

luoe@chromium.org changed reviewers:
+ einbinder@chromium.org

[dgozman, 2017-01-10 22:26:10]

Experiment stuff looks good, but let's separate if from the rest.

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

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:271:
if (value.preview && !hidePreview) {
You should check experiment here, not at the call site.

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js
(right):

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js:27:
if (preview.type !== 'object' || preview.subtype === 'null' || preview.subtype
=== 'regexp' ||
Why don't we land changes in this file separately? They affect everything, not
only the experiment, and deserve a separate description.

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:85: static
numEntries(preview) {
setOrMapEntriesCount

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:513: if
(Runtime.experiments.isEnabled('objectPreviews'))
Instead of checking experiment here, we should pass generatePreview flag from
the caller.

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:525: if
(Runtime.experiments.isEnabled('objectPreviews'))
ditto

[luoe, 2017-01-12 21:28:51]

Description was changed from

==========
DevTools: install object previews experiment

BUG=405845
==========

to

==========
DevTools: introduce object previews experiment

BUG=405845
==========

[luoe, 2017-01-13 01:11:46]

ptal

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

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:271:
if (value.preview && !hidePreview) {
On 2017/01/10 22:26:09, dgozman wrote:
> You should check experiment here, not at the call site.

Done.

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js
(right):

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js:27:
if (preview.type !== 'object' || preview.subtype === 'null' || preview.subtype
=== 'regexp' ||
The logic in lines 32 to 47 belong only to the experiment.  For everything else,
I agree with moving into another CL.

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:85: static
numEntries(preview) {
mapOrSetEntriesCount

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:513: if
(Runtime.experiments.isEnabled('objectPreviews'))
Moved to a few callers in ObjectPropertiesSection.js

https://codereview.chromium.org/2605693003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:525: if
(Runtime.experiments.isEnabled('objectPreviews'))
On 2017/01/10 22:26:10, dgozman wrote:
> ditto

Done.

[dgozman, 2017-01-13 02:55:42]

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt:27:
console-format-es6-2.html:11 MapIterator {(2) [41, 42], (2) [Object, Object]}
This looks strange, probably because we have a space between length and
contents.

What about something like this?
  Int32Array[41, 42, 43, ...(95)]
  [41, 42]
  [41, 42, ...(10 total)]

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt:220:
0: SetIterator {Object {foo: 1}}
This looks strange as well. Maybe just {foo: 1}?

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt
(left):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt:45:
console-format-es6.html:11 Map(1) {Object {a: Symbol(), Symbol(a): 2} => Object
{foo: 1}}
Looks like a regression?

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:207:
object.getAllProperties(false, false, propertiesCollected.bind(null,
scope.typeName()));
While you are here, could you please annotate it like this:

false /* accessorPropertiesOnly */, false /* generatePreview */

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPopoverHelper.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPopoverHelper.js:141:
false, didGetFunctionProperties.bind(this, result, popoverContentElement,
valueElement, anchorElement));
And here.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:223:
* @param {boolean=} hidePreview
Let's be consistent with the reset and make this a required parameter
'showPreview'.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js:35:
text = description.replace(/\[([0-9]+)\]|\(([0-9]+)\)/, '');
- Why do we do this?
- Can description contains [5] because one of the elements is "[5]" ?

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:687: return
remoteObject.getAllPropertiesPromise(false,
false).then(buildDetails.bind(this));
Please restore the comment (and add a new one).

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:241: * @param
{boolean=} generatePreview
Don't make it optional.

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

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:574:
.getAllProperties(false, false, this._prepareScopeVariables.bind(this,
callFrame));
nit: please comment parameters.

[luoe, 2017-01-13 23:20:13]

This patch is growing quickly!  In addition to changing parameters, these are
new:

- Never shows description "Object" before the preview
- Previews only go 1 level deep, iterators no longer show nested previews
(checked with isEntry), until cutoff logic is implemented
- Extracted the regexes for getting lengths in descriptions

Please take another look

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt:27:
console-format-es6-2.html:11 MapIterator {(2) [41, 42], (2) [Object, Object]}
For this experiment, I propose this output:
MapIterator {Array(2), Array(2)}

Another example:
MapIterator {Int32Array(2), Object}

The MapIterator only has n=2 entries, which should be clear.  Knowing the
contents of those entries is going 2 levels deep, which I don't want to do in
this patch.

I would like to show the 2nd level preview here for entries once cutoff logic is
in place, in the next patch.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-2-expected.txt:220:
0: SetIterator {Object {foo: 1}}
From the proposal in my comment above, I'd like to just show "Object" for now,
as "{foo: 1}" will be 2 levels deep.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt
(left):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/console/console-format-es6-expected.txt:45:
console-format-es6.html:11 Map(1) {Object {a: Symbol(), Symbol(a): 2} => Object
{foo: 1}}
Reverted.  Maps/Sets will always have their size shown in description.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:207:
object.getAllProperties(false, false, propertiesCollected.bind(null,
scope.typeName()));
On 2017/01/13 02:55:41, dgozman wrote:
> While you are here, could you please annotate it like this:
> 
> false /* accessorPropertiesOnly */, false /* generatePreview */

Done.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPopoverHelper.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPopoverHelper.js:141:
false, didGetFunctionProperties.bind(this, result, popoverContentElement,
valueElement, anchorElement));
On 2017/01/13 02:55:41, dgozman wrote:
> And here.

Done.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/ObjectPropertiesSection.js:223:
* @param {boolean=} hidePreview
On 2017/01/13 02:55:41, dgozman wrote:
> Let's be consistent with the reset and make this a required parameter
> 'showPreview'.

Done.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js
(right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/components/RemoteObjectPreviewFormatter.js:35:
text = description.replace(/\[([0-9]+)\]|\(([0-9]+)\)/, '');
Motivation:
I want "arr = new Uint16Array([9])" to show up as "Uint16Array [9]" and not
Uint16Array(1) [9].  To do so, I take the raw description "Uint16Array(1)" and
remove the length/parens.

Array descriptions should not contain things like "[5]", unless the
internalConstructorName (constructor/class/inferred name) contains "[5]". 
Today, we are relying on this assumption in SDK.RemoteObject.arrayLength()'s
implementation.  I've moved this into RemoteObject.js, perhaps there is a better
place?

My intended behavior is:
typedarray
  name + size if>1
array
  size if>1
everything else
  description

I'm not sure if this is the right thing todo.  If you'd like something different
for the experiment, I'm open to change this.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js (right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/DebuggerModel.js:687: return
remoteObject.getAllPropertiesPromise(false,
false).then(buildDetails.bind(this));
On 2017/01/13 02:55:41, dgozman wrote:
> Please restore the comment (and add a new one).

Done.

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js (right):

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/RemoteObject.js:241: * @param
{boolean=} generatePreview
On 2017/01/13 02:55:42, dgozman wrote:
> Don't make it optional.

Done.

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

https://codereview.chromium.org/2605693003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sources/JavaScriptSourceFrame.js:574:
.getAllProperties(false, false, this._prepareScopeVariables.bind(this,
callFrame));
On 2017/01/13 02:55:42, dgozman wrote:
> nit: please comment parameters.

Done.

[luoe, 2017-01-23 21:42:37]

gentle ping

[dgozman, 2017-01-24 01:51:42]

Let's undo test expectations and land it.

https://codereview.chromium.org/2605693003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2605693003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:131:
Runtime.experiments.enableForTest('objectPreviews');
Let's not do this for now.

[luoe, 2017-01-24 18:01:23]

Expectations undone.  I did add a few test cases to the current behavior.

Please take a look

https://codereview.chromium.org/2605693003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/2605693003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/main/Main.js:131:
Runtime.experiments.enableForTest('objectPreviews');
On 2017/01/24 01:51:42, dgozman wrote:
> Let's not do this for now.

Done.

[luoe, 2017-01-24 18:01:31]

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

[commit-bot: I haz the power, 2017-01-24 18:01:58]

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, 2017-01-24 18:04:06]

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

[commit-bot: I haz the power, 2017-01-24 18:04:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dgozman, 2017-01-24 18:21:42]

lgtm

[luoe, 2017-01-24 18:37:03]

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

[commit-bot: I haz the power, 2017-01-24 18:37:30]

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, 2017-01-24 20:28:17]

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

[commit-bot: I haz the power, 2017-01-24 20:28:18]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-01-24 20:28:48]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-01-24 20:28:49]

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/2605693003/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-24 20:29:20]

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, 2017-01-24 21:19:36]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485289728707420,
"parent_rev": "a6c87088de622f50ef919d19dfbd750603167162", "commit_rev":
"f5966465d96bf16d576399c85d0199944d7b420d"}

[commit-bot: I haz the power, 2017-01-24 21:20:13]

Description was changed from

==========
DevTools: introduce object previews experiment

BUG=405845
==========

to

==========
DevTools: introduce object previews experiment

BUG=405845

Review-Url: https://codereview.chromium.org/2605693003
Cr-Commit-Position: refs/heads/master@{#445814}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5966465d96bf16d576399c85d01...
==========

[commit-bot: I haz the power, 2017-01-24 21:20:15]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/f5966465d96bf16d576399c85d01...