SkPDFCanon

SkPDFCanon

SkPDFCanon's fields and methods will eventually become part of
SkPDFDocument/SkDocument_PDF.  For now, it exists as a singleton to
preflight that transition.  This replaces three global arrays in
SkPDFFont, SkPDFShader, and SkPDFGraphicsContext.  This code is still
thread-unsafe (http://skbug.com/2683), but moving this functionality
into SkPDFDocument will fix that.

This CL does not change pdf output from either GMs or SKPs.

This change also simplifies some code around the SkPDFCanon methods.

BUG=skia:2683
Committed: https://skia.googlesource.com/skia/+/fb62b3d423fa34c672df42f47017dbef087d19e9

[hal.canary, 2015-01-10 00:35:46]

halcanary@google.com changed reviewers:
+ djsollen@google.com

[hal.canary, 2015-01-10 00:35:54]

ptal

[hal.canary, 2015-01-16 16:37:22]

ping

[hal.canary, 2015-01-20 21:10:15]

halcanary@google.com changed reviewers:
+ mtklein@google.com
- djsollen@google.com

[mtklein, 2015-01-20 21:59:52]

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.cpp#newco...
src/pdf/SkPDFCanon.cpp:67: fFontRecords[i].fFont, fFontRecords[i].fFontID,
Might put all these arguments one per line, or at least group the related fontID
/ glyphID pairs?

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h
File src/pdf/SkPDFCanon.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode37
src/pdf/SkPDFCanon.h:37: // SkPDFDocument.
Let's be louder about how much of a minefield this is?

// IF YOU ARE LOOKING AT THIS API PLEASE DO NOT WRITE THE CHANGE 
// YOU ARE ABOUT TO WRITE WITHOUT TALKING TO HAL.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode42
src/pdf/SkPDFCanon.h:42: SkPDFFont* findFont(uint32_t fontID,
Add a brief comment for this?  The others look straightforward enough.

// Returns exact match if there is one.  If not, it returns NULL.
// If there is no exact match, but there is a related font, we still return
NULL, but also set *relatedFont.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFFont.h
File src/pdf/SkPDFFont.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFFont.h#newcode207
src/pdf/SkPDFFont.h:207: inline SkPDFFont::Match SkPDFFont::IsMatch(SkPDFFont*
existingFont,
This seems like an odd thing to implement inline here.  Move to SkPDFFont.cpp or
SkPDFCanon.cpp?

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h
File src/pdf/SkPDFGraphicState.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:76: bool equals(const SkPaint&) const;
If you're going to inline this, do it here?

bool equals(const SkPaint& paint) const {
  return SkPDFGraphicState::PDFPaintEquality(paint, fPaint);
}

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:97: inline const char*
SkPDFGraphicState::PDFBlendModeFromXfermode(
Same deal.  Seems like these two giant methods don't really belong.  Let's put
them in the .cpp file that calls them.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:159: inline bool
SkPDFGraphicState::PDFPaintEquality(const SkPaint& a,
I keep glazing over reading these names.. consider something like

SkPDFGraphicState::PDFPaintEquality  --> SkPDFGraphicState::Equivalent
SkPDFGraphicState::PDFBlendModeFromXfermode --> SkPDFGraphicState::AsBlendMode()

?

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:644: if (imageShader->isValid()) {
Might want to extract this if-else block as add_to_canon(SkPDFShader*) ?

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:849: const SkShader::GradientInfo* info =
&this->pdfShaderState().fInfo;
Why don't we just write "this->pdfShaderState()" as "fState"?

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.h
File src/pdf/SkPDFShader.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.h#newcode48
src/pdf/SkPDFShader.h:48: const SkPDFShaderState& pdfShaderState() const;
I am very skeptical about the number of "pdf"s in

const SkPDFShaderState& SkPDFShader::pdfShaderState() const;

Why not
  const SkPDFShader::State& SkPDFShader::state() const ?

[djsollen, 2015-01-21 14:10:27]

djsollen@google.com changed reviewers:
+ djsollen@google.com

[djsollen, 2015-01-21 14:10:27]

I echo mike's sentiment and add my 2 cents on the class definition

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h
File src/pdf/SkPDFCanon.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode68
src/pdf/SkPDFCanon.h:68: SkBaseMutex* fFontMutex;
why pointers to global variables? if the cannon is a singleton we should use the
standard SkMutex as a member variable on this object.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode74
src/pdf/SkPDFCanon.h:74: static SkPDFCanon* Create();
Any reason to expose the Create/Destroy functions in the .h file? They can just
be declared as static in the cpp file.

[mtklein, 2015-01-21 15:14:40]

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h
File src/pdf/SkPDFCanon.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode68
src/pdf/SkPDFCanon.h:68: SkBaseMutex* fFontMutex;
On 2015/01/21 14:10:27, djsollen wrote:
> why pointers to global variables? if the cannon is a singleton we should use
the
> standard SkMutex as a member variable on this object.

I missed this the first time around.  I too don't get why we need the extra
indirection.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode74
src/pdf/SkPDFCanon.h:74: static SkPDFCanon* Create();
On 2015/01/21 14:10:27, djsollen wrote:
> Any reason to expose the Create/Destroy functions in the .h file? They can
just
> be declared as static in the cpp file.

I believe this allows the constructor and destructor to be private, while
providing hooks for an SK_DECLARE_STATIC_LAZY_PTR.

[hal.canary, 2015-01-21 17:07:51]

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.cpp#newco...
src/pdf/SkPDFCanon.cpp:67: fFontRecords[i].fFont, fFontRecords[i].fFontID,
On 2015/01/20 21:59:51, mtklein wrote:
> Might put all these arguments one per line, or at least group the related
fontID
> / glyphID pairs?

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h
File src/pdf/SkPDFCanon.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode37
src/pdf/SkPDFCanon.h:37: // SkPDFDocument.
On 2015/01/20 21:59:51, mtklein wrote:
> Let's be louder about how much of a minefield this is?
> 
> // IF YOU ARE LOOKING AT THIS API PLEASE DO NOT WRITE THE CHANGE 
> // YOU ARE ABOUT TO WRITE WITHOUT TALKING TO HAL.

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode42
src/pdf/SkPDFCanon.h:42: SkPDFFont* findFont(uint32_t fontID,
On 2015/01/20 21:59:51, mtklein wrote:
> Add a brief comment for this?  The others look straightforward enough.
> 
> // Returns exact match if there is one.  If not, it returns NULL.
> // If there is no exact match, but there is a related font, we still return
> NULL, but also set *relatedFont.

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode68
src/pdf/SkPDFCanon.h:68: SkBaseMutex* fFontMutex;
On 2015/01/21 14:10:27, djsollen wrote:
> why pointers to global variables? if the cannon is a singleton we should use
the
> standard SkMutex as a member variable on this object.

I forgot to delete those.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode68
src/pdf/SkPDFCanon.h:68: SkBaseMutex* fFontMutex;
On 2015/01/21 15:14:40, mtklein wrote:
> On 2015/01/21 14:10:27, djsollen wrote:
> > why pointers to global variables? if the cannon is a singleton we should use
> the
> > standard SkMutex as a member variable on this object.
> 
> I missed this the first time around.  I too don't get why we need the extra
> indirection.

Acknowledged.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode74
src/pdf/SkPDFCanon.h:74: static SkPDFCanon* Create();
On 2015/01/21 14:10:27, djsollen wrote:
> Any reason to expose the Create/Destroy functions in the .h file? They can
just
> be declared as static in the cpp file.

I needed to because I made the constructor private.  But there's no good reason
for that, since in a later CL, SkPDFDocument will have a SkCanon as a member.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFCanon.h#newcode74
src/pdf/SkPDFCanon.h:74: static SkPDFCanon* Create();
On 2015/01/21 15:14:40, mtklein wrote:
> On 2015/01/21 14:10:27, djsollen wrote:
> > Any reason to expose the Create/Destroy functions in the .h file? They can
> just
> > be declared as static in the cpp file.
> 
> I believe this allows the constructor and destructor to be private, while
> providing hooks for an SK_DECLARE_STATIC_LAZY_PTR.

Acknowledged.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFFont.h
File src/pdf/SkPDFFont.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFFont.h#newcode207
src/pdf/SkPDFFont.h:207: inline SkPDFFont::Match SkPDFFont::IsMatch(SkPDFFont*
existingFont,
On 2015/01/20 21:59:51, mtklein wrote:
> This seems like an odd thing to implement inline here.  Move to SkPDFFont.cpp
or
> SkPDFCanon.cpp?

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h
File src/pdf/SkPDFGraphicState.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:76: bool equals(const SkPaint&) const;
On 2015/01/20 21:59:52, mtklein wrote:
> If you're going to inline this, do it here?
> 
> bool equals(const SkPaint& paint) const {
>   return SkPDFGraphicState::PDFPaintEquality(paint, fPaint);
> }

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:97: inline const char*
SkPDFGraphicState::PDFBlendModeFromXfermode(
On 2015/01/20 21:59:51, mtklein wrote:
> Same deal.  Seems like these two giant methods don't really belong.  Let's put
> them in the .cpp file that calls them.

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFGraphicState.h#...
src/pdf/SkPDFGraphicState.h:159: inline bool
SkPDFGraphicState::PDFPaintEquality(const SkPaint& a,
On 2015/01/20 21:59:52, mtklein wrote:
> I keep glazing over reading these names.. consider something like
> 
> SkPDFGraphicState::PDFPaintEquality  --> SkPDFGraphicState::Equivalent
> SkPDFGraphicState::PDFBlendModeFromXfermode -->
SkPDFGraphicState::AsBlendMode()
> 
> ?

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:644: if (imageShader->isValid()) {
On 2015/01/20 21:59:52, mtklein wrote:
> Might want to extract this if-else block as add_to_canon(SkPDFShader*) ?

GREAT idea

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:849: const SkShader::GradientInfo* info =
&this->pdfShaderState().fInfo;
On 2015/01/20 21:59:52, mtklein wrote:
> Why don't we just write "this->pdfShaderState()" as "fState"?

Done.

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.h
File src/pdf/SkPDFShader.h (right):

https://codereview.chromium.org/842253003/diff/1/src/pdf/SkPDFShader.h#newcode48
src/pdf/SkPDFShader.h:48: const SkPDFShaderState& pdfShaderState() const;
On 2015/01/20 21:59:52, mtklein wrote:
> I am very skeptical about the number of "pdf"s in
> 
> const SkPDFShaderState& SkPDFShader::pdfShaderState() const;
> 
> Why not
>   const SkPDFShader::State& SkPDFShader::state() const ?

Done.

[mtklein, 2015-01-21 17:24:38]

lgtm mod nits

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFCanon.cpp#n...
src/pdf/SkPDFCanon.cpp:105: SkDEBUGFAIL("pdfShader not found");
leftover, or something we should add to removeFont?

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFGraphicStat...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:72: bool equivilent(const SkPaint& a, const
SkPaint& b) {
speeeling

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:616: if (state.get()->fType ==
SkShader::kNone_GradientType &&
You may want to do a pass of state.get()->foo to state->foo ?

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:756: bbox.set(fShaderState.get()->fBBox);
Similiar here.  I guess a general "s/.get()->/->/g" ?

[hal.canary, 2015-01-21 17:45:30]

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFCanon.cpp#n...
src/pdf/SkPDFCanon.cpp:105: SkDEBUGFAIL("pdfShader not found");
On 2015/01/21 17:24:38, mtklein wrote:
> leftover, or something we should add to removeFont?

I noted the reason why it was left out as a comment

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFGraphicStat...
File src/pdf/SkPDFGraphicState.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFGraphicStat...
src/pdf/SkPDFGraphicState.cpp:72: bool equivilent(const SkPaint& a, const
SkPaint& b) {
On 2015/01/21 17:24:38, mtklein wrote:
> speeeling

Done.

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/842253003/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:616: if (state.get()->fType ==
SkShader::kNone_GradientType &&
On 2015/01/21 17:24:38, mtklein wrote:
> You may want to do a pass of state.get()->foo to state->foo ?

Done.

[hal.canary, 2015-01-21 17:45:35]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-21 17:46:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/842253003/40001

[commit-bot: I haz the power, 2015-01-21 17:48:00]

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

[commit-bot: I haz the power, 2015-01-21 17:48:01]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)

[hal.canary, 2015-01-21 17:51:09]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-21 17:52:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/842253003/60001

[commit-bot: I haz the power, 2015-01-21 17:59:19]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/fb62b3d423fa34c672df42f47017dbef087d19e9