Issue 5332 - Support Unicode characters in path

[Manish Jethani, 2017-06-19 14:41:06]

Patch Set 1

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:112: let characters = Array.from(text);
We just need to convert the string into an array to get each of the Unicode
characters in it.

Array.from is the only option that will work with JS Hydra and Safari 9. We
cannot use the spread operator or let..of.

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:122: let c = characters[i];
Note that c.length is 2 if the character is a surrogate pair.

Previously there was a bug because we did not read Unicode characters but rather
16-bit characters.

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:193: regexp.push(c == "%" ? c : encodeURI(c));
Do not encode the percent sign because that means this is already an encoded
part of the URL. Browsers do this too.

I've added a unit test to test this case.

[kzar, 2017-07-07 12:18:07]

(Moving Sebastian to Cc.)

Except for adding that comment this LGTM.

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:112: let characters = Array.from(text);
On 2017/06/19 14:41:06, Manish Jethani wrote:
> We just need to convert the string into an array to get each of the Unicode
> characters in it.
> 
> Array.from is the only option that will work with JS Hydra and Safari 9. We
> cannot use the spread operator or let..of.

Maybe add a comment about this?

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:122: let c = characters[i];
On 2017/06/19 14:41:06, Manish Jethani wrote:
> Note that c.length is 2 if the character is a surrogate pair.
> 
> Previously there was a bug because we did not read Unicode characters but
rather
> 16-bit characters.

Acknowledged.

[Manish Jethani, 2017-07-08 06:02:20]

Patch Set 2: Add comment about Unicode surrogate pairs

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.js
File lib/abp2blocklist.js (right):

https://codereview.adblockplus.org/29468575/diff/29468576/lib/abp2blocklist.j...
lib/abp2blocklist.js:112: let characters = Array.from(text);
On 2017/07/07 12:18:07, kzar wrote:
> On 2017/06/19 14:41:06, Manish Jethani wrote:
> > We just need to convert the string into an array to get each of the Unicode
> > characters in it.
> > 
> > Array.from is the only option that will work with JS Hydra and Safari 9. We
> > cannot use the spread operator or let..of.
> 
> Maybe add a comment about this?

Done.

[kzar, 2017-07-10 12:13:51]

LGTM

(Go ahead and push this when you're ready, this one doesn't need Sebastian to
double-check.)