Generate default AP stream for underline annotation.

Generate default AP stream for underline annotation.

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

Also, roll DEPS for testing/corpus to cae29d1 to test underline annotations.

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

[jaepark, 2016-08-01 22:56:04]

Patchset #1 (id:1) has been deleted

[jaepark, 2016-08-01 23:10:52]

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

[jaepark, 2016-08-01 23:10:53]

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

https://codereview.chromium.org/2205543002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:561: pAnnotDict->KeyExist("ca") ?
pAnnotDict->GetNumberBy("ca") : 1.0;
Although I think this is correct implementation, other PDF viewers (including
Adobe Acrobat Reader), are only depending on the value of "CA" to determine the
opacity of the annotation. 

For example, for highlight annotation, if CA is 1.0 and ca is 0.5, I think the
opacity should be 0.5 since Highlight is fill (non stroke) operation. However,
Acrobat Reader shows 1.0 opacity.

Also, Okular does not generate "ca" field when creating Highlight annotation (it
only generates "CA" field).

I'm not sure if I'm misinterpreting the spec of CA and ca in ExtGState (SECTION
4.3.4 Graphics State Parameter Dictionaries, TABLE 4.8 Entries in a graphics
state parameter dictionary).

[Lei Zhang, 2016-08-01 23:16:24]

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

https://codereview.chromium.org/2205543002/diff/20001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:561: pAnnotDict->KeyExist("ca") ?
pAnnotDict->GetNumberBy("ca") : 1.0;
On 2016/08/01 23:10:52, jaepark wrote:
> Although I think this is correct implementation, other PDF viewers (including
> Adobe Acrobat Reader), are only depending on the value of "CA" to determine
the
> opacity of the annotation. 
> 
> For example, for highlight annotation, if CA is 1.0 and ca is 0.5, I think the
> opacity should be 0.5 since Highlight is fill (non stroke) operation. However,
> Acrobat Reader shows 1.0 opacity.
> 
> Also, Okular does not generate "ca" field when creating Highlight annotation
(it
> only generates "CA" field).
> 
> I'm not sure if I'm misinterpreting the spec of CA and ca in ExtGState
(SECTION
> 4.3.4 Graphics State Parameter Dictionaries, TABLE 4.8 Entries in a graphics
> state parameter dictionary).

Have you considered adding more tests to exercise this code and see what the
correct output should be?

You can write your own PDFs by hand. Take a look at testing/resources/*.in, and
testing/tools/fixup_pdf_template.py.

[jaepark, 2016-08-01 23:43:01]

On 2016/08/01 23:16:24, Lei Zhang wrote:
> Have you considered adding more tests to exercise this code and see what the
> correct output should be?
> 
> You can write your own PDFs by hand. Take a look at testing/resources/*.in,
and
> testing/tools/fixup_pdf_template.py.

Actually, I misread the spec. CA field in Annotation dictionary represents the
opacity and is used for ExtGState's CA and ca field.
But our highlight annotations does not support opacity right now. So I've
created another patch to support opacity for highlight annotation.
https://codereview.chromium.org/2202063002 .

I'll also add a test case for this.

[jaepark, 2016-08-02 01:15:46]

Rebased to upstream and used CA field in annotation dictionary for annotation
opacity.

[Lei Zhang, 2016-08-02 01:22:57]

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

https://codereview.chromium.org/2205543002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:446: CPDF_Dictionary*
GenerateExtGStateDict(CFX_ByteString sName,
Pass CFX_ByteStrings by const-ref?

https://codereview.chromium.org/2205543002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:577: if (pAnnotDict->KeyExist("C")) {
Can this and the identical block in GenerateHighlightAP() be refactored out into
a separate helper function? Ditto below for any piece that makes sense.

[jaepark, 2016-08-02 20:31:34]

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

https://codereview.chromium.org/2205543002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:446: CPDF_Dictionary*
GenerateExtGStateDict(CFX_ByteString sName,
On 2016/08/02 01:22:57, Lei Zhang wrote:
> Pass CFX_ByteStrings by const-ref?

Done.

https://codereview.chromium.org/2205543002/diff/40001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:577: if (pAnnotDict->KeyExist("C")) {
On 2016/08/02 01:22:57, Lei Zhang wrote:
> Can this and the identical block in GenerateHighlightAP() be refactored out
into
> a separate helper function? Ditto below for any piece that makes sense.

Done.

[Lei Zhang, 2016-08-02 21:08:22]

Looking good...

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

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:458: if (pAnnotDict->KeyExist("C")) {
Can we just omit this and instead check the return value of GetArrayBy() ? If
the 'C' key exists, but it's not a dictionary, then GetArrayBy() will return a
nullptr that then gets dereferenced...

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:467: CPDF_Dictionary*
GenerateExtGStateDict(CPDF_Dictionary* pAnnotDict,
Can |pAnnotDict| be passed by const-ref instead?

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:489: CPDF_Dictionary* pExtGStateDict) {
Can you add a comment to say "take ownership of |pExtGStateDict|." ?

[jaepark, 2016-08-02 22:15:20]

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

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:458: if (pAnnotDict->KeyExist("C")) {
On 2016/08/02 21:08:22, Lei Zhang wrote:
> Can we just omit this and instead check the return value of GetArrayBy() ? If
> the 'C' key exists, but it's not a dictionary, then GetArrayBy() will return a
> nullptr that then gets dereferenced...

Done.

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:467: CPDF_Dictionary*
GenerateExtGStateDict(CPDF_Dictionary* pAnnotDict,
On 2016/08/02 21:08:22, Lei Zhang wrote:
> Can |pAnnotDict| be passed by const-ref instead?

Done.

https://codereview.chromium.org/2205543002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:489: CPDF_Dictionary* pExtGStateDict) {
On 2016/08/02 21:08:22, Lei Zhang wrote:
> Can you add a comment to say "take ownership of |pExtGStateDict|." ?

Done.

[Lei Zhang, 2016-08-02 22:43:08]

Do we need to roll DEPS?

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

https://codereview.chromium.org/2205543002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:489: // Take ownership of |pExtGStateDict|.
Move this up to line 484. It's the function itself that takes ownership, not the
new statement below.

[jaepark, 2016-08-02 22:51:33]

Description was changed from

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

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

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

to

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

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

Also, roll DEPS for testing/corpus to cae29d1 to test underline annotations.

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

[jaepark, 2016-08-02 22:53:40]

Rolled DEPS for testing/corpus.

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

https://codereview.chromium.org/2205543002/diff/80001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:489: // Take ownership of |pExtGStateDict|.
On 2016/08/02 22:43:08, Lei Zhang wrote:
> Move this up to line 484. It's the function itself that takes ownership, not
the
> new statement below.

Done.

[Lei Zhang, 2016-08-02 22:55:08]

lgtm

https://codereview.chromium.org/2205543002/diff/100001/core/fpdfdoc/cpvt_gene...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2205543002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:485: // Take ownership of |pExtGStateDict|.
Takes, sorry for the bad grammar.

[jaepark, 2016-08-02 22:56:55]

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

[commit-bot: I haz the power, 2016-08-02 22:57:03]

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-08-02 23:14:15]

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

[commit-bot: I haz the power, 2016-08-02 23:14:16]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-02 23:20:26]

https://codereview.chromium.org/2205543002/diff/100001/core/fpdfdoc/cpvt_gene...
File core/fpdfdoc/cpvt_generateap.cpp (right):

https://codereview.chromium.org/2205543002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:485: // Take ownership of |pExtGStateDict|.
On 2016/08/02 22:55:08, Lei Zhang wrote:
> Takes, sorry for the bad grammar.

Done.

[jaepark, 2016-08-02 23:22:27]

The CQ bit was checked by jaepark@google.com

[jaepark, 2016-08-02 23:22:28]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2205543002/#ps120001
(title: "Generate default AP stream for underline annotation.")

[commit-bot: I haz the power, 2016-08-02 23:22:36]

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-08-02 23:22:56]

Description was changed from

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

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

Also, roll DEPS for testing/corpus to cae29d1 to test underline annotations.

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

to

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

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

Also, roll DEPS for testing/corpus to cae29d1 to test underline annotations.

BUG=62625

Committed:
https://pdfium.googlesource.com/pdfium/+/33e9b26ba3c191262d0abb945fda1efb3cdb...
==========

[commit-bot: I haz the power, 2016-08-02 23:22:57]

Committed patchset #6 (id:120001) as
https://pdfium.googlesource.com/pdfium/+/33e9b26ba3c191262d0abb945fda1efb3cdb...