Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(56)

Issue 2289293005: Use /RECT or /QuadPoints for annotation coordinates, depending on /AP (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months ago by tonikitoo
Modified:
8 months, 2 weeks ago
CC:
pdfium-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on a text markup definition, the coordinates used to draw the annotation come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. TEST=testing/resources/pixel/bug_585_*.in BUG=pdfium:585 Committed: https://pdfium.googlesource.com/pdfium/+/0a17fafd723e8684d1deb4b5ceea58967a0154da

Patch Set 1 #

Patch Set 2 : Use /RECT or /QuadPoints for annotation coordinates, depending on /AP #

Total comments: 12

Patch Set 3 : addressed feedback from tsepez/thestig #

Patch Set 4 : Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP #

Total comments: 4

Patch Set 5 : addressed thestig's comments #

Total comments: 11

Patch Set 6 : addressed thestig's and jaepark's comments. #

Patch Set 7 : rebased against ToT #

Patch Set 8 : rebased + new DEPS #

Unified diffs Side-by-side diffs Delta from patch set Stats (+267 lines, -26 lines) Patch
M DEPS View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M core/fpdfdoc/cpdf_annot.cpp View 1 2 3 4 5 6 7 5 chunks +59 lines, -10 lines 0 comments Download
M core/fpdfdoc/cpvt_generateap.cpp View 1 2 3 4 5 6 7 15 chunks +27 lines, -15 lines 0 comments Download
M core/fpdfdoc/include/cpdf_annot.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
A testing/resources/pixel/bug_585.in View 1 2 3 4 1 chunk +175 lines, -0 lines 0 comments Download
A + testing/resources/pixel/bug_585_expected.pdf.0.png View 1 2 3 4 Binary file 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (16 generated)
tonikitoo
PTAL
8 months, 4 weeks ago (2016-09-01 20:32:49 UTC) #4
jaepark
Also, can we add a test case with very different "Rect" and "QuadPoints", and check ...
8 months, 4 weeks ago (2016-09-01 20:52:00 UTC) #7
Tom Sepez
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp#newcode80 core/fpdfdoc/cpdf_annot.cpp:80: bool CPDF_Annot::ShouldUseQuadPointsCoords(CPDF_Array*& pArray) const { nit: can we make ...
8 months, 4 weeks ago (2016-09-01 20:53:08 UTC) #8
tonikitoo
On 2016/09/01 20:52:00, jaepark wrote: > Also, can we add a test case with very ...
8 months, 4 weeks ago (2016-09-01 21:03:47 UTC) #9
jaepark
On 2016/09/01 21:03:47, tonikitoo wrote: > https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp#newcode113 > > core/fpdfdoc/cpdf_annot.cpp:113: > > Even if we ...
8 months, 4 weeks ago (2016-09-01 21:11:07 UTC) #10
Lei Zhang
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp#newcode21 core/fpdfdoc/cpdf_annot.cpp:21: namespace { When not forward declaring, add blank lines ...
8 months, 4 weeks ago (2016-09-01 21:13:00 UTC) #11
tonikitoo
> > The difference is in the pixel level, for example ~0.001% from image_diff > ...
8 months, 4 weeks ago (2016-09-01 21:27:08 UTC) #12
tonikitoo
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot.cpp#newcode21 core/fpdfdoc/cpdf_annot.cpp:21: namespace { On 2016/09/01 21:12:59, Lei Zhang wrote: > ...
8 months, 4 weeks ago (2016-09-01 21:50:42 UTC) #13
tonikitoo
For the record, I have a new patch that addresses owners' comments (including jaepark's request ...
8 months, 3 weeks ago (2016-09-06 06:22:25 UTC) #14
tonikitoo
On 2016/09/06 06:22:25, tonikitoo wrote: > For the record, I have a new patch that ...
8 months, 3 weeks ago (2016-09-06 14:23:28 UTC) #16
Lei Zhang
So just to be clear - did you use pixel tests instead of adding PDFs ...
8 months, 3 weeks ago (2016-09-06 21:27:04 UTC) #19
tonikitoo
On 2016/09/06 21:27:04, Lei Zhang wrote: > So just to be clear - did you ...
8 months, 3 weeks ago (2016-09-06 23:19:29 UTC) #22
tonikitoo
Taking care of the tests results here: https://codereview.chromium.org/2300183002/
8 months, 3 weeks ago (2016-09-06 23:21:08 UTC) #23
Lei Zhang
On 2016/09/06 23:19:29, tonikitoo wrote: > On 2016/09/06 21:27:04, Lei Zhang wrote: > > So ...
8 months, 3 weeks ago (2016-09-07 02:02:45 UTC) #24
Tom Sepez
> In the git history, it appears we had pixel tests first in February 2015, ...
8 months, 3 weeks ago (2016-09-07 16:30:35 UTC) #25
jaepark
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp#newcode51 core/fpdfdoc/cpdf_annot.cpp:51: m_bIsMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype); m_bIsTextMarkupAnnotation https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp#newcode99 core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = ...
8 months, 3 weeks ago (2016-09-07 17:30:09 UTC) #26
tonikitoo
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp#newcode51 core/fpdfdoc/cpdf_annot.cpp:51: m_bIsMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype); On 2016/09/07 17:30:09, jaepark wrote: > ...
8 months, 3 weeks ago (2016-09-08 04:10:35 UTC) #27
tonikitoo
On 2016/09/08 04:10:35, tonikitoo wrote: > (..) > We could use left over sentence. Please ...
8 months, 3 weeks ago (2016-09-08 04:11:35 UTC) #28
jaepark
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot.cpp#newcode99 core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = m_bIsMarkupAnnotation && m_bHasGeneratedAP; On 2016/09/08 04:10:35, ...
8 months, 3 weeks ago (2016-09-08 17:49:28 UTC) #29
Lei Zhang
lgtm if jaepark@ is happy with this CL.
8 months, 3 weeks ago (2016-09-08 23:02:45 UTC) #30
jaepark
On 2016/09/08 23:02:45, Lei Zhang wrote: > lgtm if jaepark@ is happy with this CL. ...
8 months, 3 weeks ago (2016-09-08 23:13:09 UTC) #31
tonikitoo
On 2016/09/08 23:13:09, jaepark wrote: > On 2016/09/08 23:02:45, Lei Zhang wrote: > > lgtm ...
8 months, 3 weeks ago (2016-09-09 13:23:59 UTC) #32
Lei Zhang
This needs a rebase, BTW.
8 months, 2 weeks ago (2016-09-13 20:35:34 UTC) #33
tonikitoo
On 2016/09/13 20:35:34, Lei Zhang (OOO) wrote: > This needs a rebase, BTW. Done in ...
8 months, 2 weeks ago (2016-09-13 21:15:34 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2289293005/120001
8 months, 2 weeks ago (2016-09-13 21:15:51 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: mac on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/2034)
8 months, 2 weeks ago (2016-09-13 21:25:47 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2289293005/140001
8 months, 2 weeks ago (2016-09-15 20:30:27 UTC) #42
commit-bot: I haz the power
8 months, 2 weeks ago (2016-09-15 20:50:53 UTC) #44
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://pdfium.googlesource.com/pdfium/+/0a17fafd723e8684d1deb4b5ceea58967a01...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06