[inspector] remove iterators and for...of loops from injected-script-source

[inspector] remove iterators and for...of loops from injected-script-source

BUG=chromium:686003
Review-Url: https://codereview.chromium.org/2705533002
Cr-Commit-Position: refs/heads/master@{#43595}
Committed: https://chromium.googlesource.com/v8/v8/+/3a20c322bbc26b453ce9c3c3b83094af2fb722f2

[luoe, 2017-02-17 01:19:43]

Description was changed from

==========
iterator removal

BUG=chromium:686003
==========

to

==========
[inspector] remove iterators and for...of loops from injected-script-source

BUG=chromium:686003
==========

[luoe, 2017-02-17 01:20:06]

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

[luoe, 2017-02-17 01:20:06]

Please take a look

[kozy, 2017-02-21 19:01:55]

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:239:
InjectedScript.defaultPropertyAdder.prototype = {
We try to avoid using any complex JS construction in injected script to make it
more reliable. Could we avoid prototype inheritance and new call? We can pass
isSpaceAvailable as a function to this._propertyDescriptors instead.

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:426: var descriptors = [];
descriptors.__proto__ = null;
and then we will be sure that we don't use Array.prototype.push but use our cool
push function().

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:439: var length = properties ?
properties.length : (o.length || 0);
o.length can throw.

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:549: process(o, Object.keys(o));
All Object.* can throw, can we wrap it with try catch?

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:999: adder.add(descriptors,
internalPropertyDescriptor);
Our push safer.

[luoe, 2017-02-21 21:53:52]

ptal

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:239:
InjectedScript.defaultPropertyAdder.prototype = {
Yes, I think a customPush() will also need to be passed into
_propertyDescriptors() so that we can remember whether an overflow occurred.

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:426: var descriptors = [];
On 2017/02/21 19:01:55, kozy wrote:
> descriptors.__proto__ = null;
> and then we will be sure that we don't use Array.prototype.push but use our
cool
> push function().

Done.

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:439: var length = properties ?
properties.length : (o.length || 0);
I'll pass in objectLength

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:549: process(o, Object.keys(o));
On 2017/02/21 19:01:55, kozy wrote:
> All Object.* can throw, can we wrap it with try catch?

Done.

https://codereview.chromium.org/2705533002/diff/60001/src/inspector/injected-...
src/inspector/injected-script-source.js:999: adder.add(descriptors,
internalPropertyDescriptor);
Replaced with a customPush() that calls push() internally.

[kozy, 2017-02-23 23:27:15]

https://codereview.chromium.org/2705533002/diff/100001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/100001/src/inspector/injected...
src/inspector/injected-script-source.js:476: customPush(descriptors,
nullifyObjectProto(descriptor));
let's use regular push and pass callback to function:
bool propertyAdded(descriptor, moreAvailable:bool).
To calculate morePropertiesAavailable we can replace current for loop with while
loop and try to adjust index to next property after actual push and before
propertyAdded call.

[luoe, 2017-02-24 00:46:01]

ptal

https://codereview.chromium.org/2705533002/diff/100001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/100001/src/inspector/injected...
src/inspector/injected-script-source.js:476: customPush(descriptors,
nullifyObjectProto(descriptor));
After our offline discussion, I've removed isSpaceAvailable(), renamed the
callback to addPropertyIfNeeded(), which returns false if is no longer accepting
properties.  Also process() can return a boolean to let us break early when it
knows.

[kozy, 2017-02-28 17:14:43]

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:429: if (accessorPropertiesOnly ||
forPreview)
Can we filter it inside of addPropertyIfNeeded by wasThrown flag for preview?

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:451: // Ignore __proto__ property.
Can we move this into addPropertyIfNeeded for preview?

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:488: if (ownProperties || forPreview)
can we pass ownProperties = true for preview and with two other comments finally
get rid of forPreview flag?

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:905: var descriptors =
injectedScript._propertyDescriptors(object, undefined, undefined,
firstLevelKeys, true, addPropertyIfNeeded);
please add /* propertyNamesOnly */ comment for undefined.

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:949: overflow = true;
preview.overflow = true

[luoe, 2017-02-28 21:46:39]

ptal

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:429: if (accessorPropertiesOnly ||
forPreview)
On 2017/02/28 17:14:43, kozy wrote:
> Can we filter it inside of addPropertyIfNeeded by wasThrown flag for preview?

Done.

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:451: // Ignore __proto__ property.
On 2017/02/28 17:14:43, kozy wrote:
> Can we move this into addPropertyIfNeeded for preview?

Done.

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:488: if (ownProperties || forPreview)
On 2017/02/28 17:14:43, kozy wrote:
> can we pass ownProperties = true for preview and with two other comments
finally
> get rid of forPreview flag?

Done.

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:905: var descriptors =
injectedScript._propertyDescriptors(object, undefined, undefined,
firstLevelKeys, true, addPropertyIfNeeded);
added comment for accessorPropertiesOnly here
and comment for propertyNamesOnly above in getProperties()

https://codereview.chromium.org/2705533002/diff/120001/src/inspector/injected...
src/inspector/injected-script-source.js:949: overflow = true;
On 2017/02/28 17:14:43, kozy wrote:
> preview.overflow = true

Done.

[kozy, 2017-03-01 03:16:35]

lgtm

https://codereview.chromium.org/2705533002/diff/140001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/140001/src/inspector/injected...
src/inspector/injected-script-source.js:385: addPropertyIfNeeded =
addPropertyIfNeeded || function(descriptors, descriptor) { push(descriptors,
descriptor); return true; };
can we always pass this function and defined it near another call?

[luoe, 2017-03-01 21:47:19]

https://codereview.chromium.org/2705533002/diff/140001/src/inspector/injected...
File src/inspector/injected-script-source.js (right):

https://codereview.chromium.org/2705533002/diff/140001/src/inspector/injected...
src/inspector/injected-script-source.js:385: addPropertyIfNeeded =
addPropertyIfNeeded || function(descriptors, descriptor) { push(descriptors,
descriptor); return true; };
On 2017/03/01 03:16:35, kozy wrote:
> can we always pass this function and defined it near another call?

Done.

[luoe, 2017-03-01 21:48:33]

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

[commit-bot: I haz the power, 2017-03-01 21:48:35]

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-03-01 22:14:49]

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

[commit-bot: I haz the power, 2017-03-01 22:14:50]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-03-01 23:03:36]

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

[commit-bot: I haz the power, 2017-03-01 23:03:41]

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-03-01 23:05:14]

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

[commit-bot: I haz the power, 2017-03-01 23:05:14]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-03-03 02:53:46]

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

[commit-bot: I haz the power, 2017-03-03 02:53:54]

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-03-03 02:55:04]

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

[commit-bot: I haz the power, 2017-03-03 02:55:05]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/18394)

[luoe, 2017-03-03 03:47:16]

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

[commit-bot: I haz the power, 2017-03-03 03:47:26]

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-03-03 04:12:00]

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

[commit-bot: I haz the power, 2017-03-03 04:12:02]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-03-03 18:20:35]

Cool!
lgtm % one nit.

https://codereview.chromium.org/2705533002/diff/200001/test/inspector/inspect...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2705533002/diff/200001/test/inspector/inspect...
test/inspector/inspector-test.cc:539: if (args.Length() != 1) {
&& args[0]->IsObject()

[luoe, 2017-03-03 18:56:22]

https://codereview.chromium.org/2705533002/diff/200001/test/inspector/inspect...
File test/inspector/inspector-test.cc (right):

https://codereview.chromium.org/2705533002/diff/200001/test/inspector/inspect...
test/inspector/inspector-test.cc:539: if (args.Length() != 1) {
Replaced the CHECK on L544 with a condition as you suggested.

[luoe, 2017-03-03 18:57:05]

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

[commit-bot: I haz the power, 2017-03-03 18:57:08]

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-03-03 19:24:53]

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

[commit-bot: I haz the power, 2017-03-03 19:24:54]

Dry run: This issue passed the CQ dry run.

[luoe, 2017-03-03 19:28:57]

The CQ bit was checked by luoe@chromium.org

[luoe, 2017-03-03 19:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2705533002/#ps220001
(title: "ac")

[commit-bot: I haz the power, 2017-03-03 19:29:03]

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-03-03 19:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488569337866940,
"parent_rev": "9ef1e35bc6c6ef2ddb55211563f169bbc3eaecb3", "commit_rev":
"3a20c322bbc26b453ce9c3c3b83094af2fb722f2"}

[commit-bot: I haz the power, 2017-03-03 19:30:47]

Description was changed from

==========
[inspector] remove iterators and for...of loops from injected-script-source

BUG=chromium:686003
==========

to

==========
[inspector] remove iterators and for...of loops from injected-script-source

BUG=chromium:686003

Review-Url: https://codereview.chromium.org/2705533002
Cr-Commit-Position: refs/heads/master@{#43595}
Committed:
https://chromium.googlesource.com/v8/v8/+/3a20c322bbc26b453ce9c3c3b83094af2fb...
==========

[commit-bot: I haz the power, 2017-03-03 19:30:48]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/3a20c322bbc26b453ce9c3c3b83094af2fb...