Make JavaScript sources compile into the library; convert JavaScript files on the fly

With this change the dependency on the current directory is gone - the library will work no matter what directory it executes in. Also, our JS sources will always load. Previously the shell had to load them explicitly - I think that shouldn`t be necessary.

[Felix Dahlke, 2013-03-15 08:07:56]

Would you mind adding Oleksandr as a reviewer? I think it makes sense to keep
all three of us in the loop.

[Wladimir Palant, 2013-03-15 08:31:14]

On 2013/03/15 08:07:56, Felix H. Dahlke wrote:
> Would you mind adding Oleksandr as a reviewer? I think it makes sense to keep
> all three of us in the loop.

Ok, added Oleksandr as reviewer here.

Please note that Patch Set 1 and Patch Set 2 are two separate changes - the
latter merely builds on the former. Also, I'll need to split up
adblockplus_compat.js in Chrome similarly once this is reviewed.

[Felix Dahlke, 2013-03-15 15:18:35]

http://codereview.adblockplus.org/9846017/diff/1/convert_js.py
File convert_js.py (right):

http://codereview.adblockplus.org/9846017/diff/1/convert_js.py#newcode5
convert_js.py:5: base_dir = os.path.abspath(os.path.dirname(__file__))
Everything else is named using camel case, why not base_dir? (I think we talked
about using underscores to follow PEP-8 more closely, but I don't mind as long
as it's consistent.)

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine.h
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:24: void Evaluate(const char* source, const char*
filename = NULL);
Have you considered having just one Evaluate function

void Evaluate(const std::string& source, const std::string& filename = "")

and checking for "" instead of NULL?

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:25: void Evaluate(const std::string& source)
I prefer to have all implementations in the .cpp file, but it's of course up to
you. However, this will lead to a compile error if included more than once as it
stands, you need to use the inline keyword for functions defined in headers.

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:40: inline v8::Handle<v8::Script>
CompileScript(const char* source, const char* filename);
You probably don't want to declare this inline. It doesn't guarantee inlining,
and you'll have to supply an identical definition in all translation units.

As a rule of thumb, it pretty much only makes sense to declare functions defined
in the header as inline (see above).

http://codereview.adblockplus.org/9846017/diff/3001/lib/info.js
File lib/info.js (right):

http://codereview.adblockplus.org/9846017/diff/3001/lib/info.js#newcode5
lib/info.js:5: exports.application = "libadblockplus";
Shouldn't this be defined by the client application, i.e. the one using
libadblockplus?

[Wladimir Palant, 2013-03-15 16:00:41]

New patchset uploaded and I think I created a mess now :-(

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine.h
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:24: void Evaluate(const char* source, const char*
filename = NULL);
On 2013/03/15 15:18:36, Felix H. Dahlke wrote:
> Have you considered having just one Evaluate function
> 
> void Evaluate(const std::string& source, const std::string& filename = "")
> 
> and checking for "" instead of NULL?

I would rather use char* for the main function - we have our JS sources as char*
and that's what we are passing to V8, converting to std::string in between
sounds pointless. Still, I can probably merge the two Evaluate functions using
std::string.

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:25: void Evaluate(const std::string& source)
On 2013/03/15 15:18:36, Felix H. Dahlke wrote:
> I prefer to have all implementations in the .cpp file, 

Yes, I tend to be a bit sloppy about that - fair point.

http://codereview.adblockplus.org/9846017/diff/3001/lib/info.js
File lib/info.js (right):

http://codereview.adblockplus.org/9846017/diff/3001/lib/info.js#newcode5
lib/info.js:5: exports.application = "libadblockplus";
On 2013/03/15 15:18:36, Felix H. Dahlke wrote:
> Shouldn't this be defined by the client application, i.e. the one using
> libadblockplus?

Yes, the entire module should be generated dynamically from data supplied by the
client application - eventually :)

[Felix Dahlke, 2013-03-15 17:10:47]

LGTM

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine.h
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/9846017/diff/1/include/AdblockPlus/JsEngine...
include/AdblockPlus/JsEngine.h:24: void Evaluate(const char* source, const char*
filename = NULL);
On 2013/03/15 16:00:41, Wladimir Palant wrote:
> On 2013/03/15 15:18:36, Felix H. Dahlke wrote:
> > Have you considered having just one Evaluate function
> > 
> > void Evaluate(const std::string& source, const std::string& filename = "")
> > 
> > and checking for "" instead of NULL?
> 
> I would rather use char* for the main function - we have our JS sources as
char*
> and that's what we are passing to V8, converting to std::string in between
> sounds pointless. Still, I can probably merge the two Evaluate functions using
> std::string.

It's up to you. std::string is obviously more C++-ish than const char*, and this
wouldn't be the only occasion where we (well, I, so far :D) wrap Google's C-y
API in a more C++-y way. The conversion overhead between char* and std::string
is neglectable in most cases.