Remove various FXJSE Value methods.

Remove various FXJSE Value methods.

This CL removes the FXJSE_Value_Set*, FXJSE_Value_To*, FXJSE_Value_Get* and
FXJSE_Value_Delete* methods.

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

[dsinclair, 2016-06-07 21:36:14]

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

[dsinclair, 2016-06-07 21:36:15]

PTAL.

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

https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode12
xfa/fxjse/value.h:12: V8_INLINE static double FXJSE_ftod(FX_FLOAT fNumber) {
Moved because SetFloat moved. See below.

https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode219
xfa/fxjse/value.h:219: V8_INLINE void SetFloat(FX_FLOAT fFloat) {
This had to be moved here in order to make things compile. I'm not sure what the
deal is with V8_INLINE.

[Tom Sepez, 2016-06-07 21:51:49]

Wow, this is so much better.  Maybe Lei has an idea about your inline problem. 
Just some suggestions for follow-ups and some nits.

LGTM otherwise.

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
File xfa/fxfa/app/xfa_ffwidgetacc.cpp (right):

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
xfa/fxfa/app/xfa_ffwidgetacc.cpp:663: pTmpRetValue->ToString(bsString);
Can we make ToString() return a string rather than taking an out param while
we're at it?  Or is that too much change?

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

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:636: argValue->GetObjectProperty("length",
lengthValue.get());
pity this doesn't manufacture the new value and return it as a unique_ptr. 
Follow-up.

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/parser/xfa_object_...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/parser/xfa_object_...
xfa/fxfa/parser/xfa_object_imp.cpp:719:
pValue->SetObject((CXFA_Object*)pNodeList,
nit: These casts to CXFA_Object shouldn't be required since NodeList inherits
from it (several places).

[Lei Zhang, 2016-06-08 01:27:43]

On 2016/06/07 21:36:15, dsinclair wrote:
> https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode219
> xfa/fxjse/value.h:219: V8_INLINE void SetFloat(FX_FLOAT fFloat) {
> This had to be moved here in order to make things compile. I'm not sure what
the
> deal is with V8_INLINE.

I'm also curious why this has to be inline.

[dsinclair, 2016-06-08 01:51:08]

On 2016/06/08 01:27:43, Lei Zhang (Slow) wrote:
> On 2016/06/07 21:36:15, dsinclair wrote:
> >
https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode219
> > xfa/fxjse/value.h:219: V8_INLINE void SetFloat(FX_FLOAT fFloat) {
> > This had to be moved here in order to make things compile. I'm not sure what
> the
> > deal is with V8_INLINE.
> 
> I'm also curious why this has to be inline.


../../xfa/fxjse/value.h:179:18: error: inline function 'CFXJSE_Value::SetFloat'
is not defined [-Werror,-Wundefined-inline]
  V8_INLINE void SetFloat(FX_FLOAT fFloat);
                 ^
../../xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:618:26: note: used here
  args.GetReturnValue()->SetFloat(

[dsinclair, 2016-06-08 02:01:41]

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
File xfa/fxfa/app/xfa_ffwidgetacc.cpp (right):

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/app/xfa_ffwidgetac...
xfa/fxfa/app/xfa_ffwidgetacc.cpp:663: pTmpRetValue->ToString(bsString);
On 2016/06/07 21:51:48, Tom Sepez wrote:
> Can we make ToString() return a string rather than taking an out param while
> we're at it?  Or is that too much change?

On my todo list.

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

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:636: argValue->GetObjectProperty("length",
lengthValue.get());
On 2016/06/07 21:51:48, Tom Sepez wrote:
> pity this doesn't manufacture the new value and return it as a unique_ptr. 
> Follow-up.

Acknowledged.

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/parser/xfa_object_...
File xfa/fxfa/parser/xfa_object_imp.cpp (right):

https://codereview.chromium.org/2043153002/diff/1/xfa/fxfa/parser/xfa_object_...
xfa/fxfa/parser/xfa_object_imp.cpp:719:
pValue->SetObject((CXFA_Object*)pNodeList,
On 2016/06/07 21:51:48, Tom Sepez wrote:
> nit: These casts to CXFA_Object shouldn't be required since NodeList inherits
> from it (several places).

Done.

[Lei Zhang, 2016-06-08 02:03:28]

On 2016/06/08 01:51:08, dsinclair wrote:
> On 2016/06/08 01:27:43, Lei Zhang (Slow) wrote:
> > On 2016/06/07 21:36:15, dsinclair wrote:
> > >
> https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode219
> > > xfa/fxjse/value.h:219: V8_INLINE void SetFloat(FX_FLOAT fFloat) {
> > > This had to be moved here in order to make things compile. I'm not sure
what
> > the
> > > deal is with V8_INLINE.
> > 
> > I'm also curious why this has to be inline.
> 
> 
> ../../xfa/fxjse/value.h:179:18: error: inline function
'CFXJSE_Value::SetFloat'
> is not defined [-Werror,-Wundefined-inline]
>   V8_INLINE void SetFloat(FX_FLOAT fFloat);
>                  ^
> ../../xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:618:26: note: used here
>   args.GetReturnValue()->SetFloat(

Ya, I applied the CL locally and saw that. What I mean is, do we really need
V8_INLINE?

[dsinclair, 2016-06-08 02:06:42]

On 2016/06/08 02:03:28, Lei Zhang (Slow) wrote:
> On 2016/06/08 01:51:08, dsinclair wrote:
> > On 2016/06/08 01:27:43, Lei Zhang (Slow) wrote:
> > > On 2016/06/07 21:36:15, dsinclair wrote:
> > > >
> >
https://codereview.chromium.org/2043153002/diff/1/xfa/fxjse/value.h#newcode219
> > > > xfa/fxjse/value.h:219: V8_INLINE void SetFloat(FX_FLOAT fFloat) {
> > > > This had to be moved here in order to make things compile. I'm not sure
> what
> > > the
> > > > deal is with V8_INLINE.
> > > 
> > > I'm also curious why this has to be inline.
> > 
> > 
> > ../../xfa/fxjse/value.h:179:18: error: inline function
> 'CFXJSE_Value::SetFloat'
> > is not defined [-Werror,-Wundefined-inline]
> >   V8_INLINE void SetFloat(FX_FLOAT fFloat);
> >                  ^
> > ../../xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:618:26: note: used here
> >   args.GetReturnValue()->SetFloat(
> 
> Ya, I applied the CL locally and saw that. What I mean is, do we really need
> V8_INLINE?


I'll file a bug and follow up with Jochen. Is this good otherwise?

[Lei Zhang, 2016-06-08 02:18:34]

On 2016/06/08 02:06:42, dsinclair wrote:
> On 2016/06/08 02:03:28, Lei Zhang (Slow) wrote:
> > Ya, I applied the CL locally and saw that. What I mean is, do we really need
> > V8_INLINE?
> 
> I'll file a bug and follow up with Jochen. Is this good otherwise?

Not sure if Jochen knows about this. The code came from commit fdc00a7.

No objections from me.

[dsinclair, 2016-06-08 02:35:58]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-06-08 02:35:58]

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/2043153002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-06-08 02:36:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043153002/20001

[commit-bot: I haz the power, 2016-06-08 02:36:22]

Description was changed from

==========
Remove various FXJSE Value methods.

This CL removes the FXJSE_Value_Set*, FXJSE_Value_To*, FXJSE_Value_Get* and
FXJSE_Value_Delete* methods.
==========

to

==========
Remove various FXJSE Value methods.

This CL removes the FXJSE_Value_Set*, FXJSE_Value_To*, FXJSE_Value_Get* and
FXJSE_Value_Delete* methods.

Committed:
https://pdfium.googlesource.com/pdfium/+/f27aeec59186e7d11a3a2167572617fd5ed4...
==========

[commit-bot: I haz the power, 2016-06-08 02:36:23]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/f27aeec59186e7d11a3a2167572617fd5ed4...