Cover fpdftext.h API with embeddertests.

Cover fpdftext.h API with embeddertests.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/526f6d5279141b270898e7e07ded7b4ce021d0d6

[Tom Sepez, 2015-01-28 01:42:33]

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

[Tom Sepez, 2015-01-28 01:43:40]

Lei, please review.  In particular, it's a nusiance to make tests against arrays
of unsigned shorts used as strings.  There may be better cases here.

[Lei Zhang, 2015-01-28 02:27:52]

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:12: static bool
check_unsigned_shorts(const char* chars,
Maybe rename this to "expected" and rename "buffer" to |actual| ?

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:14: int length) {
Why not make |length| a size_t and avoid the cast below?

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:37: FPDF_FORMHANDLE form_handle =
SetFormFillEnvironment();
Maybe the FPDFTextEmbeddertest class should have SetUp() and TearDown() 
overrides that gets |form_handle| and cleans it up?

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:43: EXPECT_EQ(30,
FPDFText_CountChars(textpage));
Can we use sizeof(expected) instead of 30 and 31?

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:45: const char* expected = "Hello,
world!\r\nGoodbye, world!";
const char expected[] = "...";

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:63: EXPECT_EQ(three_places(41.071),
three_places(left));
Why not just use EXPECT_FLOAT_EQ() and drop three_places()?

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:169: // Two occurances of "world" in test
page.
occurrences, ditto below

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:283: const char* expected_url =
"http://example.com?q=foo";
also const char foo[]

[Tom Sepez, 2015-01-28 18:12:13]

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:12: static bool
check_unsigned_shorts(const char* chars,
On 2015/01/28 02:27:52, Lei Zhang wrote:
> Maybe rename this to "expected" and rename "buffer" to |actual| ?

Done.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:14: int length) {
On 2015/01/28 02:27:52, Lei Zhang wrote:
> Why not make |length| a size_t and avoid the cast below?
Done.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:37: FPDF_FORMHANDLE form_handle =
SetFormFillEnvironment();
On 2015/01/28 02:27:51, Lei Zhang wrote:
> Maybe the FPDFTextEmbeddertest class should have SetUp() and TearDown() 
> overrides that gets |form_handle| and cleans it up?
Problem is that creating a form_handle requires a document, which isn't present
at setup() time (until we call OpenDocument()). There probably is some cleanup
here; if we assume form_handle is 1:1 with document, then we can have
OpenDocument() also create it and TearDown() close it out for us.  That would
save some boilerplate.  I'll do that as a follow-up.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:43: EXPECT_EQ(30,
FPDFText_CountChars(textpage));
On 2015/01/28 02:27:52, Lei Zhang wrote:
> Can we use sizeof(expected) instead of 30 and 31?

Done.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:45: const char* expected = "Hello,
world!\r\nGoodbye, world!";
On 2015/01/28 02:27:52, Lei Zhang wrote:
> const char expected[] = "...";
Ok, was trying to avoid a copy into the stack var, but its worth it to get
sizeof.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:63: EXPECT_EQ(three_places(41.071),
three_places(left));
On 2015/01/28 02:27:52, Lei Zhang wrote:
> Why not just use EXPECT_FLOAT_EQ() and drop three_places()?
Ah, that's almost the magic I was looking for. EXPECT_NEAR will do the trick. 
Note that I'm just guessing here about the precision we'd tolerate from changes
to pixel layout computations.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:169: // Two occurances of "world" in test
page.
On 2015/01/28 02:27:51, Lei Zhang wrote:
> occurrences, ditto below

Done.

https://codereview.chromium.org/878333003/diff/1/fpdfsdk/src/fpdftext_embedde...
fpdfsdk/src/fpdftext_embeddertest.cpp:283: const char* expected_url =
"http://example.com?q=foo";
On 2015/01/28 02:27:52, Lei Zhang wrote:
> also const char foo[]

Done.

[Lei Zhang, 2015-01-28 18:35:03]

lgtm

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
fpdfsdk/src/fpdftext_embeddertest.cpp:94: left = 0.0;
Maybe set these to some value other than 0? Ditto below at line 341 and 352.

[Tom Sepez, 2015-01-28 18:50:34]

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
fpdfsdk/src/fpdftext_embeddertest.cpp:94: left = 0.0;
On 2015/01/28 18:35:02, Lei Zhang wrote:
> Maybe set these to some value other than 0? Ditto below at line 341 and 352.
Sadly, I think these are left unchanged in face of error.  Do we want to enforce
that this is the case and that they are not zero'd?  Bad API design.

[Lei Zhang, 2015-01-28 19:27:18]

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
fpdfsdk/src/fpdftext_embeddertest.cpp:94: left = 0.0;
On 2015/01/28 18:50:34, Tom Sepez wrote:
> On 2015/01/28 18:35:02, Lei Zhang wrote:
> > Maybe set these to some value other than 0? Ditto below at line 341 and 352.
> Sadly, I think these are left unchanged in face of error.  Do we want to
enforce
> that this is the case and that they are not zero'd?  Bad API design.

Right, given the API as is, I'm just saying it would be slightly more
interesting to put a non-zero value here and in EXPECT_EQ() to test that that
the out params are not touched at all in case of failure.

[Tom Sepez, 2015-01-28 20:18:04]

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
File fpdfsdk/src/fpdftext_embeddertest.cpp (right):

https://codereview.chromium.org/878333003/diff/40001/fpdfsdk/src/fpdftext_emb...
fpdfsdk/src/fpdftext_embeddertest.cpp:94: left = 0.0;
On 2015/01/28 19:27:18, Lei Zhang wrote:
> On 2015/01/28 18:50:34, Tom Sepez wrote:
> > On 2015/01/28 18:35:02, Lei Zhang wrote:
> > > Maybe set these to some value other than 0? Ditto below at line 341 and
352.
> > Sadly, I think these are left unchanged in face of error.  Do we want to
> enforce
> > that this is the case and that they are not zero'd?  Bad API design.
> 
> Right, given the API as is, I'm just saying it would be slightly more
> interesting to put a non-zero value here and in EXPECT_EQ() to test that that
> the out params are not touched at all in case of failure.

Done.  Fascinating thing is that this API sets them to 0, while the other one
leaves them unchanged.  We should probably inflict consistency upon them at some
point down the road.

[Tom Sepez, 2015-01-28 23:49:17]

Committed patchset #5 (id:80001) manually as
526f6d5279141b270898e7e07ded7b4ce021d0d6 (presubmit successful).