Add CFDE_TxtEdtBuf tests

Add CFDE_TxtEdtBuf tests

This CL adds a set of unit tests for the text edit buffer. It fixes up a few
bugs found adding the tests and removes the unused ::Optimize method.

Review-Url: https://codereview.chromium.org/2614383003
Committed: https://pdfium.googlesource.com/pdfium/+/5e9066cbfa252b84d49f8b4adba445ba7761e81f

[dsinclair, 2017-01-09 20:12:21]

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

[commit-bot: I haz the power, 2017-01-09 20:12:30]

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

[dsinclair, 2017-01-09 20:12:39]

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

[dsinclair, 2017-01-09 20:12:39]

PTAL.

Any tests that I missed that you can think of?

[commit-bot: I haz the power, 2017-01-09 20:19:58]

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

[commit-bot: I haz the power, 2017-01-09 20:19:59]

Dry run: Try jobs failed on following builders:
  linux_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_rel/b...)

[dsinclair, 2017-01-09 20:37:37]

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

[commit-bot: I haz the power, 2017-01-09 20:37:48]

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, 2017-01-09 20:50:57]

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

[commit-bot: I haz the power, 2017-01-09 20:50:58]

Dry run: Try jobs failed on following builders:
  linux_xfa_asan_lsan on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_asan_...)

[dsinclair, 2017-01-09 21:02:33]

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

[commit-bot: I haz the power, 2017-01-09 21:02:38]

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

[Tom Sepez, 2017-01-09 21:07:28]

red bots?

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.cpp
File xfa/fde/cfde_txtedtbuf.cpp (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.cpp#...
xfa/fde/cfde_txtedtbuf.cpp:97: nBegin + nLength <= GetTextLength());
nit: this could overflow, but since its just an assert, meh.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.h
File xfa/fde/cfde_txtedtbuf.h (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.h#ne...
xfa/fde/cfde_txtedtbuf.h:30: void Insert(int32_t nPos, const FX_WCHAR* lpText,
int32_t nLength);
followup: I might be happier if this took a const CFX_WideStringC& rather than a
pointer/length pair.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
File xfa/fde/cfde_txtedtbuf_unittest.cpp (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:19: std::unique_ptr<CFDE_TxtEdtBuf> buf;
nit: buf_

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:22: typedef CFDE_TxtEdtBufTest
CFDE_TxtEdtBufTestDeathTest;
nit: using  CFDE_TxtEdtBufTestDeathTest = ...

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:28: EXPECT_EQ(2, buf->GetTextLength());
Nit: I'd check this first, then put the result of GetText into a local, and
since its a CFX_WideString, check its length matches, too.  Not sure why we even
have this method if we're returning a WideString that manages its own storage,
so now we have two ways to get a length ...

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:33: EXPECT_EQ(1UL, ChunkCount());
nit: we already made this check above, probably can avoid repeating it in every
test.

[commit-bot: I haz the power, 2017-01-09 21:14:28]

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

[commit-bot: I haz the power, 2017-01-09 21:14:29]

Dry run: This issue passed the CQ dry run.

[dsinclair, 2017-01-09 21:20:00]

PTAL.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.cpp
File xfa/fde/cfde_txtedtbuf.cpp (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.cpp#...
xfa/fde/cfde_txtedtbuf.cpp:97: nBegin + nLength <= GetTextLength());
On 2017/01/09 21:07:27, Tom Sepez wrote:
> nit: this could overflow, but since its just an assert, meh.

Acknowledged.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.h
File xfa/fde/cfde_txtedtbuf.h (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf.h#ne...
xfa/fde/cfde_txtedtbuf.h:30: void Insert(int32_t nPos, const FX_WCHAR* lpText,
int32_t nLength);
On 2017/01/09 21:07:27, Tom Sepez wrote:
> followup: I might be happier if this took a const CFX_WideStringC& rather than
a
> pointer/length pair.

Acknowledged.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
File xfa/fde/cfde_txtedtbuf_unittest.cpp (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:19: std::unique_ptr<CFDE_TxtEdtBuf> buf;
On 2017/01/09 21:07:28, Tom Sepez wrote:
> nit: buf_

Done.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:22: typedef CFDE_TxtEdtBufTest
CFDE_TxtEdtBufTestDeathTest;
On 2017/01/09 21:07:28, Tom Sepez wrote:
> nit: using  CFDE_TxtEdtBufTestDeathTest = ...

Done.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:28: EXPECT_EQ(2, buf->GetTextLength());
On 2017/01/09 21:07:27, Tom Sepez wrote:
> Nit: I'd check this first, then put the result of GetText into a local, and
> since its a CFX_WideString, check its length matches, too.  Not sure why we
even
> have this method if we're returning a WideString that manages its own storage,
> so now we have two ways to get a length ...

Done.

You don't have to get the text to get the length, so GetTextLength will save the
creation of a CFX_WideString from the internal buffer.

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:33: EXPECT_EQ(1UL, ChunkCount());
On 2017/01/09 21:07:27, Tom Sepez wrote:
> nit: we already made this check above, probably can avoid repeating it in
every
> test.

Done.

[Tom Sepez, 2017-01-09 21:27:10]

lgtm

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
File xfa/fde/cfde_txtedtbuf_unittest.cpp (right):

https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
xfa/fde/cfde_txtedtbuf_unittest.cpp:28: EXPECT_EQ(2, buf->GetTextLength());
On 2017/01/09 21:20:00, dsinclair wrote:
> On 2017/01/09 21:07:27, Tom Sepez wrote:
> > Nit: I'd check this first, then put the result of GetText into a local, and
> > since its a CFX_WideString, check its length matches, too.  Not sure why we
> even
> > have this method if we're returning a WideString that manages its own
storage,
> > so now we have two ways to get a length ...
> 
> Done.
> 
> You don't have to get the text to get the length, so GetTextLength will save
the
> creation of a CFX_WideString from the internal buffer.

Ok, but do we do that ever?

[dsinclair, 2017-01-09 21:28:37]

On 2017/01/09 21:27:10, Tom Sepez wrote:
> lgtm
> 
>
https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
> File xfa/fde/cfde_txtedtbuf_unittest.cpp (right):
> 
>
https://codereview.chromium.org/2614383003/diff/1/xfa/fde/cfde_txtedtbuf_unit...
> xfa/fde/cfde_txtedtbuf_unittest.cpp:28: EXPECT_EQ(2, buf->GetTextLength());
> On 2017/01/09 21:20:00, dsinclair wrote:
> > On 2017/01/09 21:07:27, Tom Sepez wrote:
> > > Nit: I'd check this first, then put the result of GetText into a local,
and
> > > since its a CFX_WideString, check its length matches, too.  Not sure why
we
> > even
> > > have this method if we're returning a WideString that manages its own
> storage,
> > > so now we have two ways to get a length ...
> > 
> > Done.
> > 
> > You don't have to get the text to get the length, so GetTextLength will save
> the
> > creation of a CFX_WideString from the internal buffer.
> 
> Ok, but do we do that ever?


I don't know. Will do a followup to remove GetTextLength() if it isn't needed.

[dsinclair, 2017-01-09 21:29:04]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2017-01-09 21:29: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, 2017-01-09 21:43:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483997344906890,
"parent_rev": "f04b7f1c438bf9f9e41a1925c6bcaa378c082ee1", "commit_rev":
"5e9066cbfa252b84d49f8b4adba445ba7761e81f"}

[commit-bot: I haz the power, 2017-01-09 21:43:10]

Description was changed from

==========
Add CFDE_TxtEdtBuf tests

This CL adds a set of unit tests for the text edit buffer. It fixes up a few
bugs found adding the tests and removes the unused ::Optimize method.
==========

to

==========
Add CFDE_TxtEdtBuf tests

This CL adds a set of unit tests for the text edit buffer. It fixes up a few
bugs found adding the tests and removes the unused ::Optimize method.

Review-Url: https://codereview.chromium.org/2614383003
Committed:
https://pdfium.googlesource.com/pdfium/+/5e9066cbfa252b84d49f8b4adba445ba7761...
==========

[commit-bot: I haz the power, 2017-01-09 21:43:11]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/5e9066cbfa252b84d49f8b4adba445ba7761...