Refactor fxjs_v8 and add embeddertests for it.

Refactor fxjs_v8 and add embeddertests for it.

This forces the layer defined by fxjs_v8.h to be (more)
self-contained, so that it can be tested apart from the
CJS_* objects (in fpdfsdk/{src,include}/javascript. This
implies the array buffer allocator must be part of fxjs_v8.

One wrinkle is that we'd like to be able to test an isolate
upon which no native objects have been added, so some
initialization that would have occurred as part of object
definition must be made explicit.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/09ed30750282bf56a92d0e646ab22c64bea81a36

[Tom Sepez, 2015-09-14 23:39:13]

Lei, for review.

[Tom Sepez, 2015-09-14 23:39:14]

Lei, for review.

[Tom Sepez, 2015-09-14 23:39:23]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2015-09-14 23:39:40]

Lei, for review.

[Tom Sepez, 2015-09-14 23:42:52]

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp:35: JS_PrepareIsolate(m_pIsolate);
note: this is the important step, since PrepareIsolate expects that the isolate
has had a data structure grafted onto it.

[Lei Zhang, 2015-09-15 01:59:55]

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
File fpdfsdk/include/jsapi/fxjs_v8.h (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:63: void JS_Initial(unsigned int
embedderDataSlot);
Can we rename this to JS_Initialize?

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:68: void JS_ResetIsolate(v8::Isolate* pIsolate);
Do we ever need to call this in normal usage?

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:111: // Called after JS_InitialRutime call made.
typo

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:351: pIsolate->SetData(g_embedderDataSlot, NULL);
I guess this is where we would call JS_ResetIsolate(), except we already called
GetData() above.

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp:54: IFXJS_Context* m_pContext;
(Just staring the at the code) Does the runtime/context ever get set to
non-NULL?

[Tom Sepez, 2015-09-15 15:56:42]

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
File fpdfsdk/include/jsapi/fxjs_v8.h (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:63: void JS_Initial(unsigned int
embedderDataSlot);
On 2015/09/15 01:59:55, Lei Zhang wrote:
> Can we rename this to JS_Initialize?

Done.

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:68: void JS_ResetIsolate(v8::Isolate* pIsolate);
On 2015/09/15 01:59:55, Lei Zhang wrote:
> Do we ever need to call this in normal usage?
As implemented, no. In an ideal world, yes, because the rules about calling
JS_DefineObj before other JS_Defines are not explicit, and its an unexpected
side-effect.

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:111: // Called after JS_InitialRutime call made.
On 2015/09/15 01:59:55, Lei Zhang wrote:
> typo

You don't like rutime? done.

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:351: pIsolate->SetData(g_embedderDataSlot, NULL);
On 2015/09/15 01:59:55, Lei Zhang wrote:
> I guess this is where we would call JS_ResetIsolate(), except we already
called
> GetData() above.
Removed JS_ResetIsolate, added comment about this doing everything.

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp (right):

https://codereview.chromium.org/1338073002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp:54: IFXJS_Context* m_pContext;
On 2015/09/15 01:59:55, Lei Zhang wrote:
> (Just staring the at the code) Does the runtime/context ever get set to
> non-NULL?

Not yet.  In the future, mock objects will need to get defined if we want to
test all the JS_Define stuff, and we'll need them then.  But that could be a
subclass of this test.  Removed.

[Tom Sepez, 2015-09-15 15:57:44]

https://codereview.chromium.org/1338073002/diff/80001/fpdfsdk/include/jsapi/f...
File fpdfsdk/include/jsapi/fxjs_v8.h (right):

https://codereview.chromium.org/1338073002/diff/80001/fpdfsdk/include/jsapi/f...
fpdfsdk/include/jsapi/fxjs_v8.h:104: void JS_InitialRuntime(v8::Isolate*
pIsolate,
I suppose you want this named "initialize", too ... hang on.

[Tom Sepez, 2015-09-15 16:00:54]

Fixed.

[Tom Sepez, 2015-09-15 16:09:48]

Lei, really ready now.

[Lei Zhang, 2015-09-15 21:00:32]

lgtm with typo fixed

https://codereview.chromium.org/1338073002/diff/120001/fpdfsdk/include/jsapi/...
File fpdfsdk/include/jsapi/fxjs_v8.h (right):

https://codereview.chromium.org/1338073002/diff/120001/fpdfsdk/include/jsapi/...
fpdfsdk/include/jsapi/fxjs_v8.h:69: // as part of JS_ReleaseRunime().
I don't like Runime either.

[Tom Sepez, 2015-09-15 21:03:42]

https://codereview.chromium.org/1338073002/diff/120001/fpdfsdk/include/jsapi/...
File fpdfsdk/include/jsapi/fxjs_v8.h (right):

https://codereview.chromium.org/1338073002/diff/120001/fpdfsdk/include/jsapi/...
fpdfsdk/include/jsapi/fxjs_v8.h:69: // as part of JS_ReleaseRunime().
On 2015/09/15 21:00:32, Lei Zhang wrote:
> I don't like Runime either.

Done.

[Tom Sepez, 2015-09-15 21:03:56]

Committed patchset #8 (id:140001) manually as
09ed30750282bf56a92d0e646ab22c64bea81a36 (presubmit successful).

[Nico, 2015-09-16 19:15:06]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-09-16 19:15:07]

This doesn't build with clang/win/gn 64bit debug builds:

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang64%28dbg%29/build...

FAILED: E:/b/depot_tools/python276_bin/python.exe gyp-win-tool link-wrapper
environment.x64 False link.exe /nologo /OUT:pdfium_embeddertests.exe
/PDB:pdfium_embeddertests.exe.pdb @pdfium_embeddertests.exe.rsp
v8.dll.lib(v8.dll) : error LNK2005: "public: __cdecl
v8::ArrayBuffer::Allocator::Allocator(void)"
(??0Allocator@ArrayBuffer@v8@@QEAA@XZ) already defined in
fxjs_v8_embeddertest.obj

v8.dll.lib(v8.dll) : error LNK2005: "public: __cdecl
v8::Isolate::Scope::Scope(class v8::Isolate *)"
(??0Scope@Isolate@v8@@QEAA@PEAV12@@Z) already defined in
fxjs_v8_embeddertest.obj

v8.dll.lib(v8.dll) : error LNK2005: "public: __cdecl
v8::Isolate::Scope::~Scope(void)" (??1Scope@Isolate@v8@@QEAA@XZ) already defined
in fxjs_v8_embeddertest.obj

v8.dll.lib(v8.dll) : error LNK2005: "public: virtual __cdecl
v8::ArrayBuffer::Allocator::~Allocator(void)"
(??1Allocator@ArrayBuffer@v8@@UEAA@XZ) already defined in
fxjs_v8_embeddertest.obj

pdfium_embeddertests.exe : fatal error LNK1169: one or more multiply defined
symbols found

(maybe it's broken with visual studio and gn in that config too, i'm not sure we
have a bot for that)