Remove FXJSE_Value_ToObject and call methods directly

Remove FXJSE_Value_ToObject and call methods directly

This Cl removes the global FXJSE_Value_ToObject method and adds individual
methods on CXFA_Value to do the needed conversions.

Committed: https://pdfium.googlesource.com/pdfium/+/8f3074bef53ed941daef3f32981b1449db7bcc26

[dsinclair, 2016-06-01 14:49:33]

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

[dsinclair, 2016-06-01 14:49:33]

PTAL.

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

https://codereview.chromium.org/2026993003/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3325: CFXJSE_Context* pTempContext =
Shadow'd variable.

[Tom Sepez, 2016-06-01 20:17:59]

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.cpp
File xfa/fxjse/value.cpp (right):

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.cpp#newcode11
xfa/fxjse/value.cpp:11: #include "xfa/fxfa/fm2js/xfa_fm2jscontext.h"
illegal include, in theory due to layering.

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h
File xfa/fxjse/value.h (right):

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h#newcode150
xfa/fxjse/value.h:150: CXFA_Object* ToObject(CFXJSE_Class* lpClass) const;
Layering violation.  I think you're stuck with ToHostObject() in this layer. 
You can make a helper function to do the other casts in the file where they are
used.

[dsinclair, 2016-06-01 20:34:31]

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h
File xfa/fxjse/value.h (right):

https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h#newcode150
xfa/fxjse/value.h:150: CXFA_Object* ToObject(CFXJSE_Class* lpClass) const;
On 2016/06/01 20:17:58, Tom Sepez wrote:
> Layering violation.  I think you're stuck with ToHostObject() in this layer. 
> You can make a helper function to do the other casts in the file where they
are
> used.


Why is this a violation? CXFA_Object is defined in xfa/fxfa/parser which is all
under the xfa/ tree so it should be accessible from here.

[Tom Sepez, 2016-06-01 20:37:32]

On 2016/06/01 20:34:31, dsinclair wrote:
> https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h
> File xfa/fxjse/value.h (right):
> 
> https://codereview.chromium.org/2026993003/diff/1/xfa/fxjse/value.h#newcode150
> xfa/fxjse/value.h:150: CXFA_Object* ToObject(CFXJSE_Class* lpClass) const;
> On 2016/06/01 20:17:58, Tom Sepez wrote:
> > Layering violation.  I think you're stuck with ToHostObject() in this layer.

> > You can make a helper function to do the other casts in the file where they
> are
> > used.
> 
> 
> Why is this a violation? CXFA_Object is defined in xfa/fxfa/parser which is
all
> under the xfa/ tree so it should be accessible from here.

If you look around, fxjse doesn't currently include any fxfa.
Lei and I talked about fxjse moving to a top level directory, but for the moment
you can consider fxfa a pseudo top-level directory.

[Tom Sepez, 2016-06-01 20:38:13]

> If you look around, fxjse doesn't currently include any fxfa.
> Lei and I talked about fxjse moving to a top level directory, but for the
moment
> you can consider fxfa a pseudo top-level directory.pseudo top-level directory.
Rather, consider xfa/ as such.

[dsinclair, 2016-06-02 15:27:53]

PTAL.

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script.h (right):

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script.h:50:
objArray.Add(static_cast<CXFA_Object*>(GetAt(i)->ToHostObject(nullptr)));
Can't use CXFA_ScriptContext::ToObject here because the includes become
circular.

[Tom Sepez, 2016-06-02 18:20:23]

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script.h (right):

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script.h:41: CXFA_ValueArray(v8::Isolate* pIsolate) :
m_pIsolate(pIsolate) {}
nit: explicit

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script.h:50:
objArray.Add(static_cast<CXFA_Object*>(GetAt(i)->ToHostObject(nullptr)));
On 2016/06/02 15:27:52, dsinclair wrote:
> Can't use CXFA_ScriptContext::ToObject here because the includes become
> circular.

That's usually a sign that the implementation should be in the .cpp file.

[dsinclair, 2016-06-02 18:58:05]

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
File xfa/fxfa/parser/xfa_script.h (right):

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script.h:41: CXFA_ValueArray(v8::Isolate* pIsolate) :
m_pIsolate(pIsolate) {}
On 2016/06/02 18:20:23, Tom Sepez wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2026993003/diff/20001/xfa/fxfa/parser/xfa_scr...
xfa/fxfa/parser/xfa_script.h:50:
objArray.Add(static_cast<CXFA_Object*>(GetAt(i)->ToHostObject(nullptr)));
On 2016/06/02 18:20:22, Tom Sepez wrote:
> On 2016/06/02 15:27:52, dsinclair wrote:
> > Can't use CXFA_ScriptContext::ToObject here because the includes become
> > circular.
> 
> That's usually a sign that the implementation should be in the .cpp file.

Done.

[dsinclair, 2016-06-02 18:58:13]

Patchset #3 (id:40001) has been deleted

[Tom Sepez, 2016-06-02 20:42:18]

lgtm

https://codereview.chromium.org/2026993003/diff/60001/xfa/fxfa/parser/cxfa_va...
File xfa/fxfa/parser/cxfa_valuearray.h (right):

https://codereview.chromium.org/2026993003/diff/60001/xfa/fxfa/parser/cxfa_va...
xfa/fxfa/parser/cxfa_valuearray.h:20: v8::Isolate* m_pIsolate;
nit: can this be v8::Isolate* const m_pIsolate; ?

[dsinclair, 2016-06-03 00:35:16]

https://codereview.chromium.org/2026993003/diff/60001/xfa/fxfa/parser/cxfa_va...
File xfa/fxfa/parser/cxfa_valuearray.h (right):

https://codereview.chromium.org/2026993003/diff/60001/xfa/fxfa/parser/cxfa_va...
xfa/fxfa/parser/cxfa_valuearray.h:20: v8::Isolate* m_pIsolate;
On 2016/06/02 20:42:18, Tom Sepez wrote:
> nit: can this be v8::Isolate* const m_pIsolate; ?

Done.

[dsinclair, 2016-06-03 00:35:22]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-06-03 00:35:22]

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

[commit-bot: I haz the power, 2016-06-03 00:35:29]

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

[commit-bot: I haz the power, 2016-06-03 00:45:33]

Description was changed from

==========
Remove FXJSE_Value_ToObject and call methods directly

This Cl removes the global FXJSE_Value_ToObject method and adds individual
methods on CXFA_Value to do the needed conversions.
==========

to

==========
Remove FXJSE_Value_ToObject and call methods directly

This Cl removes the global FXJSE_Value_ToObject method and adds individual
methods on CXFA_Value to do the needed conversions.

Committed:
https://pdfium.googlesource.com/pdfium/+/8f3074bef53ed941daef3f32981b1449db7b...
==========

[commit-bot: I haz the power, 2016-06-03 00:45:34]

Committed patchset #4 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/8f3074bef53ed941daef3f32981b1449db7b...