Cleanup CPDFXFA_Document.

Cleanup CPDFXFA_Document.

Remove unused params and methods. Cleanup formatting.

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

[dsinclair, 2016-09-08 15:08:37]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 15:08:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-08 15:13:50]

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

[dsinclair, 2016-09-08 15:13:51]

PTAL.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (left):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:609: int len = bs.GetLength() / sizeof(unsigned
short);
This pattern is repeated a few times in this file. len is len/sizeof(short) but
then every time we use len we do len * sizeof(short).

The len is a int, so there will be some precision lost on rounding, but I don't
see why we'd want that?

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1027: tmp = (pos == -1) ? srcURL :
srcURL.Left(pos);
()'s aren't strictly needed but seems a bit easier to read.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1072:
pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len), (FPDF_WIDESTRING)L"", 0,
These can't static_cast because it's a unsigned short to a wide char which
static_cast doesn't like.

[commit-bot: I haz the power, 2016-09-08 15:21:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 15:21:29]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-08 16:53:37]

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (left):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:609: int len = bs.GetLength() / sizeof(unsigned
short);
On 2016/09/08 15:13:51, dsinclair wrote:
> This pattern is repeated a few times in this file. len is len/sizeof(short)
but
> then every time we use len we do len * sizeof(short).
> 
> The len is a int, so there will be some precision lost on rounding, but I
don't
> see why we'd want that?

Maybe we don't want to pass an odd number of bytes as the lengh to the callback
for a buffer that is supposed to contain shorts?

Maybe call this byteLen in that case, and lets trunc downwards once here.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:51: return CFX_FloatRect(fxRectF.left,
fxRectF.bottom(), fxRectF.right(),
The code at old line 220 flipped top and bottom. Ditto with 314.

nit: Maybe there should be a CFX_FloatRect::FromCFXRectF() static method in the
CFX_FloatRect class.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:420: menuFlag |= FXFA_MEMU_UNDO;
nit: MEMU is apparently a train in India, and is not a menu.  We should fix
this, even though its a public constant.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:756: if (!m_pSDKDoc)
nit: maybe combine with 754

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:765: FX_ARGB argb = ArgbEncode((int)alpha,
color);
nit: local argb not needed (nor are the others, but it gets kinda ugly when you
write it without alpha&color locals, but maybe that way too).
(int) cast not needed to pass uint8 to int.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:871: pEnv->FFI_OpenFile(0,
(FPDF_WIDESTRING)bs.GetBuffer(len), "rb");
pre-existing:  casting a buffer of bytestring to widestring seems wrong, except
that it is 16 bit le encoded.  See below

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1072:
pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len), (FPDF_WIDESTRING)L"", 0,
On 2016/09/08 15:13:51, dsinclair wrote:
> These can't static_cast because it's a unsigned short to a wide char which
> static_cast doesn't like.

ok, pre-existing, but casting bs to FPDF_Widestring (public, always short) seems
wrong again.  L"" is either 2 or 4 bytes of terminating 0, and passing that as a
short seems fine as its covered in either case.

I thought I made some helpers for these,
https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/fsdk_mgr.cpp?...
but it seems private.  Maybe it needs to be made visible.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1074: bs.ReleaseBuffer(len);
ReleaseBuffer isn't required as |bs| still owns the GetBuffer() result.  It's
about to go out of scope and will be freed automatically.  This just adds some
more work.  If we remove this, we don't need to know len if we use the helper
above.

Several other places.

[Tom Sepez, 2016-09-08 16:55:07]

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (left):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:609: int len = bs.GetLength() / sizeof(unsigned
short);
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> On 2016/09/08 15:13:51, dsinclair wrote:
> > This pattern is repeated a few times in this file. len is len/sizeof(short)
> but
> > then every time we use len we do len * sizeof(short).
> > 
> > The len is a int, so there will be some precision lost on rounding, but I
> don't
> > see why we'd want that?
> 
> Maybe we don't want to pass an odd number of bytes as the lengh to the
callback
> for a buffer that is supposed to contain shorts?
> 
> Maybe call this byteLen in that case, and lets trunc downwards once here.

No, actually if it's smakller, getbuffer just leaves it alone if its already big
enough (which it is).

[dsinclair, 2016-09-09 00:10:08]

On 2016/09/08 16:55:07, Tom Sepez (ooo 9 sept) wrote:
>
https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
> File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (left):
> 
>
https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
> fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:609: int len = bs.GetLength() /
sizeof(unsigned
> short);
> On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> > On 2016/09/08 15:13:51, dsinclair wrote:
> > > This pattern is repeated a few times in this file. len is
len/sizeof(short)
> > but
> > > then every time we use len we do len * sizeof(short).
> > > 
> > > The len is a int, so there will be some precision lost on rounding, but I
> > don't
> > > see why we'd want that?
> > 
> > Maybe we don't want to pass an odd number of bytes as the lengh to the
> callback
> > for a buffer that is supposed to contain shorts?
> > 
> > Maybe call this byteLen in that case, and lets trunc downwards once here.
> 
> No, actually if it's smakller, getbuffer just leaves it alone if its already
big
> enough (which it is).


So, is there a change needed here, or do you now think what I've got is correct?

[dsinclair, 2016-09-12 13:25:43]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[dsinclair, 2016-09-12 13:25:49]

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (left):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:609: int len = bs.GetLength() / sizeof(unsigned
short);
On 2016/09/08 16:55:07, Tom Sepez (ooo 9 sept) wrote:
> On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> > On 2016/09/08 15:13:51, dsinclair wrote:
> > > This pattern is repeated a few times in this file. len is
len/sizeof(short)
> > but
> > > then every time we use len we do len * sizeof(short).
> > > 
> > > The len is a int, so there will be some precision lost on rounding, but I
> > don't
> > > see why we'd want that?
> > 
> > Maybe we don't want to pass an odd number of bytes as the lengh to the
> callback
> > for a buffer that is supposed to contain shorts?
> > 
> > Maybe call this byteLen in that case, and lets trunc downwards once here.
> 
> No, actually if it's smakller, getbuffer just leaves it alone if its already
big
> enough (which it is).


So, is there anything to be done here?

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
File fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:51: return CFX_FloatRect(fxRectF.left,
fxRectF.bottom(), fxRectF.right(),
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> The code at old line 220 flipped top and bottom. Ditto with 314.
> 
> nit: Maybe there should be a CFX_FloatRect::FromCFXRectF() static method in
the
> CFX_FloatRect class.

Done.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:420: menuFlag |= FXFA_MEMU_UNDO;
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> nit: MEMU is apparently a train in India, and is not a menu.  We should fix
> this, even though its a public constant.  

Done.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:756: if (!m_pSDKDoc)
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> nit: maybe combine with 754

Done.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:765: FX_ARGB argb = ArgbEncode((int)alpha,
color);
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> nit: local argb not needed (nor are the others, but it gets kinda ugly when
you
> write it without alpha&color locals, but maybe that way too).
> (int) cast not needed to pass uint8 to int.

Done.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:871: pEnv->FFI_OpenFile(0,
(FPDF_WIDESTRING)bs.GetBuffer(len), "rb");
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> pre-existing:  casting a buffer of bytestring to widestring seems wrong,
except
> that it is 16 bit le encoded.  See below

Acknowledged.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1072:
pEnv->FFI_Alert((FPDF_WIDESTRING)bs.GetBuffer(len), (FPDF_WIDESTRING)L"", 0,
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> On 2016/09/08 15:13:51, dsinclair wrote:
> > These can't static_cast because it's a unsigned short to a wide char which
> > static_cast doesn't like.
> 
> ok, pre-existing, but casting bs to FPDF_Widestring (public, always short)
seems
> wrong again.  L"" is either 2 or 4 bytes of terminating 0, and passing that as
a
> short seems fine as its covered in either case.
> 
> I thought I made some helpers for these,
>
https://cs.chromium.org/chromium/src/third_party/pdfium/fpdfsdk/fsdk_mgr.cpp?...
> but it seems private.  Maybe it needs to be made visible.


Followup.

https://codereview.chromium.org/2322003002/diff/1/fpdfsdk/fpdfxfa/fpdfxfa_doc...
fpdfsdk/fpdfxfa/fpdfxfa_doc.cpp:1074: bs.ReleaseBuffer(len);
On 2016/09/08 16:53:36, Tom Sepez (ooo 9 sept) wrote:
> ReleaseBuffer isn't required as |bs| still owns the GetBuffer() result.  It's
> about to go out of scope and will be freed automatically.  This just adds some
> more work.  If we remove this, we don't need to know len if we use the helper
> above.
> 
> Several other places.

Done.

[commit-bot: I haz the power, 2016-09-12 13:25:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-12 13:38:14]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 13:38:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 14:16:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 14:16:40]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-09-13 13:20:15]

ping.

[Tom Sepez, 2016-09-13 16:15:28]

lgtm

[dsinclair, 2016-09-13 19:53:05]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 19:53:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 20:44:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 20:44:16]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2016-09-14 12:52:30]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 12:52:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-14 13:12:00]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-14 13:12:00]

The CQ bit was unchecked by dsinclair@chromium.org

[dsinclair, 2016-09-14 13:12:01]

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/2322003002/#ps80001
(title: "Rebase to master")

[commit-bot: I haz the power, 2016-09-14 13:12:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 13:23:51]

Description was changed from

==========
Cleanup CPDFXFA_Document.

Remove unused params and methods. Cleanup formatting.
==========

to

==========
Cleanup CPDFXFA_Document.

Remove unused params and methods. Cleanup formatting.

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

[commit-bot: I haz the power, 2016-09-14 13:23:52]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/f2662c4a0bdfbe52d961ab4d2ff9a448323c...