Refactor annotation flags code #6672
Conversation
| * @see {@link shared/util.js} | ||
| */ | ||
| setFlags: function Annotation_setFlags(flags) { | ||
| if (flags === (flags | 0)) { |
There was a problem hiding this comment.
Nit: it might be a tiny bit simpler to understand this line if isInt is used here :-)
There was a problem hiding this comment.
Done in the new commit.
timvandermeij
force-pushed
the
annotation-flags
branch
2 times, most recently
from
536e1ca to
2ef2af3
Compare
|
Unfortunately the simplifications of We might need to do something similar to PR #6619, but for annotations, in order to get better test-coverage. Edit: Another PDF that's not currently working as intended in the regression tests is |
timvandermeij
force-pushed
the
annotation-flags
branch
2 times, most recently
from
04a5d7c to
011b620
Compare
This patch makes it possible to set and get all possible flags that the PDF specification defines. Even though we do not support all possible annotation types and not all possible annotation flags yet, this general framework makes it easy to access all flags for each annotation such that annotation type implementations can use this information. We add constants for all possible annotation flags such that we do not need to hardcode the flags in the code anymore. The `isViewable()` and `isPrintable()` methods are now easier to read. Additionally, unit tests have been added to ensure correct behavior. This is another part of mozilla#5218.
timvandermeij
force-pushed
the
annotation-flags
branch
from
011b620 to
0991c06
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c49cc715a4d4949/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/c49cc715a4d4949/output.txt Total script time: 0.78 mins Published |
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/588c1e0133bbfa1/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/63129c9427608a3/output.txt |
| if (data.fieldType === 'Sig') { | ||
| warn('unimplemented annotation type: Widget signature'); | ||
| this.setFlags(AnnotationFlag.HIDDEN); | ||
| } |
There was a problem hiding this comment.
This is a nice simplification, compared to the previous code!
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/63129c9427608a3/output.txt Total script time: 18.51 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/588c1e0133bbfa1/output.txt Total script time: 19.51 mins
|
Refactor annotation flags code
|
Looks good, thanks for the patch! |
| */ | ||
| setFlags: function Annotation_setFlags(flags) { | ||
| if (isInt(flags)) { | ||
| this.flags = flags; |
There was a problem hiding this comment.
Why this.flags instead of this.data.flags?
Only the data object is exposed with Page.getAnnotationsData method so this will remove the ability to read annotation flags from viewer/third-party code.
There was a problem hiding this comment.
This will be addressed by #6677.
This PDF file (see issue 4914) originally regressed in PR 4318, and was subsequently fixed in PR 4915. I added the PDF file as a (linked) test-case in PR 6481, in an effort to prevent regressions. Since we at that time didn't have the necessary framework in place, in order to correctly test annotations, this almost regressed *again* in PR mozilla#6672 (comment). In that PDF file, some of the annotations are both printable and hidden, and should definitely *not* be visible on normal display. Hence this patch, which adds the `annotations` flag to the manifest in order to ensure that those annotations won't be rendered when `intent === 'display'`.
This patch makes it possible to set and get all possible flags that the PDF specification defines. Even though we do not support all possible annotation types and not all possible annotation flags yet, this general framework makes it easy to access all flags for each annotation such that annotation type implementations can use this information.
We add constants for all possible annotation flags such that we do not need to hardcode the flags in the code anymore. The
isViewable()andisPrintable()methods are now easier to read. Additionally, unit tests have been added to ensure correct behavior.This is another part of #5218.
I have tested this patch, both with displaying and with printing, with a corpus of 23 PDF files with different annotation types and annotation flags in them and observed no difference with the current master.