xfa_fm2jscontext method cleanup - pt VI

xfa_fm2jscontext method cleanup - pt VI

Cleans up ::DecodeXML, ::DecodeHTML, ::DecodeURL, ::Decode, ::Concat, ::At,
::UnitValue, ::UnitType, ::Ref, ::Eval, ::If, ::Within.

Committed: https://pdfium.googlesource.com/pdfium/+/759de3828419522e8ecd7cdaee469e55061a7c08

[dsinclair, 2016-06-07 18:43:29]

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

[dsinclair, 2016-06-07 18:43:29]

PTAL.

[Tom Sepez, 2016-06-07 20:36:20]

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

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3047: 
nit: blank line not really needed here.  I'd put it between the 'execute and
'set lines, if anywhere at all.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3074: FXJSE_Value_IsUTF8String(argOne.get())
||
me not like the redundant calls to Is*() here.  I think I'd like this better as
a bunch of independent ifs with early returns.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3132: if
(FXJSE_Value_IsNull(unitspanValue.get())) {
Is this just cut n' paste?

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3245: std::unique_ptr<CFXJSE_Value>
unitspanValue = GetSimpleValue(pThis, args, 0);
same here?

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3513: chTemp += (ch - '0') * (!iCount ? 16 :
1);
this is stupid.  Premultiply icount by 16 and add rather than trying to scale
the operand.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3580: iCode *= 16;
Maybe as a follow-up when you consolidate these:

postmultiply seems wrong, start at zero and pre-multiply

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3583: iCode /= 16;
then you wont' have to post-divide and can avoid overflow on representable
numbers.  Nothing like a routine that can't handle the full range.

[dsinclair, 2016-06-07 20:51:48]

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

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3047: 
On 2016/06/07 20:36:19, Tom Sepez wrote:
> nit: blank line not really needed here.  I'd put it between the 'execute and
> 'set lines, if anywhere at all.

Done.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3074: FXJSE_Value_IsUTF8String(argOne.get())
||
On 2016/06/07 20:36:19, Tom Sepez wrote:
> me not like the redundant calls to Is*() here.  I think I'd like this better
as
> a bunch of independent ifs with early returns.


This currently isn't great but until we move rgValues to something other then
new'd pointers it produces the least amount of duplicating for the creating and
destroying of the value objects (line 3082 and 3117)

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3132: if
(FXJSE_Value_IsNull(unitspanValue.get())) {
On 2016/06/07 20:36:19, Tom Sepez wrote:
> Is this just cut n' paste?


I moved the early return up, out-dented and then fixed up a bunch of other early
returns?

There was a bit of cleanup in the while (u< uLen) block below I believe as well.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3245: std::unique_ptr<CFXJSE_Value>
unitspanValue = GetSimpleValue(pThis, args, 0);
On 2016/06/07 20:36:19, Tom Sepez wrote:
> same here?

ditto, early returns and cleanup.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3513: chTemp += (ch - '0') * (!iCount ? 16 :
1);
On 2016/06/07 20:36:19, Tom Sepez wrote:
> this is stupid.  Premultiply icount by 16 and add rather than trying to scale
> the operand.

Acknowledged.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3580: iCode *= 16;
On 2016/06/07 20:36:19, Tom Sepez wrote:
> Maybe as a follow-up when you consolidate these:
> 
> postmultiply seems wrong, start at zero and pre-multiply

Added todo.

https://codereview.chromium.org/2043133002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3583: iCode /= 16;
On 2016/06/07 20:36:20, Tom Sepez wrote:
> then you wont' have to post-divide and can avoid overflow on representable
> numbers.  Nothing like a routine that can't handle the full range. 

Acknowledged.

[Tom Sepez, 2016-06-07 20:57:52]

lgtm

[dsinclair, 2016-06-07 20:58:12]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-06-07 20:58:15]

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

[commit-bot: I haz the power, 2016-06-07 21:10:21]

Description was changed from

==========
xfa_fm2jscontext method cleanup - pt VI

Cleans up ::DecodeXML, ::DecodeHTML, ::DecodeURL, ::Decode, ::Concat, ::At,
::UnitValue, ::UnitType, ::Ref, ::Eval, ::If, ::Within.
==========

to

==========
xfa_fm2jscontext method cleanup - pt VI

Cleans up ::DecodeXML, ::DecodeHTML, ::DecodeURL, ::Decode, ::Concat, ::At,
::UnitValue, ::UnitType, ::Ref, ::Eval, ::If, ::Within.

Committed:
https://pdfium.googlesource.com/pdfium/+/759de3828419522e8ecd7cdaee469e55061a...
==========

[commit-bot: I haz the power, 2016-06-07 21:10:22]

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