PDF: add support for named destinations.

PDF: add support for named destinations.

[edisonn, 2013-02-25 18:18:11]

lgtm + a comment

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp#newcode96
tests/AnnotationTest.cpp:96: if (rawOutput[i + 0] == '/' &&
maybe strstr(rawOutput, "/example ") == NULL would do the same work as  this for
+ long series of ifs ...

[David Lattimore, 2013-02-25 20:48:13]

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp#newcode96
tests/AnnotationTest.cpp:96: if (rawOutput[i + 0] == '/' &&
On 2013/02/25 18:18:11, edisonn wrote:
> maybe strstr(rawOutput, "/example ") == NULL would do the same work as  this
for
> + long series of ifs ...
That would require rawOutput to not contain any null characters (except at the
end). PDFs can contain null characters and probably aren't null terminated.

[edisonn, 2013-02-25 21:12:12]

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp#newcode96
tests/AnnotationTest.cpp:96: if (rawOutput[i + 0] == '/' &&
then instead of the long series of ifs, strncmp can be used

On 2013/02/25 20:48:14, David Lattimore wrote:
> On 2013/02/25 18:18:11, edisonn wrote:
> > maybe strstr(rawOutput, "/example ") == NULL would do the same work as  this
> for
> > + long series of ifs ...
> That would require rawOutput to not contain any null characters (except at the
> end). PDFs can contain null characters and probably aren't null terminated.

[reed1, 2013-02-25 21:28:05]

Seems like we want a larger sort of test as well, perhaps one connected to
SampleApp(?) where we can see that we can create a real marked-up PDF file with
live links. Perhaps one or more GMs that generate annotated images, which we can
then open up and try on on a desktop.

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp
File src/core/SkAnnotation.cpp (right):

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp#newco...
src/core/SkAnnotation.cpp:76: rect.iset(point.x(), point.y(), point.x() + 1,
point.y() + 1);
Why are we calling iset() here, when the input is SkScalars?
Why do we add + 1 and turn the point into a rect? If point is really what you
want, why not call drawPoints() instead of drawRect()?

[Steve VanDeBogart, 2013-02-25 23:16:41]

https://codereview.appspot.com/7374052/diff/1/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.appspot.com/7374052/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:98: *  Annotate the canvas by associating a name
with the specified point.
From this change it's not obvious that you use these named destinations by way
of a url that's #destination_name.  At the least we should document this.  I
think even better would be to have another helper that's
SkAnnotateRectWithDestination or something like that.

https://codereview.appspot.com/7374052/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7374052/diff/1/include/pdf/SkPDFDevice.h#newco...
include/pdf/SkPDFDevice.h:148: SK_API SkPDFDict* getDestsDict();
Since this dictionary isn't (and can't be in the final form), this should just
return the information needed and shouldn't use PDF objects.

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp
File src/core/SkAnnotation.cpp (right):

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp#newco...
src/core/SkAnnotation.cpp:76: rect.iset(point.x(), point.y(), point.x() + 1,
point.y() + 1);
On 2013/02/25 21:28:05, reed1 wrote:
> Why are we calling iset() here, when the input is SkScalars?
> Why do we add + 1 and turn the point into a rect? If point is really what you
> want, why not call drawPoints() instead of drawRect()?

Agreed.  Call drawPoints and add a check in SkPDFDevice's draw point method for
annotations.

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFDocument.cpp#newco...
src/pdf/SkPDFDocument.cpp:125: fDocCatalog->insert("Dests", dests);
Section 3.6.1 of the PDF4 spec says that this must be an indirect reference. 
Use SkPDFObjRef

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFPage.h
File src/pdf/SkPDFPage.h (right):

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFPage.h#newcode54
src/pdf/SkPDFPage.h:54: void applyDestinations(SkPDFDict* dict);
get/appendDestinations instead of applyDestinations?

[epoger, 2013-02-26 16:15:28]

On 2013/02/25 21:28:05, reed1 wrote:
> Seems like we want a larger sort of test as well, perhaps one connected to
> SampleApp(?) where we can see that we can create a real marked-up PDF file
with
> live links. Perhaps one or more GMs that generate annotated images, which we
can
> then open up and try on on a desktop.

I'm not sure it's realistic to expect an outside contributor to add this larger
test, and I don't want to discourage outside contributors from making
contributions...  Maybe, once the other issues have been dealt with, this CL can
be committed and someone on the Skia team can work with David to create the
larger test?

[reed1, 2013-02-26 16:30:53]

On 2013/02/26 16:15:28, epoger wrote:
> On 2013/02/25 21:28:05, reed1 wrote:
> > Seems like we want a larger sort of test as well, perhaps one connected to
> > SampleApp(?) where we can see that we can create a real marked-up PDF file
> with
> > live links. Perhaps one or more GMs that generate annotated images, which we
> can
> > then open up and try on on a desktop.
> 
> I'm not sure it's realistic to expect an outside contributor to add this
larger
> test, and I don't want to discourage outside contributors from making
> contributions...  Maybe, once the other issues have been dealt with, this CL
can
> be committed and someone on the Skia team can work with David to create the
> larger test?

I'm fine if the larger test is done in-house. My request is orthogonal (in some
respects) to this particular CL, as we don't have such a test for the existing
annotation code in PDF.

All that said, the principal that untestable CLs add to our technical debt is
true for internal and external contributors... perhaps more so for external
ones, since the author isn't always around later to answer questions about
intent, etc.

[David Lattimore, 2013-02-26 20:44:43]

I got through most of the changes requested yesterday, including writing a
basic GM. Just got one more thing to fix up and I'll push out a new version.


On 27 February 2013 03:30, <reed@google.com> wrote:

> On 2013/02/26 16:15:28, epoger wrote:
>
>> On 2013/02/25 21:28:05, reed1 wrote:
>> > Seems like we want a larger sort of test as well, perhaps one
>>
> connected to
>
>> > SampleApp(?) where we can see that we can create a real marked-up
>>
> PDF file
>
>> with
>> > live links. Perhaps one or more GMs that generate annotated images,
>>
> which we
>
>> can
>> > then open up and try on on a desktop.
>>
>
>  I'm not sure it's realistic to expect an outside contributor to add
>>
> this larger
>
>> test, and I don't want to discourage outside contributors from making
>> contributions...  Maybe, once the other issues have been dealt with,
>>
> this CL can
>
>> be committed and someone on the Skia team can work with David to
>>
> create the
>
>> larger test?
>>
>
> I'm fine if the larger test is done in-house. My request is orthogonal
> (in some respects) to this particular CL, as we don't have such a test
> for the existing annotation code in PDF.
>
> All that said, the principal that untestable CLs add to our technical
> debt is true for internal and external contributors... perhaps more so
> for external ones, since the author isn't always around later to answer
> questions about intent, etc.
>
>
>
https://codereview.appspot.**com/7374052/<https://codereview.appspot.com/7374...
>

[David Lattimore, 2013-02-27 00:24:53]

I think I've addressed all comments. I added a GM. I wasn't sure about doing
something with SampleApp. I tried running it and it seemed good for looking at
visual output, but since this change affects behavior in a PDF viewer, the
visual appearance wouldn't tell us much. The GM currently fails because expected
output doesn't match. I couldn't find where the expected outputs were stored.

https://codereview.appspot.com/7374052/diff/1/include/core/SkAnnotation.h
File include/core/SkAnnotation.h (right):

https://codereview.appspot.com/7374052/diff/1/include/core/SkAnnotation.h#new...
include/core/SkAnnotation.h:98: *  Annotate the canvas by associating a name
with the specified point.
On 2013/02/25 23:16:41, Steve VanDeBogart wrote:
> From this change it's not obvious that you use these named destinations by way
> of a url that's #destination_name.  At the least we should document this.  I
> think even better would be to have another helper that's
> SkAnnotateRectWithDestination or something like that.

Done.

https://codereview.appspot.com/7374052/diff/1/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.appspot.com/7374052/diff/1/include/pdf/SkPDFDevice.h#newco...
include/pdf/SkPDFDevice.h:148: SK_API SkPDFDict* getDestsDict();
On 2013/02/25 23:16:41, Steve VanDeBogart wrote:
> Since this dictionary isn't (and can't be in the final form), this should just
> return the information needed and shouldn't use PDF objects.

Done.

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp
File src/core/SkAnnotation.cpp (right):

https://codereview.appspot.com/7374052/diff/1/src/core/SkAnnotation.cpp#newco...
src/core/SkAnnotation.cpp:76: rect.iset(point.x(), point.y(), point.x() + 1,
point.y() + 1);
On 2013/02/25 21:28:05, reed1 wrote:
> Why are we calling iset() here, when the input is SkScalars?
> Why do we add + 1 and turn the point into a rect? If point is really what you
> want, why not call drawPoints() instead of drawRect()?

Done.

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFDocument.cpp#newco...
src/pdf/SkPDFDocument.cpp:125: fDocCatalog->insert("Dests", dests);
On 2013/02/25 23:16:41, Steve VanDeBogart wrote:
> Section 3.6.1 of the PDF4 spec says that this must be an indirect reference. 
> Use SkPDFObjRef
Good catch. Fixed.

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFPage.h
File src/pdf/SkPDFPage.h (right):

https://codereview.appspot.com/7374052/diff/1/src/pdf/SkPDFPage.h#newcode54
src/pdf/SkPDFPage.h:54: void applyDestinations(SkPDFDict* dict);
On 2013/02/25 23:16:41, Steve VanDeBogart wrote:
> get/appendDestinations instead of applyDestinations?

Done.

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/1/tests/AnnotationTest.cpp#newcode96
tests/AnnotationTest.cpp:96: if (rawOutput[i + 0] == '/' &&
On 2013/02/25 21:12:12, edisonn wrote:
> then instead of the long series of ifs, strncmp can be used
> 
> On 2013/02/25 20:48:14, David Lattimore wrote:
> > On 2013/02/25 18:18:11, edisonn wrote:
> > > maybe strstr(rawOutput, "/example ") == NULL would do the same work as 
this
> > for
> > > + long series of ifs ...
> > That would require rawOutput to not contain any null characters (except at
the
> > end). PDFs can contain null characters and probably aren't null terminated.
> 

Done.

[Steve VanDeBogart, 2013-02-27 02:14:36]

Thanks for the update. Basically just some nits below.

https://codereview.appspot.com/7374052/diff/15001/src/core/SkAnnotation.cpp
File src/core/SkAnnotation.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/core/SkAnnotation.cpp#n...
src/core/SkAnnotation.cpp:58: static void SkAnnotatePaint(SkPaint& paint, const
char* key, SkData* value) {
static methods omit the Sk prefix and are in hacker style "foo_bar"

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:707: if (handleAnnotations(points, count, *d.fMatrix,
passedPaint)) {
Looks like you'll have to update the other devices as well.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1313: bool SkPDFDevice::handleAnnotations(const SkPoint*
points, size_t count,
mike: I forget, what's Skia's policy on method overloading?

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1377: nameData->size() - 1);
nit: indent

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1378: annotation->insert("Dest", new
SkPDFString(name))->unref();
The code isn't currently consistent between new and SkNEW, but new "new"'s
should use SkNEW.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1383: int x;
Why not use and SkPoint here? Seems simpler.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1407: for (NamedDestination** d =
fNamedDestinations.begin();
iterating by index might wrap nicer here... it is an array.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1410: SkAutoTUnref<SkPDFArray> record(new SkPDFArray);
nit: record -> pdfDest ?

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp#ne...
tests/AnnotationTest.cpp:76: /** Returns true if data (main contain null
characters) contains needle (null
nit: main -> may

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp#ne...
tests/AnnotationTest.cpp:78: static bool ContainsString(const char* data, size_t
dataSize, const char* needle) {
contains_string

Can you update the above test to use this method as well?

[David Lattimore, 2013-02-27 04:26:03]

https://codereview.appspot.com/7374052/diff/15001/src/core/SkAnnotation.cpp
File src/core/SkAnnotation.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/core/SkAnnotation.cpp#n...
src/core/SkAnnotation.cpp:58: static void SkAnnotatePaint(SkPaint& paint, const
char* key, SkData* value) {
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> static methods omit the Sk prefix and are in hacker style "foo_bar"

Done.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:707: if (handleAnnotations(points, count, *d.fMatrix,
passedPaint)) {
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> Looks like you'll have to update the other devices as well.

Done.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1378: annotation->insert("Dest", new
SkPDFString(name))->unref();
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> The code isn't currently consistent between new and SkNEW, but new "new"'s
> should use SkNEW.

Done - here and elsewhere. Where I was changing methods, I also updated existing
"new"s.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1383: int x;
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> Why not use and SkPoint here? Seems simpler.

Done.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1407: for (NamedDestination** d =
fNamedDestinations.begin();
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> iterating by index might wrap nicer here... it is an array.

Done.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1410: SkAutoTUnref<SkPDFArray> record(new SkPDFArray);
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> nit: record -> pdfDest ?

Done.

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp#ne...
tests/AnnotationTest.cpp:76: /** Returns true if data (main contain null
characters) contains needle (null
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> nit: main -> may

Done.

https://codereview.appspot.com/7374052/diff/15001/tests/AnnotationTest.cpp#ne...
tests/AnnotationTest.cpp:78: static bool ContainsString(const char* data, size_t
dataSize, const char* needle) {
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> contains_string
> 
> Can you update the above test to use this method as well?

Done.

[Steve VanDeBogart, 2013-02-27 21:17:59]

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1313: bool SkPDFDevice::handleAnnotations(const SkPoint*
points, size_t count,
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> mike: I forget, what's Skia's policy on method overloading?

Ben says that method overloading is generally avoided in Skia. 
handleRectAnnotation and handlePointAnnotation ?

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1377: nameData->size() - 1);
On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> nit: indent

lining up with static_cast is what I would expect, it was one space short
before.

[David Lattimore, 2013-02-27 22:21:31]

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1313: bool SkPDFDevice::handleAnnotations(const SkPoint*
points, size_t count,
On 2013/02/27 21:17:59, Steve VanDeBogart wrote:
> On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> > mike: I forget, what's Skia's policy on method overloading?
> 
> Ben says that method overloading is generally avoided in Skia. 
> handleRectAnnotation and handlePointAnnotation ?

Done.

https://codereview.appspot.com/7374052/diff/15001/src/pdf/SkPDFDevice.cpp#new...
src/pdf/SkPDFDevice.cpp:1377: nameData->size() - 1);
On 2013/02/27 21:17:59, Steve VanDeBogart wrote:
> On 2013/02/27 02:14:36, Steve VanDeBogart wrote:
> > nit: indent
> 
> lining up with static_cast is what I would expect, it was one space short
> before.

Done.

[Steve VanDeBogart, 2013-02-27 22:23:21]

LGTM

[David Lattimore, 2013-02-27 22:31:44]

Thanks for the review. Is there anything else I need to do with the GM file I
added, or will the expected outputs get generated by some automated process?
Assuming they're needed at all that is, since the GM is mostly useful for manual
verification by loading the resulting PDF in a PDF viewer.

Also, should I wait for more LGTMs before submitting?

[Steve VanDeBogart, 2013-02-27 22:53:50]

On 2013/02/27 22:31:44, David Lattimore wrote:
> Thanks for the review. Is there anything else I need to do with the GM file I
> added, or will the expected outputs get generated by some automated process?

I think we now collect output so that can be committed subsequently as new
expectations.  So not a completley automated process, but not manual either.
 
> Assuming they're needed at all that is, since the GM is mostly useful for
manual
> verification by loading the resulting PDF in a PDF viewer.
> 
> Also, should I wait for more LGTMs before submitting?

I don't expect anyone to object, but lets give them a chance to.  I suspect you
won't be able to commit directly, since I don't see you on here:
https://code.google.com/p/skia/people/list

[Steve VanDeBogart, 2013-02-27 22:55:46]

And thanks for making the change. Feature development on the PDF backend is
on-demand and it's even better when the demand comes with code.

[epoger, 2013-02-28 19:21:07]

On 2013/02/27 22:55:46, Steve VanDeBogart wrote:
> And thanks for making the change. Feature development on the PDF backend is
> on-demand and it's even better when the demand comes with code.

Yeah, thank you David, not only for taking the initiative to do this in the
first place but also sticking with it to address all the review comments!

I haven't reviewed the code myself, but Steve's LGTM and 20 hours without
objection is good enough for me. :-)  I'll patch this change into our new
Rietveld server (which provides commit queue functionality, enabling external
contributors to commit approved changes) and commit it from there.

[epoger, 2013-02-28 19:44:42]

> I haven't reviewed the code myself, but Steve's LGTM and 20 hours without
> objection is good enough for me. :-)  I'll patch this change into our new
> Rietveld server (which provides commit queue functionality, enabling external
> contributors to commit approved changes) and commit it from there.

I have patched this changelist into https://codereview.chromium.org/12382023/ . 
David, please take a quick look and see if that seems right.  (I promise I won't
ask you to do that yet again!)  And I will take it from there...

[David Lattimore, 2013-02-28 23:43:08]

Continuing discussion started at https://codereview.chromium.org/12382023/.

The new GM I added is failing with:
---- internal_links_comparison-serialize: 1001 (of 350000) differing pixels, max
per-channel mismatch R=0 G=0 B=255 A=0

Which I wrongly assumed was because of a missing baseline. Apparently it's a
difference between rendering directly and serializing/deserializing.

So it seems like the noDrawAnnotation flag is not getting flattened. SkPaint
stores the no-draw flag in fPrivFlags which has a comment on it explicitly
saying that it's not flattened. This means that no-draw calls to drawPoints and
drawRect get written without their no-draw annotation. When they're unflattened
they then get drawn.

This is a problem for the annotations on drawRect as well, but was never noticed
since there was previously no GM that used SkAnnotateRectWithURL.

I was able to make the test pass by adding paint.isNoDrawAnnotation checks to
SkPictureRecord and SkGPipeWrite.

Does this seem like the right way to fix it or should SkPaint be flattening the
no-draw annotation?

[Steve VanDeBogart, 2013-02-28 23:46:28]

On 2013/02/28 23:43:08, David Lattimore wrote:
> Continuing discussion started at https://codereview.chromium.org/12382023/.
> 
> The new GM I added is failing with:
> ---- internal_links_comparison-serialize: 1001 (of 350000) differing pixels,
max
> per-channel mismatch R=0 G=0 B=255 A=0
> 
> Which I wrongly assumed was because of a missing baseline. Apparently it's a
> difference between rendering directly and serializing/deserializing.
> 
> So it seems like the noDrawAnnotation flag is not getting flattened. SkPaint
> stores the no-draw flag in fPrivFlags which has a comment on it explicitly
> saying that it's not flattened. This means that no-draw calls to drawPoints
and
> drawRect get written without their no-draw annotation. When they're
unflattened
> they then get drawn.
> 
> This is a problem for the annotations on drawRect as well, but was never
noticed
> since there was previously no GM that used SkAnnotateRectWithURL.
> 
> I was able to make the test pass by adding paint.isNoDrawAnnotation checks to
> SkPictureRecord and SkGPipeWrite.
> 
> Does this seem like the right way to fix it or should SkPaint be flattening
the
> no-draw annotation?

But that means the playback won't ever have annotations, right?  I think the
write thing is to flatten the no-draw bit.  I'll defer to mike on this point
though.

[reed1, 2013-03-01 04:07:55]

I think the righter fix is to flatten the no-draw-bit along with the annotation.

[David Lattimore, 2013-03-01 04:17:09]

On 2013/03/01 04:07:55, reed1 wrote:
> I think the righter fix is to flatten the no-draw-bit along with the
annotation.

SGTM. I'm working on that now. It turns out that the no-draw-bit is being
flattened as part of the SkAnnotation. I'm not sure why the paint is duplicating
the bit from the annotation. In any case, I'm currently running into problems
with the whole annotation being lost during serialization/deserialization. Still
trying to figure out what's going on.

[David Lattimore, 2013-03-01 06:22:30]

Figured out what was going on. There are two problems.

1) SkAnnotation, SkDataSet and SkData aren't registered as named factories (e.g.
in SkFlattenable::InitializeFlattenables). So they get flattened, but when
unflattening it fails to find the appropriate function to unflatten with, so the
SkAnnotation is dropped.

2) SkGPipeCanvas::writePaint doesn't write the annotation.

For (1), is it reasonable to add SkAnnotation, SkDataSet and SkData to
SkFlattenable::InitializeFlattenables?

For (2), adding an isNoDrawAnnotation check to SkGPipeWrite makes the test pass
(together with the fix for (1)), but I'm guessing we want the no-draw-annotation
to get passed through the SkGPipe? If that's the case, then I guess I need to
add support to SkGPipeCanvas for serializing annotations.

It's after 5pm Friday here, so I won't get back to this until Monday morning
(Sydney time).

[reed1, 2013-03-01 12:30:57]

We should support flattening of annotation and its payload (SkData, etc.), but
they do not need to inherit from Flattenable, since they are all inspectable and
their data can be extract directly for flattening.

[David Lattimore, 2013-03-04 01:44:04]

SkAnnotation already inherited from Flattenable, so that wasn't any trouble.
I've registered SkAnnotation, SkData and SkDataSet as being flattenable and
changed SkGPipe to pass through annotations. With this change, the new GM I
added passes. Please take another look and let me know if this seems like the
right approach.

[reed1, 2013-03-04 16:25:45]

lgtm

[epoger, 2013-03-05 19:15:23]

Patched into a writeable checkout as https://codereview.chromium.org/12466008 ;
I'll run a couple of trybots there and commit it if they pass.

[epoger, 2013-03-06 15:53:38]

We encountered problems when we landed patchset 5 as
https://codereview.chromium.org/12466008 ...

Now trying patchset 7 as https://codereview.chromium.org/12533009/