xfa_fm2jscontext formatting and cleanup - pt I

xfa_fm2jscontext formatting and cleanup - pt I

Committed: https://pdfium.googlesource.com/pdfium/+/48d91dd174933b4881fb500b76fb2e3ecbc7f548

[dsinclair, 2016-05-30 21:10:50]

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

[dsinclair, 2016-05-30 21:10:51]

PTAL.

I'm breaking this up in a bunch of smaller patches. Tried to do it as one but
it's too big to review. Will start adding early returns in small chunks next.

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:6514: CXFA_FM2JSContext* pContext =
I duplicated this because it will be compiled out of this branch in release mode
so we won't do the assignment at all in that case.

[Lei Zhang, 2016-05-30 23:11:00]

Looking. There are red trybots.

[Lei Zhang, 2016-05-30 23:15:59]

lgtm

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:2710: while ((nRet > kFinancialPrecision ||
nRet < -kFinancialPrecision) &&
fabs(nRet) ?

[dsinclair, 2016-05-31 13:39:51]

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:2710: while ((nRet > kFinancialPrecision ||
nRet < -kFinancialPrecision) &&
On 2016/05/30 23:15:59, Lei Zhang wrote:
> fabs(nRet) ?

We need to know if the -kFinancialPrecision < nRet < kFinancialPrecision.

I could while(fabs(nRet) > kFinancialPrecision && !bFlags) but I almost think
the current is clearer?

[Tom Sepez, 2016-05-31 16:33:17]

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jsapi.cpp (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:42: XFA_HFM2JSCONTEXT
XFA_FM2JS_ContextCreate(v8::Isolate* pScriptIsolate,
can we lose the XFA_HFM2JSCONTEXT opaque type and just make this
CXFA_FM2JSContext::Create() ?

Ditto with the rest of these -- we have a layer of abstraction here that doesn't
serve anyone.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:49: void
XFA_FM2JS_GlobalPropertyGetter(XFA_HFM2JSCONTEXT hFM2JSContext,
This ought to be static method CXFA_FM2JSContext::Getter(void* or whaterver).

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:56: void
XFA_FM2JS_ContextRelease(XFA_HFM2JSCONTEXT hFM2JSContext) {
just delete.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:408: bool PatternStringType(const
CFX_ByteStringC& szPattern,
just cut n' paste, right?

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:606: } else {
follow-up: can we make these into early returns and lose the elses?

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jscontext.h (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.h:450: CXFA_Document* m_pDocument;
probaby CXFA_Document* const m_pDocument, as I don't see these hopping between
documents during their lifespan.

[Tom Sepez, 2016-05-31 16:38:43]

On 2016/05/31 16:33:17, Tom Sepez wrote:
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jsapi.cpp (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:42: XFA_HFM2JSCONTEXT
> XFA_FM2JS_ContextCreate(v8::Isolate* pScriptIsolate,
> can we lose the XFA_HFM2JSCONTEXT opaque type and just make this
> CXFA_FM2JSContext::Create() ?
> 
> Ditto with the rest of these -- we have a layer of abstraction here that
doesn't
> serve anyone.
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:49: void
> XFA_FM2JS_GlobalPropertyGetter(XFA_HFM2JSCONTEXT hFM2JSContext,
> This ought to be static method CXFA_FM2JSContext::Getter(void* or whaterver).
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:56: void
> XFA_FM2JS_ContextRelease(XFA_HFM2JSCONTEXT hFM2JSContext) {
> just delete.
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:408: bool PatternStringType(const
> CFX_ByteStringC& szPattern,
> just cut n' paste, right?
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:606: } else {
> follow-up: can we make these into early returns and lose the elses?
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jscontext.h (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.h:450: CXFA_Document* m_pDocument;
> probaby CXFA_Document* const m_pDocument, as I don't see these hopping between
> documents during their lifespan.
Fine to do these as followups, btw. LGTM otherwise.

[Tom Sepez, 2016-05-31 16:38:44]

On 2016/05/31 16:33:17, Tom Sepez wrote:
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jsapi.cpp (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:42: XFA_HFM2JSCONTEXT
> XFA_FM2JS_ContextCreate(v8::Isolate* pScriptIsolate,
> can we lose the XFA_HFM2JSCONTEXT opaque type and just make this
> CXFA_FM2JSContext::Create() ?
> 
> Ditto with the rest of these -- we have a layer of abstraction here that
doesn't
> serve anyone.
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:49: void
> XFA_FM2JS_GlobalPropertyGetter(XFA_HFM2JSCONTEXT hFM2JSContext,
> This ought to be static method CXFA_FM2JSContext::Getter(void* or whaterver).
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:56: void
> XFA_FM2JS_ContextRelease(XFA_HFM2JSCONTEXT hFM2JSContext) {
> just delete.
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:408: bool PatternStringType(const
> CFX_ByteStringC& szPattern,
> just cut n' paste, right?
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:606: } else {
> follow-up: can we make these into early returns and lose the elses?
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> File xfa/fxfa/fm2js/xfa_fm2jscontext.h (right):
> 
>
https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
> xfa/fxfa/fm2js/xfa_fm2jscontext.h:450: CXFA_Document* m_pDocument;
> probaby CXFA_Document* const m_pDocument, as I don't see these hopping between
> documents during their lifespan.
Fine to do these as followups, btw. LGTM otherwise.

[Lei Zhang, 2016-05-31 18:03:48]

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:2710: while ((nRet > kFinancialPrecision ||
nRet < -kFinancialPrecision) &&
On 2016/05/31 13:39:51, dsinclair wrote:
> On 2016/05/30 23:15:59, Lei Zhang wrote:
> > fabs(nRet) ?
> 
> We need to know if the -kFinancialPrecision < nRet < kFinancialPrecision.
> 
> I could while(fabs(nRet) > kFinancialPrecision && !bFlags) but I almost think
> the current is clearer?

In my mind, fabs() is clearer, but I'm ok either way.

[dsinclair, 2016-05-31 18:42:40]

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:2710: while ((nRet > kFinancialPrecision ||
nRet < -kFinancialPrecision) &&
On 2016/05/31 18:03:48, Lei Zhang wrote:
> On 2016/05/31 13:39:51, dsinclair wrote:
> > On 2016/05/30 23:15:59, Lei Zhang wrote:
> > > fabs(nRet) ?
> > 
> > We need to know if the -kFinancialPrecision < nRet < kFinancialPrecision.
> > 
> > I could while(fabs(nRet) > kFinancialPrecision && !bFlags) but I almost
think
> > the current is clearer?
> 
> In my mind, fabs() is clearer, but I'm ok either way.

Done.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jsapi.cpp (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:42: XFA_HFM2JSCONTEXT
XFA_FM2JS_ContextCreate(v8::Isolate* pScriptIsolate,
On 2016/05/31 16:33:17, Tom Sepez wrote:
> can we lose the XFA_HFM2JSCONTEXT opaque type and just make this
> CXFA_FM2JSContext::Create() ?
> 
> Ditto with the rest of these -- we have a layer of abstraction here that
doesn't
> serve anyone.


Will do as a followup

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:49: void
XFA_FM2JS_GlobalPropertyGetter(XFA_HFM2JSCONTEXT hFM2JSContext,
On 2016/05/31 16:33:17, Tom Sepez wrote:
> This ought to be static method CXFA_FM2JSContext::Getter(void* or whaterver).

This will just go a way once the opaque type is removed and we can change the
callers to ->GlobalPropertyGetter.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jsapi.cpp:56: void
XFA_FM2JS_ContextRelease(XFA_HFM2JSCONTEXT hFM2JSContext) {
On 2016/05/31 16:33:17, Tom Sepez wrote:
> just delete.

Acknowledged.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:408: bool PatternStringType(const
CFX_ByteStringC& szPattern,
On 2016/05/31 16:33:17, Tom Sepez wrote:
> just cut n' paste, right?

I broke apart the if/else if chains that had returns in them.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:606: } else {
On 2016/05/31 16:33:17, Tom Sepez wrote:
> follow-up: can we make these into early returns and lose the elses?

Acknowledged.

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
File xfa/fxfa/fm2js/xfa_fm2jscontext.h (right):

https://codereview.chromium.org/2025723002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.h:450: CXFA_Document* m_pDocument;
On 2016/05/31 16:33:17, Tom Sepez wrote:
> probaby CXFA_Document* const m_pDocument, as I don't see these hopping between
> documents during their lifespan.

Done.

[dsinclair, 2016-05-31 18:42:43]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-05-31 18:42:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2025723002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-31 18:42:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025723002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2025723002/40001

[commit-bot: I haz the power, 2016-05-31 18:54:06]

Description was changed from

==========
xfa_fm2jscontext formatting and cleanup - pt I
==========

to

==========
xfa_fm2jscontext formatting and cleanup - pt I

Committed:
https://pdfium.googlesource.com/pdfium/+/48d91dd174933b4881fb500b76fb2e3ecbc7...
==========

[commit-bot: I haz the power, 2016-05-31 18:54:07]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/48d91dd174933b4881fb500b76fb2e3ecbc7...