[runtime] Implement encodeURI as single runtime function.

[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.

BUG=v8:4912
R=yangguo@chromium.org

Committed: https://crrev.com/6502a1bfb36476810b6be2a9271db05fd8e0fb7a
Cr-Commit-Position: refs/heads/master@{#36257}

[Yang, 2016-05-12 07:39:22]

First round of comments. I see you have a new patch set uploaded, but I haven't
updated my comments to that new patch set.

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h#newcode563
src/factory.h:563: Handle<Object> NewMakeURIError() {
Can we call this NewURIError and have it call into NewError with the right
constructor and template directly?

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h#newcode584
src/factory.h:584: DECLARE_ERROR(URIError)
Let's not declare this helper.

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js
File src/js/uri.js (right):

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js#newcode230
src/js/uri.js:230: return EncodeJs(uri, true);
Let's remove this indirection and inline the content of EncodeJs here.

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js#newcode235
src/js/uri.js:235: return EncodeJs(component, false);
Same here.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
File src/runtime/runtime-strings.cc (right):

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:60: Isolate *isolate, Handle<String> subject,
Handle<String> search,
V8's convention is to have the asterisk at the type, not the variable name. "git
cl format" probably will fix this, among other things.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1020: MUST_USE_RESULT static Object *ConvertCase(
You probably did some auto format on your IDE. Please don't do that, at least
not on code unrelated to your change. And also, the formatting rules should fit
with the rest of V8.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1150: bool unescapePredicateInComponent(uint16_t
c) {
CamelCase with capital first letter please.

Also put these local functions into an anonymous namespace please.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1152: if (c < 256 && isalnum(c)) {
Let's use IsAlphaNumeric from char-predicates.h. There is also no need to
compare to 256.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1156: c == '.' || c == '_' || c == '~') {
A switch would be a lot nicer to read.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1165: if (c == '#' || c == ':' || c == ';' || c
== '/' || c == '?' || c == '$' ||
Same here, a switch would be nice.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1172: bool unescape(uint16_t c, bool is_url) {
This is only used once. Let's inline it at the call site. I.e.

if (unescapePredicateInComponent(c) || (is_url && uriSeparator(c)))

We also seem to be inconsistent about "is_url" and "is_uri" here.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1180: // Do I need lazy initialization?
No. Just declare them as static const. And using a char array (instead of int)
would suffice.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1181: int hexCharCodeArray[16] = {48, 49, 50, 51,
52, 53, 54, 55, 56, 57,  // 0..9
We have a HexCharOfValue in bignum.cc. You could move that to utils.h next to
HexValue and use that.

I'm not sure whether that one is faster than a simple table lookup though. If
this one is faster, let's replace the other implementation.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1184: result->Add(37);  // Char code of '%'.
How about result->Add('%')

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1203: // 16 bits total, cut it in 4,6, and 6 bits
whitespace after comma.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1204: uint16_t x = (cc >> 12) & 0xF;
uint8_t should suffice.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1209: octets[0] = cc;                   // ascii
same as UTF-8
Instead of collecting octets, let's just call uriAddEncodedOctetToBuffer
directly from here.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1242: size_t uriLength = uri->length();
Let's use int here.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1243: List<uint16_t> buffer;
The result can only be ASCII characters, so please use uint8_t here. As a small
optimization, we could pre-allocate the input string length, since the result is
at least that long.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1245: for (size_t k = 0; k < uriLength; k++) {
And int here.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1246: uint16_t cc1 =
uri->Get(static_cast<int>(k));
Calling Get is fairly inefficient.  We should flatten the string first and then
use a FlatStringReader. Even better would be using String::GetFlatContent and
call into a templatized helper function depending on the encoding.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1252: if (cc1 >= 0xDC00 && cc1 <= 0xDFFF) {
please do not use magic numbers here. Use unibrow::Utf16::IsTrailSurrogate

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1255: if (cc1 < 0xD800 || cc1 > 0xDBFF) {
!unibrow::Utf16::IsLeadSurrogate

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1263: if (cc2 < 0xDC00 || cc2 > 0xDFFF) {
Here as well.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1275: isolate, result,
isolate->factory()->NewRawOneByteString(totalLength));
NewStringFromAscii(buffer.ToConstVector()) would work even better.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:836: F(Encode, 2, 1)                         \
Can we choose a better name for this? Like URIEncode?

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/messages.js
File test/mjsunit/messages.js (right):

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/messages.j...
test/mjsunit/messages.js:460: var invalidUtf8 =
String.fromCharCode(leadingByte)+String.fromCharCode(continuationByte);
please adher to the 80 character limit, and whitespaces around +

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/uri.js
File test/mjsunit/uri.js (right):

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/uri.js#new...
test/mjsunit/uri.js:56: assertEquals('%7D', encodeURI(s1)); // }
We usually use two whitespaces before a comment.

[Franzi, 2016-05-12 17:35:51]

Patchset #11 (id:200001) has been deleted

[Franzi, 2016-05-13 08:19:32]

Patchset #12 (id:240001) has been deleted

[Franzi, 2016-05-13 09:02:04]

Description was changed from

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function for starters, we well probably
repackage runtime_Encode as a C++ builtin.


4912: Formatting


4912: Fix minor code style issues


4912: Delete comment


4912: Define URI Error in C++


4912: Use camelCase everywhere


4912: Rename temporary encoded list to buffer


4912: Throw MakeURIError in JS for now


4912: Update comments


4912: Remove unused JS code


4912: Remove comment


4912: Encode for single and pairs


4912: Encode single chars correctly


4912: Detect separators and non-escape characters


4912: Add comments to octet encoding


4912: Pass bool isUri to Encode for choosing correct unescapeCharacters


4912: Encode as runtime function instead of JavaScript, no functionality yet


4912: Rename test

BUG=v8:4912

R=yangguo@chromium.org
==========

to

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

[Franzi, 2016-05-13 09:10:48]

Description was changed from

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

to

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

[Franzi, 2016-05-13 09:10:48]

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

[Franzi, 2016-05-13 09:42:03]

Re-wrote encodeURI as runtime function, worked in Yang's comments.

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h
File src/factory.h (right):

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h#newcode563
src/factory.h:563: Handle<Object> NewMakeURIError() {
On 2016/05/12 07:39:20, Yang wrote:
> Can we call this NewURIError and have it call into NewError with the right
> constructor and template directly?

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/factory.h#newcode584
src/factory.h:584: DECLARE_ERROR(URIError)
On 2016/05/12 07:39:20, Yang wrote:
> Let's not declare this helper.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js
File src/js/uri.js (right):

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js#newcode230
src/js/uri.js:230: return EncodeJs(uri, true);
On 2016/05/12 07:39:20, Yang wrote:
> Let's remove this indirection and inline the content of EncodeJs here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/js/uri.js#newcode235
src/js/uri.js:235: return EncodeJs(component, false);
On 2016/05/12 07:39:20, Yang wrote:
> Same here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
File src/runtime/runtime-strings.cc (right):

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:60: Isolate *isolate, Handle<String> subject,
Handle<String> search,
On 2016/05/12 07:39:21, Yang wrote:
> V8's convention is to have the asterisk at the type, not the variable name.
"git
> cl format" probably will fix this, among other things.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1020: MUST_USE_RESULT static Object *ConvertCase(
On 2016/05/12 07:39:21, Yang wrote:
> You probably did some auto format on your IDE. Please don't do that, at least
> not on code unrelated to your change. And also, the formatting rules should
fit
> with the rest of V8.

Acknowledged.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1150: bool unescapePredicateInComponent(uint16_t
c) {
On 2016/05/12 07:39:21, Yang wrote:
> CamelCase with capital first letter please.
> 
> Also put these local functions into an anonymous namespace please.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1152: if (c < 256 && isalnum(c)) {
On 2016/05/12 07:39:21, Yang wrote:
> Let's use IsAlphaNumeric from char-predicates.h. There is also no need to
> compare to 256.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1156: c == '.' || c == '_' || c == '~') {
On 2016/05/12 07:39:21, Yang wrote:
> A switch would be a lot nicer to read.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1165: if (c == '#' || c == ':' || c == ';' || c
== '/' || c == '?' || c == '$' ||
On 2016/05/12 07:39:21, Yang wrote:
> Same here, a switch would be nice.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1172: bool unescape(uint16_t c, bool is_url) {
On 2016/05/12 07:39:21, Yang wrote:
> This is only used once. Let's inline it at the call site. I.e.
> 
> if (unescapePredicateInComponent(c) || (is_url && uriSeparator(c)))
> 
> We also seem to be inconsistent about "is_url" and "is_uri" here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1180: // Do I need lazy initialization?
On 2016/05/12 07:39:21, Yang wrote:
> No. Just declare them as static const. And using a char array (instead of int)
> would suffice.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1184: result->Add(37);  // Char code of '%'.
On 2016/05/12 07:39:21, Yang wrote:
> How about result->Add('%')

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1203: // 16 bits total, cut it in 4,6, and 6 bits
On 2016/05/12 07:39:21, Yang wrote:
> whitespace after comma.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1204: uint16_t x = (cc >> 12) & 0xF;
On 2016/05/12 07:39:21, Yang wrote:
> uint8_t should suffice.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1209: octets[0] = cc;                   // ascii
same as UTF-8
On 2016/05/12 07:39:21, Yang wrote:
> Instead of collecting octets, let's just call uriAddEncodedOctetToBuffer
> directly from here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1242: size_t uriLength = uri->length();
On 2016/05/12 07:39:20, Yang wrote:
> Let's use int here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1243: List<uint16_t> buffer;
On 2016/05/12 07:39:21, Yang wrote:
> The result can only be ASCII characters, so please use uint8_t here. As a
small
> optimization, we could pre-allocate the input string length, since the result
is
> at least that long.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1245: for (size_t k = 0; k < uriLength; k++) {
On 2016/05/12 07:39:21, Yang wrote:
> And int here.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1246: uint16_t cc1 =
uri->Get(static_cast<int>(k));
On 2016/05/12 07:39:21, Yang wrote:
> Calling Get is fairly inefficient.  We should flatten the string first and
then
> use a FlatStringReader. Even better would be using String::GetFlatContent and
> call into a templatized helper function depending on the encoding.

Done. Using Get() on FlatContent.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1252: if (cc1 >= 0xDC00 && cc1 <= 0xDFFF) {
On 2016/05/12 07:39:21, Yang wrote:
> please do not use magic numbers here. Use unibrow::Utf16::IsTrailSurrogate

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1255: if (cc1 < 0xD800 || cc1 > 0xDBFF) {
On 2016/05/12 07:39:21, Yang wrote:
> !unibrow::Utf16::IsLeadSurrogate

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1263: if (cc2 < 0xDC00 || cc2 > 0xDFFF) {
On 2016/05/12 07:39:21, Yang wrote:
> Here as well.

Done.

https://codereview.chromium.org/1968953002/diff/80001/src/runtime/runtime-str...
src/runtime/runtime-strings.cc:1275: isolate, result,
isolate->factory()->NewRawOneByteString(totalLength));
On 2016/05/12 07:39:21, Yang wrote:
> NewStringFromAscii(buffer.ToConstVector()) would work even better.

Done. I'm using NewStringFromOneByte() because buffer is a List<uint8_t>, so
less casting.

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/messages.js
File test/mjsunit/messages.js (right):

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/messages.j...
test/mjsunit/messages.js:460: var invalidUtf8 =
String.fromCharCode(leadingByte)+String.fromCharCode(continuationByte);
On 2016/05/12 07:39:21, Yang wrote:
> please adher to the 80 character limit, and whitespaces around +

Done.

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/uri.js
File test/mjsunit/uri.js (right):

https://codereview.chromium.org/1968953002/diff/80001/test/mjsunit/uri.js#new...
test/mjsunit/uri.js:56: assertEquals('%7D', encodeURI(s1)); // }
On 2016/05/12 07:39:21, Yang wrote:
> We usually use two whitespaces before a comment.

Done.

[Yang, 2016-05-13 13:25:16]

LGTM with comments. Do we have some performance numbers on this btw?

https://codereview.chromium.org/1968953002/diff/300001/src/runtime/runtime-st...
File src/runtime/runtime-strings.cc (right):

https://codereview.chromium.org/1968953002/diff/300001/src/runtime/runtime-st...
src/runtime/runtime-strings.cc:1276: }
Can we try to save some duplicate code here? Something like

for (...) {
  if (IsUnescapePredicate...) {
    buffer.Add(cc1);
    continue;
  }

  if (IsLeadSurrogate(cc1)) {
    k++;
    if (k < uri_length) {
      uc16 cc2 = uri->Get(k);
      if (IsTrailSurrogate(cc2)) {
        EncodePair(cc1, cc2...
        continue;
      }
    }
  } else if (!IsTrailSurrogate(cc1)) {
    EncodeSingle...
    continue;
  }

  AllowHeapAllocation allocate_error_and_return;
  THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewURIError());
}

[Franzi, 2016-05-14 04:42:42]

Patchset #14 (id:300001) has been deleted

[Franzi, 2016-05-14 07:13:20]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-05-14 07:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1968953002/#ps330007
(title: "Remove code duplication")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1968953002/330007
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1968953002/330007

[commit-bot: I haz the power, 2016-05-14 07:15:25]

Description was changed from

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

to

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

[commit-bot: I haz the power, 2016-05-14 07:15:26]

Committed patchset #15 (id:330007)

[commit-bot: I haz the power, 2016-05-14 07:17:07]

Description was changed from

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.


BUG=v8:4912

R=yangguo@chromium.org
==========

to

==========
[runtime] Implement encodeURI as single runtime function.

Rewrite encodeURI as runtime function. We well probably
repackage runtime_URIEncode as a C++ builtin.

BUG=v8:4912

R=yangguo@chromium.org

Committed: https://crrev.com/6502a1bfb36476810b6be2a9271db05fd8e0fb7a
Cr-Commit-Position: refs/heads/master@{#36257}
==========

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

Patchset 15 (id:??) landed as
https://crrev.com/6502a1bfb36476810b6be2a9271db05fd8e0fb7a
Cr-Commit-Position: refs/heads/master@{#36257}

[Benedikt Meurer (Google), 2016-05-15 13:21:05]

bmeurer@google.com changed reviewers:
+ bmeurer@google.com

[Benedikt Meurer (Google), 2016-05-15 13:21:06]

Very nice, LGTM. Thanks.