Fix expectations in GM PDF rendering

Update expectations for Mac PDF

R=epoger@google.com, vandebo@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=10863

[ducky, 2013-08-21 16:15:24]

Whitespace to get bots to show up
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.
.

[ducky, 2013-08-21 17:36:05]

The should fix the GM PDF rendering.
- Mac expectation names now also contain the PDF renderer backend.
- Dashes used in configName, so we have things like _pdf-poppler.png and
_pdf-mac.png.
- GM self-tests re-baselined, so the housekeeper bot doesn't die.

Hopefully this will make the Mac tests green.
In the future, it may also be worth rebaselining the other bots to add poppler
expectations.

[vandebo (ex-Chrome), 2013-08-21 17:45:33]

LGTM

[epoger, 2013-08-21 17:55:32]

LGTM, assuming it won't work for gRec.fName to return "pdf-mac" directly.

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp#newcode862
gm/gmmain.cpp:862: * @param gRec
If you make any other changes before committing, please add @param configName to
doc.  (Otherwise, I wouldn't bother.)

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp#newcode1054
gm/gmmain.cpp:1054: SkString configName(gRec.fName);
I guess it wouldn't work for gRec.fName to return "pdf-mac" in this case?

[ducky, 2013-08-21 18:01:33]

Reply to comments.

... waiting for trybots to go green now...

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp#newcode862
gm/gmmain.cpp:862: * @param gRec
On 2013/08/21 17:55:32, epoger wrote:
> If you make any other changes before committing, please add @param configName
to
> doc.  (Otherwise, I wouldn't bother.)

Why not, fixed.

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp#newcode1054
gm/gmmain.cpp:1054: SkString configName(gRec.fName);
On 2013/08/21 17:55:32, epoger wrote:
> I guess it wouldn't work for gRec.fName to return "pdf-mac" in this case?

Nope. gRec doesn't have any fields to run arbitrary functions to generate the
images (and doing such a thing would require a massive GM refactor), so the PDF
rasterizers are stored in a separate struct which contains a pointer to a
function that takes in PDF data and outputs a bitmap.

[vandebo (ex-Chrome), 2013-08-21 18:04:24]

Committed patchset #4 manually as r10863 (presubmit successful).

[epoger, 2013-08-21 18:05:20]

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/22865029/diff/11001/gm/gmmain.cpp#newcode1054
gm/gmmain.cpp:1054: SkString configName(gRec.fName);
On 2013/08/21 18:01:34, ducky wrote:
> On 2013/08/21 17:55:32, epoger wrote:
> > I guess it wouldn't work for gRec.fName to return "pdf-mac" in this case?
> 
> Nope. gRec doesn't have any fields to run arbitrary functions to generate the
> images (and doing such a thing would require a massive GM refactor), so the
PDF
> rasterizers are stored in a separate struct which contains a pointer to a
> function that takes in PDF data and outputs a bitmap.

Gotcha, thanks for the explanation.

Would it make sense for the PDF rasterizer struct to subclass gRec, and thus
various gRec accessors (including gRec.fName) would return more detailed
information in the PDF case?  I'm not suggesting making that change right now,
but I'm curious whether it would be a Good Thing.

[ducky, 2013-08-21 18:10:55]

I'm not sure if that would make things much cleaner:
- you would still have two gRec structs (one gRec indicating global configs and
one subclassed gRec containing PDF-specific data), since you probably don't want
to have multiple PDF renderers in the main gRec. It's probably also not clean to
have a pdfRenderingFunction field in the main gRec either.
- the PDF renderer doesn't care about most of the fields in gRec either, so
there would be additional copying code to populate the extra fields from the
base PDF gRec.
- and we only save one argument in one function that only gets called from one
place (well, three calls in the same function).

[epoger, 2013-08-21 18:12:14]

On 2013/08/21 18:10:55, ducky wrote:
> I'm not sure if that would make things much cleaner:
> - you would still have two gRec structs (one gRec indicating global configs
and
> one subclassed gRec containing PDF-specific data), since you probably don't
want
> to have multiple PDF renderers in the main gRec. It's probably also not clean
to
> have a pdfRenderingFunction field in the main gRec either.
> - the PDF renderer doesn't care about most of the fields in gRec either, so
> there would be additional copying code to populate the extra fields from the
> base PDF gRec.
> - and we only save one argument in one function that only gets called from one
> place (well, three calls in the same function).

Sounds like it's not worth it.  Thanks for the exploration.