[PDF] Fix bug in graphic state comparison.

[PDF] Fix bug in graphic state comparison.

SkPDFGraphicState::GSCanonicalEntry::operator== was out of sync with SkPDFGraphicState::populateDict  leading to graphic state objects with the same value.

Committed: http://code.google.com/p/skia/source/detail?r=1278

[Chris Guillory, 2011-05-09 08:37:46]

LGTM. Some comments.

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp
File src/pdf/SkPDFGraphicState.cpp (right):

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:191: const char* aXfermodeString =
blendModeFromXfermode(aXfermodeName);
Nit: Almost seems like we want to assert that aXfermodeString isn't NULL.

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
bXfermodeString) == 0;
Nit: !strcmp

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
bXfermodeString) == 0;
Optional: Can we get away with just doing a pointer comparison here? Maybe
update the comments of blendModeFromXfermode to indicate that it returns the
address of a string literal constant that can be used for this type of
comparison.

[Chris Guillory, 2011-05-09 08:38:10]

On 2011/05/09 08:37:46, Chris Guillory wrote:
> LGTM. Some comments.
> 
> http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp
> File src/pdf/SkPDFGraphicState.cpp (right):
> 
>
http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
> src/pdf/SkPDFGraphicState.cpp:191: const char* aXfermodeString =
> blendModeFromXfermode(aXfermodeName);
> Nit: Almost seems like we want to assert that aXfermodeString isn't NULL.
> 
>
http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
> src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
> bXfermodeString) == 0;
> Nit: !strcmp
Optional.
> 
>
http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
> src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
> bXfermodeString) == 0;
> Optional: Can we get away with just doing a pointer comparison here? Maybe
> update the comments of blendModeFromXfermode to indicate that it returns the
> address of a string literal constant that can be used for this type of
> comparison.

[Steve VanDeBogart, 2011-05-09 17:33:33]

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp
File src/pdf/SkPDFGraphicState.cpp (right):

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:191: const char* aXfermodeString =
blendModeFromXfermode(aXfermodeName);
On 2011/05/09 08:37:46, Chris Guillory wrote:
> Nit: Almost seems like we want to assert that aXfermodeString isn't NULL.

Done.

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
bXfermodeString) == 0;
On 2011/05/09 08:37:46, Chris Guillory wrote:
> Optional: Can we get away with just doing a pointer comparison here? Maybe
> update the comments of blendModeFromXfermode to indicate that it returns the
> address of a string literal constant that can be used for this type of
> comparison.

Yea, I considered that, but it's pretty fragile.  The strings are short, so the
overhead for doing it the robust way isn't too bad.

http://codereview.appspot.com/4516043/diff/1/src/pdf/SkPDFGraphicState.cpp#ne...
src/pdf/SkPDFGraphicState.cpp:204: return strcmp(aXfermodeString,
bXfermodeString) == 0;
On 2011/05/09 08:37:46, Chris Guillory wrote:
> Nit: !strcmp

In this case, I prefer == 0.  !foo implies that foo failed, but in this case
!foo is success, so foo == 0 indicates that better.