Short story: somebody got lost in two different indices. pi is an index in the stream and is explained on page 198 of the 32000-spec (however 1-based there), and ps is an index to something in PDF.js. I used the code from flag 0 (which works) to understand which is which. It is also important to understand that for flags 1,2 and 3, the stream is always assigned to the same coordinates and colors. What changes is which "old" coordinates and colors are assigned to what is "missing" in the stream. This is why for these flags, the code is identical except for the assignments in the first "row".
Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned.
Aside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at https://github.com/mozilla/pdf.js/pull/5218#issuecomment-52779659) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns.
Doing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported.
I have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.
Built-in DOM properties are slower than plain JS properties.
A few lines before, textContent is assigned as follows:
textDiv.textContent = geom.str;
So replacing textDiv.textContent.length with geom.str.length slightly
improves performance without side effects.
Since the zoom value should be in percent, using `PDFViewer.currentScale` will be wrong by a factor of 100, potentially causing the zoom level of the document to become wrong on load.
*This fixes a regression from PR 6192.*
Under some circumstances, the `resize` event handler in `viewer.js` is fired before the scale has been set. This can lead to PDF documents being rendered at the wrong zoom level when they are opened.
It seems that a way to reliably trigger this is to, using the Firefox addon, open a PDF file that triggers the `fallback` bar, in a new background tab (i.e. middle clicking on a link, or use the context menu).
Prior to PR 6192, we checked the selected option in the `scaleSelect` dropdown instead. Since `pageAutoOption` is selected by default in `viewer.html`, this should explain why the issue wasn't visible previously.
This patch refactors the code responsible for setting the annotation's rectangle. Its goal is to:
- Actually check that the input array is actually an array, and if so, that it contains exactly four elements.
- Only call `normalizeRect` if the input array is valid, i.e., we do not call it for the default rectangle anymore.
Unit tests are provided just like with the other patches in this series.
Fixes#6106
To avoid future regressions, two new unit tests were added:
1. A new PDF based on the report from #6106, which contains an
OpenAction of type JavaScript and a string "this.print({...}".
2. An existing PDF from https://bugzil.la/1001080 (from #4698).
Although it does not matter, since we don't execute the JavaScript code,
I have also changed "print(true)" to "print({})" since the print method
takes an object (not a boolean). See "Printing PDF documents", page 62:
http://adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/js_developer_guide.pdf
The special handling of the 'custom' scale value is only relevant for the `scaleSelect` dropdown in the standard viewer, hence I think that it should be placed in `viewer.js` instead.
Features / bug fixes in the preprocessor:
- Add word boundary after regex for preprocessor token matching.
Previously, when you mistakenly used "#ifdef" instead of "#if", the
line would be parsed as a preprocessor directive (because "#ifdef"
starts with "#if"), but without condition (because "def" does not
start with a space). Consequently, the condition would always be false
and anything between "#ifdef" and "#endif" would not be included.
- Add validation and error reporting everywhere, to aid debugging.
- Support nested comments (by accounting for the whole stack of
conditions, instead of only the current one).
- Add #elif preprocessor command. Could be used as follows:
//#if !FEATURE_ENABLED
//#error FEATURE_ENABLED must be set
//#endif
- Add #error preprocessor command.
- Add end-of-line word boundary after "-->" in the comment trimmer.
Otherwise the pattern would also match "-->" in the middle of a line,
and incorrectly convert something like "while(i-->0)" to "while(i0)".
Code health:
- Add unit tests for the preprocessor (run external/builder/test.js).
- Fix broken link to MDN (resolved to DXR).
- Refactor to use STATE_* names instead of magic numbers (the original
meaning of the numbers is preserved, with one exception).
- State 3 has been split in two states, to distinguish between being in
an #if and #else. This is needed to ensure that #else cannot be
started without an #if.
The previous regex was too greedy, it stripped a significant amount of
code when I put another file after core/murmurhash3.js. This was caused
by the fact that the license header in murmurhash3.js does not contain
"Mozilla Foundation", so the regex continued to match until my new file
(which had the standard license header containing "Mozilla Foundation").
It took a while to figure out why adding comments in worker_loader.js
caused the build to fail, because getWorkerSrcFiles did not print an
error message when it failed to parse the file. These issues have been
resolved as follows:
- Leading comments are stripped.
- The trailing comma is removed from the array.
- Errors are detected and useful error messages are printed.
- Use rimraf instead of a custom removeDirSync implementation - rimraf
deals with edge cases like EPERM on Windows.
- Detect when the process exits before it was requested via stop(),
instead of running the cleanup handler.
- Add fallback for process detection when the process exits before it
was requested. On *nix systems, this is done via pkill and pgrep, on
Windows this is done via wmic.
- Add some asserts to check the preconditions of the methods, and output
some status information to aid debugging in case of failure.
I have verified that these changes work on ArchLinux and Windows XP,
using Chrome and Firefox, as follows:
1. node make unittest
2. node make unittest
3. Restart the Firefox process via the task manager as soon as possible.
4. node make unittest
5. Temporary lock a file/directory within the temporary profile
directory until the tests have finished, and then unlock the file
within 10 seconds.
In all cases, the auxilary browser processes are killed, and the
temporary profile directory is wiped.
