xfa_fm2jscontext method cleanup - pt IV

xfa_fm2jscontext method cleanup - pt IV

This Cl cleans up ::Num2AllTime, ::GetLocalTimeFormat, ::GetLocalDateFormat,
::DateString2Num, ::GetGMTime, ::IsoTime2Local, ::IsoDate2Local,
::Local2IsoTime, ::Local2IsoDate, ::IsIsoDateTimeFormat, ::IsIsoTimeFormat,
::IsIsoDateFormat, ::TimeFmt, ::Time2Num, ::Time, ::Num2Time, ::Num2GMTime,
::Num2Date, ::LocalTimeFmt and ::LocalDateFmt

Committed: https://pdfium.googlesource.com/pdfium/+/2b6d64eb67c23c31b29371023351b399495f23f8

[dsinclair, 2016-06-06 14:17:41]

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

[dsinclair, 2016-06-06 14:17:42]

PTAL.

[Tom Sepez, 2016-06-06 17:01:53]

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

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1231: if (argc >= 3) {
maybe should be > 2 for parallelism with line 1238

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1250: if (argc == 2) {
probably should be > 1 for parallelism with line 1237

Same throughout.  Not sure if > or >= is the preferred form, but use
consistently for all arg length comparisions.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1307: if (argc <= 0 || argc >= 4) {
argc < 0 seems impossible given getlength. 
  if (argc < 1 || argc > 3) {
seems to read the cleanest for me.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1450: if (argc <= 0 && argc >= 4) {
||

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1498: if (argc <= 0 && argc >= 4) {
||

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1566: if (argc <= 0 && argc >= 4) {
|| (doublecheck all of these).

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1709: if (*(pData + i) > '9' || *(pData + i)
< '0')
nit: just pData[i]  (throughout) Sheesh.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1794: if ((*(pData + i) > '9' || *(pData +
i) < '0') && *(pData + i) != ':') {
same here.

[dsinclair, 2016-06-06 18:07:54]

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

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1231: if (argc >= 3) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> maybe should be > 2 for parallelism with line 1238

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1250: if (argc == 2) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> probably should be > 1 for parallelism with line 1237
> 
> Same throughout.  Not sure if > or >= is the preferred form, but use
> consistently for all arg length comparisions.

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1307: if (argc <= 0 || argc >= 4) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> argc < 0 seems impossible given getlength. 
>   if (argc < 1 || argc > 3) {
> seems to read the cleanest for me.

Done. The GetLength() method returns int, so it could be negative.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1450: if (argc <= 0 && argc >= 4) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> ||

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1498: if (argc <= 0 && argc >= 4) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> ||

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1566: if (argc <= 0 && argc >= 4) {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> || (doublecheck all of these).

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1709: if (*(pData + i) > '9' || *(pData + i)
< '0')
On 2016/06/06 17:01:53, Tom Sepez wrote:
> nit: just pData[i]  (throughout) Sheesh.

Done.

https://codereview.chromium.org/2043743002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:1794: if ((*(pData + i) > '9' || *(pData +
i) < '0') && *(pData + i) != ':') {
On 2016/06/06 17:01:53, Tom Sepez wrote:
> same here.

Done.

[Tom Sepez, 2016-06-06 18:16:10]

lgtm

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

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3219: while (pData[u] == 0x20 || pData[u] ==
0x09 || pData[u] == 0x0B ||
nit: helper function.

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3320: while (pData[u] == 0x20 || pData[u] ==
0x09 || pData[u] == 0x0B ||
nit: here too.

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3334: pData[u] == 0x0C || pData[u] == 0x0A
|| pData[u] == 0x0D) {
nit: and a'here.

[dsinclair, 2016-06-06 18:27:50]

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

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3219: while (pData[u] == 0x20 || pData[u] ==
0x09 || pData[u] == 0x0B ||
On 2016/06/06 18:16:09, Tom Sepez wrote:
> nit: helper function.

Done.

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3320: while (pData[u] == 0x20 || pData[u] ==
0x09 || pData[u] == 0x0B ||
On 2016/06/06 18:16:09, Tom Sepez wrote:
> nit: here too.

Done.

https://codereview.chromium.org/2043743002/diff/50002/xfa/fxfa/fm2js/xfa_fm2j...
xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:3334: pData[u] == 0x0C || pData[u] == 0x0A
|| pData[u] == 0x0D) {
On 2016/06/06 18:16:10, Tom Sepez wrote:
> nit: and a'here.

Done.

[dsinclair, 2016-06-06 18:27:54]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-06-06 18:27:54]

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/2043743002/#ps70001
(title: " ")

[commit-bot: I haz the power, 2016-06-06 18:27:57]

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

[commit-bot: I haz the power, 2016-06-06 18:39:46]

Description was changed from

==========
xfa_fm2jscontext method cleanup - pt IV

This Cl cleans up ::Num2AllTime, ::GetLocalTimeFormat, ::GetLocalDateFormat,
::DateString2Num, ::GetGMTime, ::IsoTime2Local, ::IsoDate2Local,
::Local2IsoTime, ::Local2IsoDate, ::IsIsoDateTimeFormat, ::IsIsoTimeFormat,
::IsIsoDateFormat, ::TimeFmt, ::Time2Num, ::Time, ::Num2Time, ::Num2GMTime,
::Num2Date, ::LocalTimeFmt and ::LocalDateFmt
==========

to

==========
xfa_fm2jscontext method cleanup - pt IV

This Cl cleans up ::Num2AllTime, ::GetLocalTimeFormat, ::GetLocalDateFormat,
::DateString2Num, ::GetGMTime, ::IsoTime2Local, ::IsoDate2Local,
::Local2IsoTime, ::Local2IsoDate, ::IsIsoDateTimeFormat, ::IsIsoTimeFormat,
::IsIsoDateFormat, ::TimeFmt, ::Time2Num, ::Time, ::Num2Time, ::Num2GMTime,
::Num2Date, ::LocalTimeFmt and ::LocalDateFmt

Committed:
https://pdfium.googlesource.com/pdfium/+/2b6d64eb67c23c31b29371023351b399495f...
==========

[commit-bot: I haz the power, 2016-06-06 18:39:47]

Committed patchset #5 (id:70001) as
https://pdfium.googlesource.com/pdfium/+/2b6d64eb67c23c31b29371023351b399495f...