Gold support in PDFium

Gold support in PDFium

Extends the PDFium tests to collect images and meta data to be uploaded 
to Gold. This feature is triggered by adding the --gold_* flags. 

It extends pdfium_test to output the MD5 hash of the underlying pixel 
buffer for each page it renders. 
That output is then processed by test_runner.py to generate the 
gold meta data. 

This behavior is modeled after the 'dm' tool in skia. See
https://skia.googlesource.com/skia/+/master/dm/DM.cpp#1090

This should not cause any change in the current behavior of the tests,
it will be trigger once we update the buildbot recipe. 



BUG=skia:5973
Review-Url: https://codereview.chromium.org/2578893004
Committed: https://pdfium.googlesource.com/pdfium/+/fa05e97fc4d796bd12e21c32634d9614f8edf607

[stephana, 2016-12-19 15:57:40]

Description was changed from

==========
WIP: Gold support in PDFium

BUG=
==========

to

==========
Gold support in PDFium


BUG=skia:5973
==========

[stephana, 2016-12-19 16:06:16]

Description was changed from

==========
Gold support in PDFium


BUG=skia:5973
==========

to

==========
Gold support in PDFium

Extends the PDFium tests to collect images and meta data to be uploaded 
to Gold. This feature is triggered by adding the --gold_* flags. 

It extends pdfium_test to output the MD5 hash of the underlying pixel 
buffer for each page it renders. 
That output is then processed by test_runner.py to generate the 
gold meta data. 

This behavior is modeled after the 'dm' tool in skia. See
https://skia.googlesource.com/skia/+/master/dm/DM.cpp#1090

This should not cause any change in the current behavior of the tests,
it will be trigger once we update the buildbot recipe. 



BUG=skia:5973
==========

[stephana, 2016-12-19 16:06:16]

stephana@google.com changed reviewers:
+ dsinclair@chromium.org

[stephana, 2016-12-19 16:12:58]

The CQ bit was checked by stephana@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 16:13:04]

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-12-19 16:13:05]

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

[commit-bot: I haz the power, 2016-12-19 16:13:06]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[caryclark, 2016-12-19 16:14:20]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 16:14:22]

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-12-19 16:17:12]

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

[commit-bot: I haz the power, 2016-12-19 16:17:13]

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

[stephana, 2016-12-19 20:32:14]

The CQ bit was checked by stephana@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:32:16]

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-12-19 20:32:17]

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

[commit-bot: I haz the power, 2016-12-19 20:32:18]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[stephana, 2016-12-20 00:56:01]

The CQ bit was checked by stephana@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 00:56:04]

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-12-20 00:56:05]

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

[commit-bot: I haz the power, 2016-12-20 00:56:05]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[stephana, 2016-12-20 22:21:18]

PTAL 

This is the first of probably 3 CLs that will enable Gold for PDFium. This just
adds behavior that will be used by the buildbot recipes later. None of the
current tests should fail or behave differently.

[caryclark, 2016-12-20 22:42:59]

looks good with nits

I'll defer to dsinclair (or anyone else, for that matter) on the python -- not
anything I know about. Maybe someone on Skia infra or Chrome infra should be on
the review?

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc...
samples/pdfium_test.cc:119: printf("%02x", digest[i]);
pdfium style doesn't require braces here

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc...
samples/pdfium_test.cc:703: OutputMD5Hash(image_file_name.c_str(), buffer,
stride * height);
Maybe add a new command line flag that permits this printf, so existing clients
don't see it? (also, no brace needed here either)

[stephana, 2016-12-21 16:12:06]

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc...
samples/pdfium_test.cc:119: printf("%02x", digest[i]);
On 2016/12/20 22:42:59, caryclark wrote:
> pdfium style doesn't require braces here

Done.

https://codereview.chromium.org/2578893004/diff/140001/samples/pdfium_test.cc...
samples/pdfium_test.cc:703: OutputMD5Hash(image_file_name.c_str(), buffer,
stride * height);
On 2016/12/20 22:42:59, caryclark wrote:
> Maybe add a new command line flag that permits this printf, so existing
clients
> don't see it? (also, no brace needed here either)

Done.

[stephana, 2016-12-21 19:16:48]

stephana@google.com changed reviewers:
+ borenet@google.com

[stephana, 2016-12-21 19:17:55]

Adding borenet@ to take a look at the Python part of the CL.

[borenet, 2016-12-21 19:47:29]

Mostly just style nits.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/common.py
File testing/tools/common.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/common.p...
testing/tools/common.py:28: # RunCommandExtractHashedFiles returns a tuple:
(raised_exception, hashed_files)
Isn't it more idiomatic just to let the exception propagate?

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py
File testing/tools/gold.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:4: 
Style nit: 2 spaces between anything at the top level.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:90: kvPairs = kvStr.split()
I recommend shlex.split, if kvPairs might include quoted strings with spaces in
them.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:106: if __name__ == "__main__":
I prefer to put the below lines into a function since any variables declared in
this block become file-global.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:116: propStr = "build_number 2 builder Builder-Name
gitHash a4a338179013b029d6dd55e737b5bd648a9fb68c"
Nit: line break

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
File testing/tools/test_runner.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:153: help='Key value pairs that are written to the
top level of the JSON file that is ingested by Gold.')
Nit: Line break.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:157: help='Path of where to write the JSON output
to be uploaded to Gold.')
Nit: Line break.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:159: help='Prevents the return value from being
non-zero when image comparison fails.')
Nit: Line break.

[stephana, 2016-12-21 21:47:58]

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/common.py
File testing/tools/common.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/common.p...
testing/tools/common.py:28: # RunCommandExtractHashedFiles returns a tuple:
(raised_exception, hashed_files)
On 2016/12/21 19:47:28, borenet wrote:
> Isn't it more idiomatic just to let the exception propagate?

I agree, but I wanted the changes to be minimal to avoid breaking 
the already working tests, so I kept with the pattern for the existing 
codebase.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py
File testing/tools/gold.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:4: 
On 2016/12/21 19:47:28, borenet wrote:
> Style nit: 2 spaces between anything at the top level.

Done.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:90: kvPairs = kvStr.split()
On 2016/12/21 19:47:28, borenet wrote:
> I recommend shlex.split, if kvPairs might include quoted strings with spaces
in
> them.

Done.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:106: if __name__ == "__main__":
On 2016/12/21 19:47:28, borenet wrote:
> I prefer to put the below lines into a function since any variables declared
in
> this block become file-global.

I don't think so. If I run this from another file:

import gold 
gold.testDir 

The second line will cause an error.
I might misunderstand what you mean though.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/gold.py#...
testing/tools/gold.py:116: propStr = "build_number 2 builder Builder-Name
gitHash a4a338179013b029d6dd55e737b5bd648a9fb68c"
On 2016/12/21 19:47:28, borenet wrote:
> Nit: line break

Done.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
File testing/tools/test_runner.py (right):

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:153: help='Key value pairs that are written to the
top level of the JSON file that is ingested by Gold.')
On 2016/12/21 19:47:29, borenet wrote:
> Nit: Line break.

Done.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:157: help='Path of where to write the JSON output
to be uploaded to Gold.')
On 2016/12/21 19:47:28, borenet wrote:
> Nit: Line break.

Done.

https://codereview.chromium.org/2578893004/diff/180001/testing/tools/test_run...
testing/tools/test_runner.py:159: help='Prevents the return value from being
non-zero when image comparison fails.')
On 2016/12/21 19:47:28, borenet wrote:
> Nit: Line break.

Done.

[dsinclair, 2017-01-02 14:04:57]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2017-01-02 14:04:57]

lgtm

[commit-bot: I haz the power, 2017-01-02 14:05:05]

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-02 14:19:42]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483365897247610,
"parent_rev": "aea77059d309820dbcea9ec3e583fa673960a0b9", "commit_rev":
"fa05e97fc4d796bd12e21c32634d9614f8edf607"}

[commit-bot: I haz the power, 2017-01-02 14:19:46]

Description was changed from

==========
Gold support in PDFium

Extends the PDFium tests to collect images and meta data to be uploaded 
to Gold. This feature is triggered by adding the --gold_* flags. 

It extends pdfium_test to output the MD5 hash of the underlying pixel 
buffer for each page it renders. 
That output is then processed by test_runner.py to generate the 
gold meta data. 

This behavior is modeled after the 'dm' tool in skia. See
https://skia.googlesource.com/skia/+/master/dm/DM.cpp#1090

This should not cause any change in the current behavior of the tests,
it will be trigger once we update the buildbot recipe. 



BUG=skia:5973
==========

to

==========
Gold support in PDFium

Extends the PDFium tests to collect images and meta data to be uploaded 
to Gold. This feature is triggered by adding the --gold_* flags. 

It extends pdfium_test to output the MD5 hash of the underlying pixel 
buffer for each page it renders. 
That output is then processed by test_runner.py to generate the 
gold meta data. 

This behavior is modeled after the 'dm' tool in skia. See
https://skia.googlesource.com/skia/+/master/dm/DM.cpp#1090

This should not cause any change in the current behavior of the tests,
it will be trigger once we update the buildbot recipe. 



BUG=skia:5973

Review-Url: https://codereview.chromium.org/2578893004
Committed:
https://pdfium.googlesource.com/pdfium/+/fa05e97fc4d796bd12e21c32634d9614f8ed...
==========

[commit-bot: I haz the power, 2017-01-02 14:19:46]

Committed patchset #12 (id:220001) as
https://pdfium.googlesource.com/pdfium/+/fa05e97fc4d796bd12e21c32634d9614f8ed...