Mojo: Reimplement unicode.js in JavaScript, remove native implementation.

Mojo: Reimplement unicode.js in JavaScript, remove native implementation.

This version is likely less efficient, but doesn't require gin.

BUG=372065
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275342

[Matt Perry, 2014-05-22 21:33:03]

cpu: fyi

[Matt Perry, 2014-05-27 22:11:39]

ping

[Matt Perry, 2014-05-28 17:57:29]

Adam is OOO, over to Aaron.

[Aaron Boodman, 2014-05-29 00:56:21]

Can you explain to me again what was wrong with the native version?

[darin (slow to review), 2014-05-29 03:34:46]

The idea was to make this code usable in a normal browser context. For the
mojo spy, we'd like to be able to run the mojo JS bindings over a websocket
transport. To pull that off, we can't rely on any additional native APIs.

-Darin


On Wed, May 28, 2014 at 5:56 PM, <aa@chromium.org> wrote:

> Can you explain to me again what was wrong with the native version?
>
> https://codereview.chromium.org/294123008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-05-29 18:10:23]

lgtm w/ nits.

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
File mojo/public/js/bindings/unicode.js (right):

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:18: String.fromCharCode.apply(undefined,
buffer)));
'undefined' is not a reserved word in JavaScript.  It's just undefined here
because it happens to not be defined. You mean 'null'.

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:18: String.fromCharCode.apply(undefined,
buffer)));
This is gnarly... never heard of this hack.

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:30: var utf8String =
unescape(encodeURIComponent(str));
My instinct would be to throw if the buffer isn't at least big enough (or maybe
exactly the right size?). Maybe there is a reason to silently truncate though?

[Matt Perry, 2014-05-29 19:45:08]

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
File mojo/public/js/bindings/unicode.js (right):

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:18: String.fromCharCode.apply(undefined,
buffer)));
On 2014/05/29 18:10:24, Aaron Boodman wrote:
> 'undefined' is not a reserved word in JavaScript.  It's just undefined here
> because it happens to not be defined. You mean 'null'.

Done.

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:18: String.fromCharCode.apply(undefined,
buffer)));
On 2014/05/29 18:10:24, Aaron Boodman wrote:
> This is gnarly... never heard of this hack.

Yeah it's pretty silly that it works. I found it here:
http://monsur.hossa.in/2012/07/20/utf-8-in-javascript.html

https://codereview.chromium.org/294123008/diff/1/mojo/public/js/bindings/unic...
mojo/public/js/bindings/unicode.js:30: var utf8String =
unescape(encodeURIComponent(str));
On 2014/05/29 18:10:24, Aaron Boodman wrote:
> My instinct would be to throw if the buffer isn't at least big enough (or
maybe
> exactly the right size?). Maybe there is a reason to silently truncate though?

Done.

[Matt Perry, 2014-05-29 19:45:12]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-05-29 19:47:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/294123008/20001

[commit-bot: I haz the power, 2014-05-30 00:15:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-30 00:20:42]

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

[commit-bot: I haz the power, 2014-05-30 00:20:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Matt Perry, 2014-05-30 21:35:27]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-05-30 21:41:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/294123008/40001

[Matt Perry, 2014-05-30 22:48:11]

+darin for content/ OWNERS

[commit-bot: I haz the power, 2014-05-31 02:49:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-31 02:51:46]

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

[commit-bot: I haz the power, 2014-05-31 02:51:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Matt Perry, 2014-06-02 18:36:40]

(fixing reviewer list)

[Matt Perry, 2014-06-03 21:19:32]

Ben, can you do a quick OWNERS check on content/ ?

[Ben Goodger (Google), 2014-06-05 20:22:00]

content lgtm

[Matt Perry, 2014-06-05 20:27:19]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-06-05 20:29:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/294123008/100001

[commit-bot: I haz the power, 2014-06-06 01:05:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-06 05:31:49]

Change committed as 275342