[builtins] Move StringNormalize to a cpp builtin.

[builtins] Move StringNormalize to a cpp builtin.

CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364
Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Committed: https://crrev.com/e7b7ba8edd9cccd963940ac4b3e03b1e639a6418
Cr-Original-Commit-Position: refs/heads/master@{#39331}
Cr-Commit-Position: refs/heads/master@{#39342}

[petermarshall, 2016-09-07 14:13:21]

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

[commit-bot: I haz the power, 2016-09-07 14:13:24]

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

[Benedikt Meurer, 2016-09-08 04:41:30]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-09-08 04:41:30]

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:494: MaybeHandle<String>
maybe_form(Object::ToString(isolate, args.at<Object>(1)));
This is not correct, as the ToString can throw and thus the MaybeHandle may be
null, and form would also be null then. You need to use soemthing along the
lines of

Handle<Object> form = args.atOrUndefined(isolate, 1);
if (form->IsUndefined(isolate)) return *string;
Handle<String> form_string;
ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, form_string,
Object::ToString(isolate, form));

[petermarshall, 2016-09-09 08:58:49]

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

[commit-bot: I haz the power, 2016-09-09 08:58:56]

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-09-09 09:32:56]

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

[commit-bot: I haz the power, 2016-09-09 09:32:56]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-09 09:59:42]

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

[commit-bot: I haz the power, 2016-09-09 09:59:46]

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-09-09 10:01:41]

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

[commit-bot: I haz the power, 2016-09-09 10:01:41]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-09 10:34:59]

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

[commit-bot: I haz the power, 2016-09-09 10:35:04]

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

[petermarshall, 2016-09-09 10:35:32]

The CQ bit was unchecked by petermarshall@chromium.org

[petermarshall, 2016-09-09 10:35:59]

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

[commit-bot: I haz the power, 2016-09-09 10:36:04]

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-09-09 11:01:14]

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

[commit-bot: I haz the power, 2016-09-09 11:01:15]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-09 11:24:00]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.

BUG=
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.

BUG=v8:5364
==========

[petermarshall, 2016-09-09 11:38:26]

petermarshall@chromium.org changed reviewers:
+ franzih@chromium.org

[petermarshall, 2016-09-09 11:41:06]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-09-09 11:41:07]

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:494: MaybeHandle<String>
maybe_form(Object::ToString(isolate, args.at<Object>(1)));
On 2016/09/08 04:41:30, Benedikt Meurer wrote:
> This is not correct, as the ToString can throw and thus the MaybeHandle may be
> null, and form would also be null then. You need to use soemthing along the
> lines of
> 
> Handle<Object> form = args.atOrUndefined(isolate, 1);
> if (form->IsUndefined(isolate)) return *string;
> Handle<String> form_string;
> ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, form_string,
> Object::ToString(isolate, form));

Done.

[commit-bot: I haz the power, 2016-09-09 11:41:09]

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-09-09 11:41:14]

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

[commit-bot: I haz the power, 2016-09-09 11:41:17]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Franzi, 2016-09-09 12:51:04]

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:487: TO_THIS_STRING(string,
"String.prototype.trim");
trim?

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-string.cc:483: // If Intl is enabled, then i18n.js will
override it and provide the the
There's a double "the" in there. 

Can we change the comment? Maybe point out that this is only a fallback?
I'd prefer to write out Intl.

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-string.cc:492: Handle<String> form_string;
I usually try to avoid variable names that indicate its type. You can assign to
the same variable name, similar to builtins-symbol.cc:18.

[Franzi, 2016-09-09 12:52:28]

On 2016/09/09 at 12:51:04, Franzi wrote:
>
https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
> File src/builtins/builtins-string.cc (right):
> 
>
https://codereview.chromium.org/2315343002/diff/20001/src/builtins/builtins-s...
> src/builtins/builtins-string.cc:487: TO_THIS_STRING(string,
"String.prototype.trim");
> trim?
> 
Ignore this, comment refers to an older patch. Sorry!

[petermarshall, 2016-09-09 13:23:02]

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-string.cc:483: // If Intl is enabled, then i18n.js will
override it and provide the the
On 2016/09/09 at 12:51:04, Franzi wrote:
> There's a double "the" in there. 
> 
> Can we change the comment? Maybe point out that this is only a fallback?
> I'd prefer to write out Intl.

Done

https://codereview.chromium.org/2315343002/diff/140001/src/builtins/builtins-...
src/builtins/builtins-string.cc:492: Handle<String> form_string;
On 2016/09/09 at 12:51:04, Franzi wrote:
> I usually try to avoid variable names that indicate its type. You can assign
to the same variable name, similar to builtins-symbol.cc:18.

Hmm String::Equals complains because it needs String handles specifically, maybe
it makes sense to call the first one form_input or something?

[Benedikt Meurer, 2016-09-09 16:36:06]

LGTM once nit is addressed.

https://codereview.chromium.org/2315343002/diff/160001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2315343002/diff/160001/src/builtins/builtins....
src/builtins/builtins.h:537: /* ECMA-262 v6, section 21.1.3.12
String.prototype.normalize ( [form] ) */  \
Nit: for consistency use /* ES6 section ... */

[Franzi, 2016-09-12 05:49:55]

LGTM

[petermarshall, 2016-09-12 07:48:46]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-09-12 07:48:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, franzih@chromium.org
Link to the patchset: https://codereview.chromium.org/2315343002/#ps180001
(title: "Fix comment style")

[commit-bot: I haz the power, 2016-09-12 07:48:59]

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-09-12 08:31:29]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-12 08:32:08]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.

BUG=v8:5364
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

[commit-bot: I haz the power, 2016-09-12 08:32:09]

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

[petermarshall, 2016-09-12 08:33:04]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2335553002/ by petermarshall@chromium.org.

The reason for reverting is: Tests fail when i18n is switched off, trybots do
not run this configuration.

[petermarshall, 2016-09-12 08:49:06]

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

[commit-bot: I haz the power, 2016-09-12 08:49:13]

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

[Michael Achenbach, 2016-09-12 09:05:43]

machenbach@chromium.org changed reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2016-09-12 09:05:44]

Hint: if you suspect a CL might break something that's not on CQ by default, you
can make CQ add it by adding to your CL description e.g.:

CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

[petermarshall, 2016-09-12 09:07:15]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

[petermarshall, 2016-09-12 09:07:41]

On 2016/09/12 at 09:05:44, machenbach wrote:
> Hint: if you suspect a CL might break something that's not on CQ by default,
you can make CQ add it by adding to your CL description e.g.:
> 
> CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

Thanks :)

[commit-bot: I haz the power, 2016-09-12 09:14:30]

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

[commit-bot: I haz the power, 2016-09-12 09:14:32]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.v8
   For more details, see http://crbug.com/617627.

[petermarshall, 2016-09-12 09:49:05]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

[petermarshall, 2016-09-12 09:59:14]

PTAL, missed a bug in the last change because the i18n bot didn't run, it's
green now

[Michael Achenbach, 2016-09-12 10:05:00]

nit: Please add another linebreak after the commit title.

[petermarshall, 2016-09-12 10:27:59]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.

CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

[petermarshall, 2016-09-12 10:28:11]

On 2016/09/12 at 10:05:00, machenbach wrote:
> nit: Please add another linebreak after the commit title.

Done

[Franzi, 2016-09-12 10:35:33]

lgtm

[Benedikt Meurer, 2016-09-12 10:37:16]

LGTM

[Michael Achenbach, 2016-09-12 10:39:41]

lgtm

[petermarshall, 2016-09-12 10:46:17]

The CQ bit was checked by petermarshall@chromium.org

[commit-bot: I haz the power, 2016-09-12 10:46:21]

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-09-12 10:53:41]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-12 10:54:34]

Description was changed from

==========
[builtins] Move StringNormalize to a cpp builtin.

CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Cr-Commit-Position: refs/heads/master@{#39331}
==========

to

==========
[builtins] Move StringNormalize to a cpp builtin.

CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng

BUG=v8:5364

Committed: https://crrev.com/7f84a6a2e7000bebba49354b4648346ff606ca34
Committed: https://crrev.com/e7b7ba8edd9cccd963940ac4b3e03b1e639a6418
Cr-Original-Commit-Position: refs/heads/master@{#39331}
Cr-Commit-Position: refs/heads/master@{#39342}
==========

[commit-bot: I haz the power, 2016-09-12 10:54:35]

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