Create first pdfium embedder test.

Create first pdfium embedder test.

BUG=https://code.google.com/p/pdfium/issues/detail?id=62
R=jam@chromium.org

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

[Tom Sepez, 2015-01-13 00:19:18]

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

[Tom Sepez, 2015-01-13 00:19:18]

John, please review.

[jam, 2015-01-13 19:21:18]

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_embeddert...
File testing/pdfium_embeddertest.cpp (right):

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_embeddert...
testing/pdfium_embeddertest.cpp:8: class PDFiumEmbeddertest : public
PDFiumTestEmbedder {
it's confusing having both PDFiumEmbeddertest and PDFiumTestEmbedder

I would have thought that the test harness would be called PDFiumEmbedderTest
(i.e. analogous to UnitTest or BrowserTest), or even just EmbedderTest (since
pdfium is redundant in this repo). this class could then be called BasicTest or
something similar

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
File testing/pdfium_test_embedder.cpp (right):

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.cpp:246: FPDF_FORMHANDLE form =
FPDFDOC_InitFormFillEnvironment(document_, &form_callbacks);
nit: 80 chars

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.cpp:287: FPDF_BITMAP
PDFiumTestEmbedder::RenderPage(FPDF_PAGE page, FPDF_FORMHANDLE form) {
ditto

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
File testing/pdfium_test_embedder.h (right):

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.h:17: class PDFiumTestEmbedder : public
::testing::Test {
nit: add documentation about what this class is used to test

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.h:36: virtual bool OpenDocument(const std::string&
filename);
nit: add documentation for these methods

[Tom Sepez, 2015-01-15 20:42:32]

> it's confusing having both PDFiumEmbeddertest and PDFiumTestEmbedder
> 
> I would have thought that the test harness would be called PDFiumEmbedderTest
> (i.e. analogous to UnitTest or BrowserTest), or even just EmbedderTest (since
> pdfium is redundant in this repo). this class could then be called BasicTest
or
> something similar
> 
I went back and forth a few times trying to get my head around the whole
"adjective noun" naming convention, and wasn't happy either way. I'll go with
your naming suggestions.

[Tom Sepez, 2015-01-15 21:24:00]

John, please review PS#4.

[Tom Sepez, 2015-01-15 21:24:40]

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
File testing/pdfium_test_embedder.cpp (right):

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.cpp:246: FPDF_FORMHANDLE form =
FPDFDOC_InitFormFillEnvironment(document_, &form_callbacks);
On 2015/01/13 19:21:18, jam wrote:
> nit: 80 chars

Done.

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.cpp:287: FPDF_BITMAP
PDFiumTestEmbedder::RenderPage(FPDF_PAGE page, FPDF_FORMHANDLE form) {
On 2015/01/13 19:21:18, jam wrote:
> ditto

Done.

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
File testing/pdfium_test_embedder.h (right):

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.h:17: class PDFiumTestEmbedder : public
::testing::Test {
On 2015/01/13 19:21:18, jam wrote:
> nit: add documentation about what this class is used to test

Done.

https://codereview.chromium.org/827733006/diff/20001/testing/pdfium_test_embe...
testing/pdfium_test_embedder.h:36: virtual bool OpenDocument(const std::string&
filename);
On 2015/01/13 19:21:18, jam wrote:
> nit: add documentation for these methods

Done.

[jam, 2015-01-15 23:28:27]

lgtm

for now, it's ok to keep that small pdf file there. but eventually when we have
a lot of files, we will probably want it in another repo so that chromium
doesn't have to pull this in

[Tom Sepez, 2015-01-16 00:31:55]

Committed patchset #4 (id:60001) manually as
d0edcea1a1b22813f593022ca29fca79074757bf (presubmit successful).

[Bo Xu, 2015-01-22 21:30:37]

On 2015/01/16 00:31:55, Tom Sepez wrote:
> Committed patchset #4 (id:60001) manually as
> d0edcea1a1b22813f593022ca29fca79074757bf (presubmit successful).

Do we need to add this to Build.gn?

[Tom Sepez, 2015-01-22 21:31:51]

On 2015/01/22 21:30:37, Bo Xu wrote:
> On 2015/01/16 00:31:55, Tom Sepez wrote:
> > Committed patchset #4 (id:60001) manually as
> > d0edcea1a1b22813f593022ca29fca79074757bf (presubmit successful).
> 
> Do we need to add this to Build.gn?

Yes, eventually.