Merge to XFA: Replace ugly JS_Define macros with templates.

Merge to XFA: Replace ugly JS_Define macros with templates.

Original Review URL: https://codereview.chromium.org/908033002

R=brucedawson@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/a116045ee959ed032e8c2eb91aa668dcd4a24156

[Tom Sepez, 2015-02-18 21:22:52]

Bruce, need a more careful review here than usual, since the XFA branch is
getting its context in a different manner than origin/master.

[Tom Sepez, 2015-02-18 21:23:27]

(Caught by run_javascript_tests.py, I might add, which now passes against this
patch).

[brucedawson, 2015-02-18 22:23:34]

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
File fpdfsdk/include/javascript/JS_Define.h (right):

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:75: IFXJS_Context* pContext =
pRuntime->GetCurrentContext();
The distinction between context and pContext is non-obvious when examining uses
of the variables. Maybe give them names that reflect their sources:
isolateContext and runtimeContext?

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:79: CJS_PropValue value(isolate);
Why are these two lines (value declaration and first usage) moved below
JS_GetPrivate? It makes verifying the behavior is unchanged more difficult.

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:99: IFXJS_Context* pContext =
pRuntime->GetCurrentContext();
The comments from above about context/pContext and moving the value declaration
and first usage apply here also.

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:136: IFXJS_Context* cc =
pRuntime->GetCurrentContext();
Should use the same name for this context variable as above.

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:474: IFXJS_Context* cc =
pRuntime->GetCurrentContext();
Should use the same name for this context variable as above.

[Tom Sepez, 2015-02-18 23:26:49]

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
File fpdfsdk/include/javascript/JS_Define.h (right):

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:75: IFXJS_Context* pContext =
pRuntime->GetCurrentContext();
On 2015/02/18 22:23:34, brucedawson wrote:
> The distinction between context and pContext is non-obvious when examining
uses
> of the variables. Maybe give them names that reflect their sources:
> isolateContext and runtimeContext?

It turns out that isolateContext, as you call it, is unused, and it doesn't look
like having a v8::local holding it for the duration provides any benefit
(searching around chromium code, other places all do something with the
context).  So I deleted this line, as it was giving compilation warnings galore

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:79: CJS_PropValue value(isolate);
On 2015/02/18 22:23:34, brucedawson wrote:
> Why are these two lines (value declaration and first usage) moved below
> JS_GetPrivate? It makes verifying the behavior is unchanged more difficult.
Ok, I'll put it back the other way.  The order in which they create these seemed
awkward in that things were being made earlier than they needed to be.

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:99: IFXJS_Context* pContext =
pRuntime->GetCurrentContext();
On 2015/02/18 22:23:34, brucedawson wrote:
> The comments from above about context/pContext and moving the value
declaration
> and first usage apply here also.

Done.

https://codereview.chromium.org/932223002/diff/20001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:136: IFXJS_Context* cc =
pRuntime->GetCurrentContext();
On 2015/02/18 22:23:34, brucedawson wrote:
> Should use the same name for this context variable as above.

Done.

[brucedawson, 2015-02-19 01:19:23]

lgtm

[Tom Sepez, 2015-02-19 18:01:00]

Committed patchset #3 (id:40001) manually as
a116045ee959ed032e8c2eb91aa668dcd4a24156 (presubmit successful).