Hidden annotations should not be drawn

Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: PDF files being added in [1].

[1] https://codereview.chromium.org/2239713003/

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

[tonikitoo, 2016-08-11 21:11:51]

Description was changed from

==========
Hidden annotations should not be drawn

Since now PDFium is gradually adding support to
specific annotations' drawing, it should also respect
the "flags" that feature them.

For instance, annotations flags currenly supported
in PDFium are: invisible, hidden, print, nozoom, norotate,
noview, readonly, locked and togglenoview (see their respective
values in core/fpdfdoc/include/cpdf_annot.h).

In a PDF file source, "flags" definition can be seen by looking at
"/F {value}" syntax, where {value} is an predefined integer value.

CL adds a check for the specific "hidden" flag, in order to
match IE + acrobat reader behavior.

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

to

==========
Hidden annotations should not be drawn

Since now PDFium is gradually adding support to
specific annotations' drawing, it should also respect
the "flags" that feature them.

For instance, annotations flags currenly supported
in PDFium are: invisible, hidden, print, nozoom, norotate,
noview, readonly, locked and togglenoview (see their respective
values in core/fpdfdoc/include/cpdf_annot.h).

In a PDF file source, "flags" definition can be seen by looking at
"/F {value}" syntax, where {value} is an predefined integer value.

CL adds a check for the specific "hidden" flag, in order to
match IE + acrobat reader behavior.

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

[tonikitoo, 2016-08-11 21:11:51]

tonikitoo@igalia.com changed reviewers:
+ dsinclair@chromium.org, jaepark@google.com, tsepez@chromium.org

[tonikitoo, 2016-08-11 21:12:14]

PTAL (tests are being added here: https://codereview.chromium.org/2239713003)

[tonikitoo, 2016-08-11 21:23:48]

On 2016/08/11 21:21:13, tonikitoo1 wrote:

To compare the behavior of PDFium and IE/Acroread, load this URL in both
browsers: https://people.igalia.com/agomes/pdfium/samples/test_modified.pdf

- Chromium highlights 4 annotations, that are "flagged" as hidden (see "/F 2" in
the code).
- IE does not highlight any annotation.

[Tom Sepez, 2016-08-11 21:28:47]

Looks OK, but deferring to others.  Thanks.

[jaepark, 2016-08-11 21:39:21]

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

https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:626: 
Wouldn't the annotation still be drawn if it has an existing AP dictionary?

[tonikitoo, 2016-08-11 21:41:56]

On 2016/08/11 21:39:21, jaepark wrote:
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> File core/fpdfdoc/cpvt_generateap.cpp (right):
> 
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> core/fpdfdoc/cpvt_generateap.cpp:626: 
> Wouldn't the annotation still be drawn if it has an existing AP dictionary?

in the current incarnation of the patch, yes, but we should match whatever
IE/Acroread does here, I believe.

Would you have a sample PDF with an AP so I could try the behavior on both
browsers/PDF engines?

[jaepark, 2016-08-11 21:59:30]

On 2016/08/11 21:41:56, tonikitoo1 wrote:
> On 2016/08/11 21:39:21, jaepark wrote:
> in the current incarnation of the patch, yes, but we should match whatever
> IE/Acroread does here, I believe.
> 
> Would you have a sample PDF with an AP so I could try the behavior on both
> browsers/PDF engines?

You can modify the F flag in testing/corpus/fx/mulobj/new/text_markup/*.pdf and
check how it behaves.

[dsinclair, 2016-08-12 03:06:58]

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

https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
core/fpdfdoc/cpvt_generateap.cpp:798: if (flags & ANNOTFLAG_HIDDEN)
if flags is just used in the if no need to store it to a temporary.

if (pAnnotDict->GetIntegerBy("F") & ANNOTFLAG_HIDDEN).

Actually, it maybe worth adding a short helper so these just become

if (!AnnotationsVisible(pAnnotDict))

or something like that.

[tonikitoo, 2016-08-12 03:16:52]

On 2016/08/11 21:59:30, jaepark wrote:
> On 2016/08/11 21:41:56, tonikitoo1 wrote:
> > On 2016/08/11 21:39:21, jaepark wrote:
> > in the current incarnation of the patch, yes, but we should match whatever
> > IE/Acroread does here, I believe.
> > 
> > Would you have a sample PDF with an AP so I could try the behavior on both
> > browsers/PDF engines?
> 
> You can modify the F flag in testing/corpus/fx/mulobj/new/text_markup/*.pdf
and
> check how it behaves.

I cant find PDF samples with "AP" in this directory @jaepark.

On 2016/08/12 03:06:58, dsinclair wrote:
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> File core/fpdfdoc/cpvt_generateap.cpp (right):
> 
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> core/fpdfdoc/cpvt_generateap.cpp:798: if (flags & ANNOTFLAG_HIDDEN)
> if flags is just used in the if no need to store it to a temporary.
> 
> if (pAnnotDict->GetIntegerBy("F") & ANNOTFLAG_HIDDEN).
> 
> Actually, it maybe worth adding a short helper so these just become
> 
> if (!AnnotationsVisible(pAnnotDict))
> 
> or something like that.

Will do.

[tonikitoo, 2016-08-12 14:39:06]

On 2016/08/11 21:39:21, jaepark wrote:
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> File core/fpdfdoc/cpvt_generateap.cpp (right):
> 
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> core/fpdfdoc/cpvt_generateap.cpp:626: 
> Wouldn't the annotation still be drawn if it has an existing AP dictionary?

I checked that in testing/corpus/fx/js/calculate_sum_a_b_c.pdf (which has an
/Annot and /AP), and "no", the annotation is not draw is "AP" is present (so no
need to move the early-return upper the "AP" check in the methods).

On 2016/08/12 03:06:58, dsinclair wrote:
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> File core/fpdfdoc/cpvt_generateap.cpp (right):
> 
>
https://codereview.chromium.org/2239853002/diff/1/core/fpdfdoc/cpvt_generatea...
> core/fpdfdoc/cpvt_generateap.cpp:798: if (flags & ANNOTFLAG_HIDDEN)
> if flags is just used in the if no need to store it to a temporary.
> 
> if (pAnnotDict->GetIntegerBy("F") & ANNOTFLAG_HIDDEN).
> 
> Actually, it maybe worth adding a short helper so these just become
> 
> if (!AnnotationsVisible(pAnnotDict))
> 
> or something like that.

Done in patchset 2

[jaepark, 2016-08-12 18:08:35]

On 2016/08/12 14:39:06, tonikitoo1 wrote:
> 
> I checked that in testing/corpus/fx/js/calculate_sum_a_b_c.pdf (which has an
> /Annot and /AP), and "no", the annotation is not draw is "AP" is present (so
no
> need to move the early-return upper the "AP" check in the methods).

In calculate_sum_a_b_c.pdf there are only Widget annotations, and we handle
Widget annotations completely separately. (And since you didn't add any code in
GenerateWidgetAP, I think we can assume that "/F 2" was working correctly
without any modification of the code for Widget annotation.)

All pdfs in testing/corpus/fx/mulobj/new/text_markup/*.pdf actually do have AP
field with AP streams. It's not visible right away because all PDFs are
compressed. If you uncompress the PDF, you'll see there are annotations with AP
streams. I tested your code and I think Circle, Highlight, Square, Underline,
Ink, Squiggly and Strike annotations (the ones you added early return for HIDDEN
flag), still draw annotations even if the "/F 2" is set.

[tonikitoo, 2016-08-12 18:17:49]

On 2016/08/12 18:08:35, jaepark wrote:
> On 2016/08/12 14:39:06, tonikitoo1 wrote:
> > 
> > I checked that in testing/corpus/fx/js/calculate_sum_a_b_c.pdf (which has an
> > /Annot and /AP), and "no", the annotation is not draw is "AP" is present (so
> no
> > need to move the early-return upper the "AP" check in the methods).
> 
> In calculate_sum_a_b_c.pdf there are only Widget annotations, and we handle
> Widget annotations completely separately. (And since you didn't add any code
in
> GenerateWidgetAP, I think we can assume that "/F 2" was working correctly
> without any modification of the code for Widget annotation.)
> 
> All pdfs in testing/corpus/fx/mulobj/new/text_markup/*.pdf actually do have AP
> field with AP streams. It's not visible right away because all PDFs are
> compressed. If you uncompress the PDF, you'll see there are annotations with
AP
> streams. I tested your code and I think Circle, Highlight, Square, Underline,
> Ink, Squiggly and Strike annotations (the ones you added early return for
HIDDEN
> flag), still draw annotations even if the "/F 2" is set.

Awesome feedback, @jaepark.

Based on it, I moved the early-returns upper the "is AP annotation" branches.

[jaepark, 2016-08-12 18:26:34]

On 2016/08/12 18:17:49, tonikitoo1 wrote:
> 
> Awesome feedback, @jaepark.
> 
> Based on it, I moved the early-returns upper the "is AP annotation" branches.

I don't think moving the early-return up is the right answer.
If the annotation already has the AP Stream, PDFium will not generate PDFium's
AP Stream and use the existing one. Even if we early returned when the
annotation has HIDDEN flag, PDFium will still draw the annotation since it will
parse the stream and make them into page objects.

Please check the code with testing/corpus/fx/mulobj/new/text_markup/*.pdf after
modifying the annotation flag to "/F 2".

[tonikitoo, 2016-08-12 19:28:04]

On 2016/08/12 18:26:34, jaepark wrote:
> On 2016/08/12 18:17:49, tonikitoo1 wrote:
> > 
> > Awesome feedback, @jaepark.
> > 
> > Based on it, I moved the early-returns upper the "is AP annotation"
branches.
> 
> I don't think moving the early-return up is the right answer.

You are right.

I was able to decompress the PDFs in testing/corpus/fx/mulobj/new/text_markup/
(with qpdf on Linux), and verified that the moving the early-return upper does
not make a difference when adding "/F 2" manually, although it does on
IE/acroread.

> If the annotation already has the AP Stream, PDFium will not generate PDFium's
> AP Stream and use the existing one. Even if we early returned when the
> annotation has HIDDEN flag, PDFium will still draw the annotation since it
will
> parse the stream and make them into page objects.

Right again. We need to avoid drawing AP annotations altogether, and
CPVT_GenerateAP is just a fallback, so too down the road.

I believe a better place to check for "/F 2" (and then bail out earlier when
needed) would be CPDF_Annot::DrawAppearance (see cpdf_annot.cpp):

FX_BOOL CPDF_Annot::DrawAppearance(CPDF_Page* pPage,
                                   CFX_RenderDevice* pDevice,
                                   const CFX_Matrix* pUser2Device,
                                   AppearanceMode mode,
                                   const CPDF_RenderOptions* pOptions) {
  CFX_Matrix matrix;
  CPDF_Form* pForm =
      FPDFDOC_Annot_GetMatrix(pPage, this, mode, pUser2Device, matrix);
  if (!pForm) { <-----
    return FALSE;

We could expand the "if" clause indicated above with another
"IsAnnotationHidden" check. It does fix the issues for the annotations in
testing/corpus/fx/mulobj/new/text_markup/*.pdf.

WDYT?

[jaepark, 2016-08-12 21:43:27]

On 2016/08/12 19:28:04, tonikitoo1 wrote:
> On 2016/08/12 18:26:34, jaepark wrote:
> > On 2016/08/12 18:17:49, tonikitoo1 wrote:
> > > 
> > > Awesome feedback, @jaepark.
> > > 
> > > Based on it, I moved the early-returns upper the "is AP annotation"
> branches.
> > 
> > I don't think moving the early-return up is the right answer.
> 
> You are right.
> 
> I was able to decompress the PDFs in testing/corpus/fx/mulobj/new/text_markup/
> (with qpdf on Linux), and verified that the moving the early-return upper does
> not make a difference when adding "/F 2" manually, although it does on
> IE/acroread.
> 
> > If the annotation already has the AP Stream, PDFium will not generate
PDFium's
> > AP Stream and use the existing one. Even if we early returned when the
> > annotation has HIDDEN flag, PDFium will still draw the annotation since it
> will
> > parse the stream and make them into page objects.
> 
> Right again. We need to avoid drawing AP annotations altogether, and
> CPVT_GenerateAP is just a fallback, so too down the road.
> 
> I believe a better place to check for "/F 2" (and then bail out earlier when
> needed) would be CPDF_Annot::DrawAppearance (see cpdf_annot.cpp):
> 
> FX_BOOL CPDF_Annot::DrawAppearance(CPDF_Page* pPage,
>                                    CFX_RenderDevice* pDevice,
>                                    const CFX_Matrix* pUser2Device,
>                                    AppearanceMode mode,
>                                    const CPDF_RenderOptions* pOptions) {
>   CFX_Matrix matrix;
>   CPDF_Form* pForm =
>       FPDFDOC_Annot_GetMatrix(pPage, this, mode, pUser2Device, matrix);
>   if (!pForm) { <-----
>     return FALSE;
> 
> We could expand the "if" clause indicated above with another
> "IsAnnotationHidden" check. It does fix the issues for the annotations in
> testing/corpus/fx/mulobj/new/text_markup/*.pdf.
> 
> WDYT?

Yes, I think CPDF_Annot::DrawAppearance is a better place to check for flag.

[tonikitoo, 2016-08-12 22:41:48]

Description was changed from

==========
Hidden annotations should not be drawn

Since now PDFium is gradually adding support to
specific annotations' drawing, it should also respect
the "flags" that feature them.

For instance, annotations flags currenly supported
in PDFium are: invisible, hidden, print, nozoom, norotate,
noview, readonly, locked and togglenoview (see their respective
values in core/fpdfdoc/include/cpdf_annot.h).

In a PDF file source, "flags" definition can be seen by looking at
"/F {value}" syntax, where {value} is an predefined integer value.

CL adds a check for the specific "hidden" flag, in order to
match IE + acrobat reader behavior.

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

to

==========
Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: - pdf files been added in [1].

[1] https://codereview.chromium.org/2239713003/

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

[tonikitoo, 2016-08-12 22:43:06]

> > We could expand the "if" clause indicated above with another
> > "IsAnnotationHidden" check. It does fix the issues for the annotations in
> > testing/corpus/fx/mulobj/new/text_markup/*.pdf.
> > 
> > WDYT?
> 
> Yes, I think CPDF_Annot::DrawAppearance is a better place to check for flag.

Added in patchset 4. Also tested with PDFs in
testing/corpus/fx/mulobj/new/text_markup/*.pdf

[tonikitoo, 2016-08-13 15:43:14]

Description was changed from

==========
Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: - pdf files been added in [1].

[1] https://codereview.chromium.org/2239713003/

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

to

==========
Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: PDF files being added in [1].

[1] https://codereview.chromium.org/2239713003/

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

[tonikitoo, 2016-08-13 15:43:14]

tonikitoo@igalia.com changed reviewers:
+ thestig@chromium.org

[tonikitoo, 2016-08-13 15:44:21]

(adding @thestig to the review list)

[Lei Zhang, 2016-08-15 00:10:09]

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

https://codereview.chromium.org/2239853002/diff/60001/core/fpdfdoc/cpvt_gener...
core/fpdfdoc/cpvt_generateap.cpp:616: static bool
IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
Can you only implement this once? static bool
CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict); ?

[tonikitoo, 2016-08-15 04:25:13]

>
https://codereview.chromium.org/2239853002/diff/60001/core/fpdfdoc/cpvt_gener...
> core/fpdfdoc/cpvt_generateap.cpp:616: static bool
> IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
> Can you only implement this once? static bool
> CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict); ?

Done in patchset 5.

[dsinclair, 2016-08-15 13:39:01]

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:137: FX_BOOL
CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
Add a // static. comment before the method.

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:63: static FX_BOOL
IsAnnotationHidden(CPDF_Dictionary* pAnnotDict);
Move static definition up below the enum on line 38.

[tonikitoo, 2016-08-15 13:51:02]

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/cpdf_annot...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/cpdf_annot...
core/fpdfdoc/cpdf_annot.cpp:137: FX_BOOL
CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
On 2016/08/15 13:39:00, dsinclair wrote:
> Add a // static. comment before the method.

Done.

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/include/cp...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2239853002/diff/80001/core/fpdfdoc/include/cp...
core/fpdfdoc/include/cpdf_annot.h:63: static FX_BOOL
IsAnnotationHidden(CPDF_Dictionary* pAnnotDict);
On 2016/08/15 13:39:00, dsinclair wrote:
> Move static definition up below the enum on line 38.

Done.

[dsinclair, 2016-08-15 15:34:38]

I'm happy when jaepark@ is happy.

[jaepark, 2016-08-15 18:25:22]

On 2016/08/15 15:34:38, dsinclair wrote:
> I'm happy when jaepark@ is happy.

I'm good with this after landing test cases[1] and rolling DEPS for
pdfium_tests.
Also, give thestig@ a chance to have a look.

[Lei Zhang, 2016-08-15 18:39:39]

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

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:623: if
(CPDF_Annot::IsAnnotationHidden(pAnnotDict))
Maybe it is time to combine the AP key check and this into a common function,
and call that from all the GenerateFooAP() methods?

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/include/c...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/include/c...
core/fpdfdoc/include/cpdf_annot.h:40: static FX_BOOL
IsAnnotationHidden(CPDF_Dictionary* pAnnotDict);
bool

[tonikitoo, 2016-08-15 18:59:31]

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

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:623: if
(CPDF_Annot::IsAnnotationHidden(pAnnotDict))
On 2016/08/15 18:39:39, Lei Zhang wrote:
> Maybe it is time to combine the AP key check and this into a common function,
> and call that from all the GenerateFooAP() methods?

Done.

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/include/c...
File core/fpdfdoc/include/cpdf_annot.h (right):

https://codereview.chromium.org/2239853002/diff/100001/core/fpdfdoc/include/c...
core/fpdfdoc/include/cpdf_annot.h:40: static FX_BOOL
IsAnnotationHidden(CPDF_Dictionary* pAnnotDict);
On 2016/08/15 18:39:39, Lei Zhang wrote:
> bool

Done.

[Lei Zhang, 2016-08-15 19:02:10]

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

https://codereview.chromium.org/2239853002/diff/120001/core/fpdfdoc/cpvt_gene...
core/fpdfdoc/cpvt_generateap.cpp:616: static bool
ShouldGenerateAPForAnnotation(CPDF_Dictionary* pAnnotDict) {
Move this into the anonymous namespace above and drop the "static ".

We like to group all the anonymous functions together at the top of the file,
rather than scatter them all over.

[jaepark, 2016-08-15 20:20:59]

Looks good. Please update DEPS after landing the test cases.

[Lei Zhang, 2016-08-15 20:47:28]

Looks good. Can you land https://codereview.chromium.org/2239713003/ or do you
need help there?

[tonikitoo, 2016-08-15 21:02:57]

On 2016/08/15 20:47:28, Lei Zhang wrote:
> Looks good. Can you land https://codereview.chromium.org/2239713003/ or do you
> need help there?

I believe to need some help there (specially if my committer account -
tonikitoo@chromium - has no push rights to pdfium_test repo).

[jaepark, 2016-08-16 02:15:30]

On 2016/08/15 21:02:57, tonikitoo1 wrote:
> I believe to need some help there (specially if my committer account -
> tonikitoo@chromium - has no push rights to pdfium_test repo).

Test cases landed in https://codereview.chromium.org/2250543003/ .

[tonikitoo, 2016-08-16 02:20:26]

Patchset 9 rolls DEPS in testing/corpus/ to e4926b9e.

[jaepark, 2016-08-16 02:23:12]

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

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

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-16 02:26:49]

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

[commit-bot: I haz the power, 2016-08-16 02:26:50]

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

[Lei Zhang, 2016-08-16 02:27:36]

corpus tests are failing

https://codereview.chromium.org/2239853002/diff/160001/core/fpdfdoc/cpdf_anno...
File core/fpdfdoc/cpdf_annot.cpp (right):

https://codereview.chromium.org/2239853002/diff/160001/core/fpdfdoc/cpdf_anno...
core/fpdfdoc/cpdf_annot.cpp:139: return pAnnotDict->GetIntegerBy("F") &
ANNOTFLAG_HIDDEN;
Looks like MSVC isn't happy with this. You need to make it: return !!(foo &
bar);

!!!!

[tonikitoo, 2016-08-16 03:09:01]

On 2016/08/16 02:27:36, Lei Zhang wrote:
> corpus tests are failing
> 
>
https://codereview.chromium.org/2239853002/diff/160001/core/fpdfdoc/cpdf_anno...
> File core/fpdfdoc/cpdf_annot.cpp (right):
> 
>
https://codereview.chromium.org/2239853002/diff/160001/core/fpdfdoc/cpdf_anno...
> core/fpdfdoc/cpdf_annot.cpp:139: return pAnnotDict->GetIntegerBy("F") &
> ANNOTFLAG_HIDDEN;
> Looks like MSVC isn't happy with this. You need to make it: return !!(foo &
> bar);
> 
> !!!!

Done :(

[tonikitoo, 2016-08-16 03:10:03]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 03:10:12]

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-16 03:10:14]

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

[commit-bot: I haz the power, 2016-08-16 03:10:15]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[not to use - tonikitoo, 2016-08-16 03:24:38]

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

[commit-bot: I haz the power, 2016-08-16 03:24:42]

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-16 03:24:43]

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

[commit-bot: I haz the power, 2016-08-16 03:24:44]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[Lei Zhang, 2016-08-16 03:42:15]

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

[commit-bot: I haz the power, 2016-08-16 03:42:24]

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-16 03:46:53]

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

[commit-bot: I haz the power, 2016-08-16 03:46:54]

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

[Lei Zhang, 2016-08-16 03:53:50]

Looks like we need another corpus update. I'll do that in ~2 hours and give you
a new git hash to update with.

[Lei Zhang, 2016-08-16 04:22:06]

On 2016/08/16 03:53:50, Lei Zhang wrote:
> Looks like we need another corpus update. I'll do that in ~2 hours and give
you
> a new git hash to update with.

https://pdfium.googlesource.com/pdfium_tests/+/1fdec9eaee514481092dd8e066029d...

[tonikitoo, 2016-08-16 04:38:40]

On 2016/08/16 04:22:06, Lei Zhang wrote:
> On 2016/08/16 03:53:50, Lei Zhang wrote:
> > Looks like we need another corpus update. I'll do that in ~2 hours and give
> you
> > a new git hash to update with.
> 
>
https://pdfium.googlesource.com/pdfium_tests/+/1fdec9eaee514481092dd8e066029d...

Done. I tried a DRY run of the bots, but it looks like pdfium committers are
different than chromium committers, so it failed.

[Lei Zhang, 2016-08-16 04:42:23]

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

[commit-bot: I haz the power, 2016-08-16 04:42:27]

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-16 04:48:37]

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

[commit-bot: I haz the power, 2016-08-16 04:48:38]

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

[Lei Zhang, 2016-08-16 04:50:32]

On 2016/08/16 04:38:40, tonikitoo1 wrote:
> Done. I tried a DRY run of the bots, but it looks like pdfium committers are
> different than chromium committers, so it failed.

Yes, they are. Looks like there are some Mac failures. I don't have a Mac handy.
Will help take a look tomorrow.

[tonikitoo, 2016-08-16 19:25:39]

On 2016/08/16 04:50:32, Lei Zhang wrote:
> On 2016/08/16 04:38:40, tonikitoo1 wrote:
> > Done. I tried a DRY run of the bots, but it looks like pdfium committers are
> > different than chromium committers, so it failed.
> 
> Yes, they are. Looks like there are some Mac failures. I don't have a Mac
handy.
> Will help take a look tomorrow.

Right. According to [1], Bot fails on the tests added in [2]:

Summary of Failures:
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup1_hidden.pdf
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup2_hidden.pdf
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup4_hidden.pdf
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup5_hidden.pdf
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup7_hidden.pdf
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup8_hidden.pdf

[1]
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...
[2] https://codereview.chromium.org/2250543003/

Is this common that they pass on other bots (Win/Linux), but fail on Mac? Also,
feel free to tell if I can help with it somehow.

[dsinclair, 2016-08-16 19:52:38]

On 2016/08/16 19:25:39, tonikitoo1 wrote:
> On 2016/08/16 04:50:32, Lei Zhang wrote:
> > On 2016/08/16 04:38:40, tonikitoo1 wrote:
> > > Done. I tried a DRY run of the bots, but it looks like pdfium committers
are
> > > different than chromium committers, so it failed.
> > 
> > Yes, they are. Looks like there are some Mac failures. I don't have a Mac
> handy.
> > Will help take a look tomorrow.
> 
> Right. According to [1], Bot fails on the tests added in [2]:
> 
> Summary of Failures:
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup1_hidden.pdf
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup2_hidden.pdf
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup4_hidden.pdf
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup5_hidden.pdf
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup7_hidden.pdf
>
/b/build/slave/pdfium/build/pdfium/testing/corpus/fx/mulobj/new/text_markup/new_textmarkup8_hidden.pdf
> 
> [1]
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...
> [2] https://codereview.chromium.org/2250543003/
> 
> Is this common that they pass on other bots (Win/Linux), but fail on Mac?
Also,
> feel free to tell if I can help with it somehow.



You need to suppress the tests on mac. There are font rendering differences
which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll see
entries for the original new_textmarkup*.pdf files for mac, you need equivalent
lines for new_textmarkup*_hidden.pdf files.

[tonikitoo, 2016-08-16 20:05:43]

> You need to suppress the tests on mac. There are font rendering differences
> which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll see
> entries for the original new_textmarkup*.pdf files for mac, you need
equivalent
> lines for new_textmarkup*_hidden.pdf files.

Done.

[jaepark, 2016-08-16 20:12:43]

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

[commit-bot: I haz the power, 2016-08-16 20:12:46]

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-16 20:32:01]

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

[commit-bot: I haz the power, 2016-08-16 20:32:02]

Dry run: This issue passed the CQ dry run.

[jaepark, 2016-08-16 20:33:05]

On 2016/08/16 20:05:43, tonikitoo1 wrote:
> > You need to suppress the tests on mac. There are font rendering differences
> > which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll
see
> > entries for the original new_textmarkup*.pdf files for mac, you need
> equivalent
> > lines for new_textmarkup*_hidden.pdf files.
> 
> Done.

lgtm

[Lei Zhang, 2016-08-16 20:34:02]

On 2016/08/16 19:52:38, dsinclair wrote:
> You need to suppress the tests on mac. There are font rendering differences
> which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll see
> entries for the original new_textmarkup*.pdf files for mac, you need
equivalent
> lines for new_textmarkup*_hidden.pdf files.

Well, let me test and see why it's failing...

[tonikitoo, 2016-08-16 20:34:11]

The CQ bit was checked by tonikitoo@igalia.com

[commit-bot: I haz the power, 2016-08-16 20:34:21]

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-16 20:34:38]

Description was changed from

==========
Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: PDF files being added in [1].

[1] https://codereview.chromium.org/2239713003/

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

to

==========
Hidden annotations should not be drawn

Now that PDFium supports drawing of more annotation
types, it should also respect the "hidden" flag that
annotations might feature.
For instance, in IE/Acroread if an annotation is flagged as
"hidden" it does not get drawn.

CL adds a check for the specific "hidden" flag, not drawing
annotation that are flagged with it, in order to match IE + acrobat
reader behavior.

The "flags" definition can be seen by looking at "/F {value}"
syntax in a PDF file source, where {value} is an predefined
integer value.

Test: PDF files being added in [1].

[1] https://codereview.chromium.org/2239713003/

BUG=62625

Committed:
https://pdfium.googlesource.com/pdfium/+/66c26e84e6e154f0466db6fce4f3f36d445c...
==========

[commit-bot: I haz the power, 2016-08-16 20:34:40]

Committed patchset #12 (id:220001) as
https://pdfium.googlesource.com/pdfium/+/66c26e84e6e154f0466db6fce4f3f36d445c...

[Lei Zhang, 2016-08-16 20:36:47]

On 2016/08/16 20:34:02, Lei Zhang wrote:
> Well, let me test and see why it's failing...

Or not wait. It's fine. No worries.

[dsinclair, 2016-08-16 20:37:05]

On 2016/08/16 20:34:02, Lei Zhang wrote:
> On 2016/08/16 19:52:38, dsinclair wrote:
> > You need to suppress the tests on mac. There are font rendering differences
> > which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll
see
> > entries for the original new_textmarkup*.pdf files for mac, you need
> equivalent
> > lines for new_textmarkup*_hidden.pdf files.
> 
> Well, let me test and see why it's failing...


I built and tested the first two files on my mac and they were both rendered
identical just font differences.

[tonikitoo, 2016-08-16 20:37:23]

On 2016/08/16 20:34:02, Lei Zhang wrote:
> On 2016/08/16 19:52:38, dsinclair wrote:
> > You need to suppress the tests on mac. There are font rendering differences
> > which cause the diffs to fail. If you look in testing/SUPPRESSIONS you'll
see
> > entries for the original new_textmarkup*.pdf files for mac, you need
> equivalent
> > lines for new_textmarkup*_hidden.pdf files.
> 
> Well, let me test and see why it's failing...

I believe cq+ just went ahead and merged the patch in a fast way, given the
successful dry run.

In any case, it is totally worth it to check the failures, as you said.. Either
as part of this CL (i.e. reverting it), or as a follow up.

[Lei Zhang, 2016-08-16 20:48:59]

On 2016/08/16 20:37:05, dsinclair wrote:
> I built and tested the first two files on my mac and they were both rendered
> identical just font differences.

Ok, and we have no per-platform expectation files, so this is all fine as is.