Replace ugly JS_Define macros with templates

Replace ugly JS_Define macros with templates.

This allows us to step through the JS bindings code with the
debugger, which I could not do (but wanted to) the other day.

In the process, get rid of some else after returns, and one
unreachable code path.


I also get rid of some tracing macros that we would never
use for the sake of clarity, and some plain unused
definitions.

R=brucedawson@chromium.org

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

[Tom Sepez, 2015-02-13 20:52:21]

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

[Tom Sepez, 2015-02-13 20:52:21]

Lei, for your consideration.

[Tom Sepez, 2015-02-13 20:52:39]

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

[Lei Zhang, 2015-02-14 00:00:15]

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

https://codereview.chromium.org/908033002/diff/40001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:498: if (v.IsEmpty()) {
Can we ever hit this? If we were to hit this case, does that mean we've somehow
managed to not call JS_InitialRuntime() ? If we don't return and crashing
instead, is that just a null pointer crash?

[brucedawson, 2015-02-16 20:42:00]

I'd rather see the expanding of the macros as a separate (presumably mechanical)
change in order to make it easier to see the more interesting changes.

I am definitely in favor of replacing multi-line macros with template functions.
Debuggability will be greatly improved.

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

https://codereview.chromium.org/908033002/diff/40001/fpdfsdk/include/javascri...
fpdfsdk/include/javascript/JS_Define.h:75: if (v.IsEmpty()) {
The assert/return was ambiguous in that it made it unclear whether IsEmpty() was
a valid state. Is it definitely a valid state? If not then the assert should be
retained I think. I guess I'm duplicating Lei Zhang's question, in a different
form.

[Tom Sepez, 2015-02-17 19:57:00]

On 2015/02/16 20:42:00, brucedawson wrote:
> I'd rather see the expanding of the macros as a separate (presumably
mechanical)
> change in order to make it easier to see the more interesting changes.
> 
> I am definitely in favor of replacing multi-line macros with template
functions.
> Debuggability will be greatly improved.
> 
Good idea.  Will do.

>
https://codereview.chromium.org/908033002/diff/40001/fpdfsdk/include/javascri...
> File fpdfsdk/include/javascript/JS_Define.h (right):
> 
>
https://codereview.chromium.org/908033002/diff/40001/fpdfsdk/include/javascri...
> fpdfsdk/include/javascript/JS_Define.h:75: if (v.IsEmpty()) {
> The assert/return was ambiguous in that it made it unclear whether IsEmpty()
was
> a valid state. Is it definitely a valid state? If not then the assert should
be
> retained I think. I guess I'm duplicating Lei Zhang's question, in a different
> form.
Empty handles are valid handles, but this code should never encounter one.  If
it does, we'll get a NULL deref taking the ->Value() on the next line.

[Tom Sepez, 2015-02-17 21:39:51]

Please take another look.

[brucedawson, 2015-02-17 22:43:30]

It is difficult to verify that the behavior is unchanged, but in all my checks I
couldn't find any problems.

lgtm

[Tom Sepez, 2015-02-18 20:42:57]

Committed patchset #5 (id:80001) manually as
a96cc9bffc200d4ea83c2a283ca3ac0b72e9a588 (presubmit successful).