Generate default AP stream for highlight annotation.

Generate default AP stream for highlight annotation.

This patch generates a default AP stream for highlight annotation so that
highlight annotations without AP stream can be displayed.

BUG=62625
Committed: https://pdfium.googlesource.com/pdfium/+/ab47acec5252d4a09da3cbde1a118b5255fe52ae

[jaepark, 2016-07-29 21:57:45]

jaepark@google.com changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-07-29 22:09:39]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 22:09:47]

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-07-29 22:15:17]

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

[commit-bot: I haz the power, 2016-07-29 22:15:18]

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

[Lei Zhang, 2016-07-29 22:20:50]

As we discussed in person, we have failing corpus tests on the trybots. You will
need to prepare a CL to update the testing corpus. When both CLs have been
approved:

1) Land the testing corpus update CL
2) Update this CL to roll DEPS for the testing corpus
3) Land this CL

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:511: // Defaults to 0xFFFF0 color for
highlight.
0xFFFF00 ?

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:530:
pNormalStream->SetData((uint8_t*)sAppStream.GetBuffer(), sAppStream.GetSize(),
No C-style casting. Use static_cast or reinterpret_cast.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:547: pExtGStateDict->SetAt("TransGs",
pTransGsDict);
So if we have multiple highlight annotations, can we reuse "TransGs" or do the
other annotations need unique names for their dictionaries? I've seen other PDFs
that have /GS1 /GS2, etc.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generateap.h
File core/fpdfdoc/cpvt_generateap.h (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.h:34: static FX_BOOL
GenerateHighlightAP(CPDF_Document* pDoc,
Can we put these in alphabetical order?

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.h:34: static FX_BOOL
GenerateHighlightAP(CPDF_Document* pDoc,
Can we use bool instead of FX_BOOL? We can convert the others separately.

[jaepark, 2016-07-29 23:23:51]

Patchset #2 (id:20001) has been deleted

[jaepark, 2016-07-29 23:32:26]

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:511: // Defaults to 0xFFFF0 color for
highlight.
On 2016/07/29 22:20:50, Lei Zhang wrote:
> 0xFFFF00 ?

Done.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:530:
pNormalStream->SetData((uint8_t*)sAppStream.GetBuffer(), sAppStream.GetSize(),
On 2016/07/29 22:20:50, Lei Zhang wrote:
> No C-style casting. Use static_cast or reinterpret_cast.

Done.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:547: pExtGStateDict->SetAt("TransGs",
pTransGsDict);
On 2016/07/29 22:20:50, Lei Zhang wrote:
> So if we have multiple highlight annotations, can we reuse "TransGs" or do the
> other annotations need unique names for their dictionaries? I've seen other
PDFs
> that have /GS1 /GS2, etc.

We can reuse "TransGs" because the scope of a named resources, like "TransGs",
is local to a particular content stream.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generateap.h
File core/fpdfdoc/cpvt_generateap.h (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.h:34: static FX_BOOL
GenerateHighlightAP(CPDF_Document* pDoc,
On 2016/07/29 22:20:50, Lei Zhang wrote:
> Can we put these in alphabetical order?

Done.

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.h:34: static FX_BOOL
GenerateHighlightAP(CPDF_Document* pDoc,
On 2016/07/29 22:20:50, Lei Zhang wrote:
> Can we use bool instead of FX_BOOL? We can convert the others separately.

Done.

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:542: pTransGsDict->SetAtNumber("CA", 1);
Data type for CA and ca field are Number.

[Lei Zhang, 2016-07-30 00:56:44]

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:547: pExtGStateDict->SetAt("TransGs",
pTransGsDict);
On 2016/07/29 23:32:26, jaepark wrote:
> On 2016/07/29 22:20:50, Lei Zhang wrote:
> > So if we have multiple highlight annotations, can we reuse "TransGs" or do
the
> > other annotations need unique names for their dictionaries? I've seen other
> PDFs
> > that have /GS1 /GS2, etc.
> 
> We can reuse "TransGs" because the scope of a named resources, like "TransGs",
> is local to a particular content stream.

How about just "GS" then?

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:501: return FALSE;
return true/false

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:542: pTransGsDict->SetAtNumber("CA", 1);
On 2016/07/29 23:32:26, jaepark wrote:
> Data type for CA and ca field are Number.

I can't find references to "ca" in the PDF spec. Do you know where it is?

[jaepark, 2016-07-30 01:24:59]

Patchset #3 (id:60001) has been deleted

[jaepark, 2016-07-30 01:28:28]

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:547: pExtGStateDict->SetAt("TransGs",
pTransGsDict);
On 2016/07/30 00:56:44, Lei Zhang wrote:
> On 2016/07/29 23:32:26, jaepark wrote:
> > On 2016/07/29 22:20:50, Lei Zhang wrote:
> > > So if we have multiple highlight annotations, can we reuse "TransGs" or do
> the
> > > other annotations need unique names for their dictionaries? I've seen
other
> > PDFs
> > > that have /GS1 /GS2, etc.
> > 
> > We can reuse "TransGs" because the scope of a named resources, like
"TransGs",
> > is local to a particular content stream.
> 
> How about just "GS" then?

Done.

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:501: return FALSE;
On 2016/07/30 00:56:44, Lei Zhang wrote:
> return true/false

Done.

https://codereview.chromium.org/2193983002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:542: pTransGsDict->SetAtNumber("CA", 1);
On 2016/07/30 00:56:44, Lei Zhang wrote:
> I can't find references to "ca" in the PDF spec. Do you know where it is?

It's in the "4.3.4 Graphics State Parameter Dictionaries" section. TABLE 4.8
Entries in a graphics state parameter dictionary.

[Lei Zhang, 2016-07-30 01:32:28]

Looking good. We still need to update the testing repo and roll DEPS here.

[jaepark, 2016-07-30 01:45:49]

Roll DEPS for testing/corpus to use updated test case for highlight annotation,
which was reviewed in https://codereview.chromium.org/2193313004.

[Lei Zhang, 2016-07-30 01:47:48]

lgtm

[jaepark, 2016-07-30 02:40:55]

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

[commit-bot: I haz the power, 2016-07-30 02:41:05]

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-07-30 02:52:55]

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

[commit-bot: I haz the power, 2016-07-30 02:52:56]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-07-30 02:53:16]

The CQ bit was checked by jaepark@google.com

[commit-bot: I haz the power, 2016-07-30 02:53:25]

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-07-30 02:53:42]

Description was changed from

==========
Generate default AP stream for highlight annotation.

This patch generates a default AP stream for highlight annotation so that
highlight annotations without AP stream can be displayed.

BUG=62625
==========

to

==========
Generate default AP stream for highlight annotation.

This patch generates a default AP stream for highlight annotation so that
highlight annotations without AP stream can be displayed.

BUG=62625

Committed:
https://pdfium.googlesource.com/pdfium/+/ab47acec5252d4a09da3cbde1a118b5255fe...
==========

[commit-bot: I haz the power, 2016-07-30 02:53:43]

Committed patchset #4 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/ab47acec5252d4a09da3cbde1a118b5255fe...