Issue 5201 - [emscripten] Replace EM_ASM calls by a custom JavaScript library

Issue 5201 - [emscripten] Replace EM_ASM calls by a custom JavaScript library

[Wladimir Palant, 2017-04-30 15:16:06]

With Patch Set 2, the idea was to stop including emscripten.h everywhere.
However, turns out we still need it for the EMSCRIPTEN_KEEPALIVE macro - and we
forgot to include it in quite a few places where this macro is being used.

[sergei, 2017-05-02 09:48:06]

On 2017/04/30 15:16:06, Wladimir Palant wrote:
> With Patch Set 2, the idea was to stop including emscripten.h everywhere.
> However, turns out we still need it for the EMSCRIPTEN_KEEPALIVE macro - and
we
> forgot to include it in quite a few places where this macro is being used.

I think that having emscripten.h included in different files is in conflict with
the issue. I would expect a transition header which either includes emscripten.h
or defines empty EMSCRIPTEN_KEEPALIVE and what ever is required to work without
anything from emscription.

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/debug.h
File compiled/debug.h (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/debug.h#ne...
compiled/debug.h:20: #include <emscripten/trace.h>
should it be also wrapped by `#ifdef __EMSCRIPTEN_TRACING__`?

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js
File compiled/library.js (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js...
compiled/library.js:43: return
String.fromCharCode(charCode).toLowerCase().charCodeAt(0);
Coming back to
https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilt...
and the question about lower casing of UTF-8 (whether it maps to the same number
of bytes), I think we need to look into it later and find out a proper solution,
maybe even negotiate with filter list maintainers some constraints.

"Σ".toLowerCase() /* => ς*/.charCodeAt(0) => 962
"Σa".toLowerCase() /* => σa*/[0].charCodeAt(0) => 963

[sergei, 2017-05-02 09:48:46]

emscription -> emscripten*

[Wladimir Palant, 2017-05-03 11:57:17]

Ok, with Patch Set 3 I now made sure to abstract away all Emscripten
dependencies. I could confirm that the code is now compilable with the regular
g++ install.

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js
File compiled/library.js (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js...
compiled/library.js:43: return
String.fromCharCode(charCode).toLowerCase().charCodeAt(0);
On 2017/05/02 09:48:06, sergei wrote:
> "Σ".toLowerCase() /* => ς*/.charCodeAt(0) => 962
> "Σa".toLowerCase() /* => σa*/[0].charCodeAt(0) => 963

I cannot reproduce that - neither in Firefox nor in Chrome nor in Node. Where
did you get 962 as result?

[sergei, 2017-05-03 13:24:05]

LGTM

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js
File compiled/library.js (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js...
compiled/library.js:43: return
String.fromCharCode(charCode).toLowerCase().charCodeAt(0);
On 2017/05/03 11:57:17, Wladimir Palant wrote:
> On 2017/05/02 09:48:06, sergei wrote:
> > "Σ".toLowerCase() /* => ς*/.charCodeAt(0) => 962
> > "Σa".toLowerCase() /* => σa*/[0].charCodeAt(0) => 963
> 
> I cannot reproduce that - neither in Firefox nor in Chrome nor in Node. Where
> did you get 962 as result?

Only in Chrome but I'm on Windows, additional info
https://en.wiktionary.org/wiki/ς.
It's one of the most famous cases German ß<>SS and Turkish i which are often
recalled.

I think all implementation have some restrictions and some have even bugs, we
could simply document the restrictions in a convenient for us way.

According to ECMA-262 String.prototype.toLowerCase
(http://www.ecma-international.org/ecma-262/6.0/#sec-string.prototype.tolowercase)
uses UnicodeData.txt and locale-insensitive mappings from SpecialCasings.txt,
what versions though? We could also say that our implementation does not take
into account context, it's locale-insensitive and works only for the characters
from these particular our (e.g. based on UnicodeData.txt and SpecialCasings.txt)
files and the everything else would be officially undefined behavior. Locally we
could inspect all conversions defined in those files, check whether there are
some cases when the size after lowering/uppering is different for UTF-8,
periodically update the files in accordance to the actual results from used
libraries, etc.

[Wladimir Palant, 2017-05-04 09:14:28]

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js
File compiled/library.js (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js...
compiled/library.js:43: return
String.fromCharCode(charCode).toLowerCase().charCodeAt(0);
On 2017/05/03 13:24:04, sergei wrote:
> I think all implementation have some restrictions and some have even bugs, we
> could simply document the restrictions in a convenient for us way.

We are only using this functionality on domain names, in order to bring these
into the canonical form. Most of the time we should have the data in the
canonical form already. And I think that chances of somebody using an upper-case
Greek domain name in a filter are rather slim.

In the end, typing ΑΝΑΝΆΣ.gr into the browser produces ανανάσ.gr and not
ανανάς.gr in both Firefox and Chrome for me - so the browsers have issues here
as well (interesting that you get different results on Windows though). I guess
that anybody using such a domain name will be careful to always specify it in
the lower-cased form.

[Wladimir Palant, 2017-05-04 09:35:19]

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js
File compiled/library.js (right):

https://codereview.adblockplus.org/29425555/diff/29425584/compiled/library.js...
compiled/library.js:43: return
String.fromCharCode(charCode).toLowerCase().charCodeAt(0);
Going through the locale-insensitive mappings in
ftp://ftp.unicode.org/Public/UCD/latest/ucd/SpecialCasing.txt, it seems that
Greek sigma is the only letter that is "special" with regards to lowercasing.
All the other special cases listed only relate to uppercasing. So if we really
run into issues here, we could simply add an additional code path for this one
letter.