Added test for util.printx() and make it match spec.

Added test for util.printx() and make it match spec.

R=dsinclair@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/4f1f41f338ce0899e48605c82375a72883f4eb7c

[Tom Sepez, 2016-03-25 18:59:05]

Dan, PTAL.

[dsinclair, 2016-03-28 18:39:01]

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
File fpdfsdk/javascript/util.cpp (right):

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:327: return input | 0x20;
Why not toupper and tolower for these?

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:353: eCaseMode = kUpperCase;
The original code did tolower for < and toupper for > was that a bug that's
fixed here?

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:374: (cSource[iSourceIndex] >= 'a' &&
cSource[iSourceIndex] <= 'z') ||
std::islower() and std::isupper() ?

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:386: if ((cSource[iSourceIndex] >= 'a' &&
cSource[iSourceIndex] <= 'z') ||
ditto

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util.h
File fpdfsdk/javascript/util.h (right):

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.h:41: static std::string printx(const std::string&
cFormat,
std::string and not one of the CFX_ByteString variants?

[Tom Sepez, 2016-03-28 19:03:42]

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
File fpdfsdk/javascript/util.cpp (right):

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:327: return input | 0x20;
On 2016/03/28 18:39:01, dsinclair wrote:
> Why not toupper and tolower for these?

ToUpper() isn't safe if |input| isn't an unsigned char value (undefined
behaviour per spec), and we're passing (possibly) signed chars here from
std::string operator[], and we might get a negative value.  Sometimes I think
the RTL is designed to hit you with gotchas on purpose.

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:353: eCaseMode = kUpperCase;
On 2016/03/28 18:39:01, dsinclair wrote:
> The original code did tolower for < and toupper for > was that a bug that's
> fixed here?

No, I botched it.  Good catch.

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:373: if (std::isdigit(cSource[iSourceIndex]) ||
This should also be avoided due to same issue.

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:374: (cSource[iSourceIndex] >= 'a' &&
cSource[iSourceIndex] <= 'z') ||
On 2016/03/28 18:39:01, dsinclair wrote:
> std::islower() and std::isupper() ?
Same issue.

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.cpp:386: if ((cSource[iSourceIndex] >= 'a' &&
cSource[iSourceIndex] <= 'z') ||
On 2016/03/28 18:39:01, dsinclair wrote:
> ditto
ditto.

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util.h
File fpdfsdk/javascript/util.h (right):

https://codereview.chromium.org/1837543002/diff/40001/fpdfsdk/javascript/util...
fpdfsdk/javascript/util.h:41: static std::string printx(const std::string&
cFormat,
On 2016/03/28 18:39:01, dsinclair wrote:
> std::string and not one of the CFX_ByteString variants?

Yes, because we're going to pass it around to various C RTL foo.

[dsinclair, 2016-03-28 19:08:47]

lgtm

[Tom Sepez, 2016-03-28 20:18:08]

On 2016/03/28 19:08:47, dsinclair wrote:
> lgtm

Please take a look at PS#5.  I've pulled the std::strings, since I was concerned
about narrowing.

[dsinclair, 2016-03-28 20:43:23]

lgtm

[Tom Sepez, 2016-03-28 20:45:47]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-03-28 20:46:00]

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

[commit-bot: I haz the power, 2016-03-28 21:13:25]

Description was changed from

==========
Added test for util.printx() and make it match spec.

R=dsinclair@chromium.org
==========

to

==========
Added test for util.printx() and make it match spec.

R=dsinclair@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/4f1f41f338ce0899e48605c82375a72883f4...
==========

[commit-bot: I haz the power, 2016-03-28 21:13:26]

Committed patchset #6 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/4f1f41f338ce0899e48605c82375a72883f4...