GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
| + container.style.left = data.rect[0] + 'px'; | ||
| + container.style.top = data.rect[1] + 'px'; | ||
| + | ||
| + // Size |
Let's remove trivial comment such as "Size", "Border" or "Border color". If we can, let's add a sentence about what we are planing to do instead.
Done in b1937e7, also for the existing border style code. The code is indeed very readable already without these comments.
| + | ||
| + self.div = document.createElement('div'); | ||
| + self.div.className = 'annotationLayer'; | ||
| + self.pageDiv.appendChild(self.div); |
We can create two functions "updateAnnotations" and "renderAnnotations" for PDFJS.AnnotationUtils that accept div, annotations, etc. and move logic of this if branches there. We can keep logic of the div creation and annotations.length check here.
|
Looks good after comments are addressed. |
|
/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/b0ca424f1f902ed/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/b0ca424f1f902ed/output.txt Total script time: 0.81 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/6f2fc4f4d1c7ee3/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/5d2c0c873f5dd05/output.txt |
| + PDFJS.AnnotationLayer.render(viewport, self.div, annotations, | ||
| + self.pdfPage, self.linkService); | ||
| + if (typeof mozL10n !== 'undefined') { | ||
| + mozL10n.translate(self.div); |
Does this line actually do the same thing as before, since the current code attempts to translate the individual annotation elements, and not the entire annotationLayerdiv?
yes, but we don't want to move mozL10n dependency to the src/display/
I have verified that mozL10n.translate() does work on the entire div too instead of only on individual annotation elements. I have tested this with PDFs with only one or multiple annotations per page and each annotation was properly translated. Therefore this seems to do the same thing as before. As Yury mentioned, it's nicer to keep localization in the web folder as it does not belong in the core code.
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/5d2c0c873f5dd05/output.txt Total script time: 19.23 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/6f2fc4f4d1c7ee3/output.txt Total script time: 20.16 mins
|
|
Merging with r=yurydelendik and r=me. |
1b5940e
into
mozilla:master
timvandermeij commentedDec 2, 2015
Fixes #6689 (as the testing part is tracked in #6673). It is also another part of #5218.
I have tested this PR with a set of 20 PDF files with various kinds of annotations (Link, Text, Widget, et cetera) and found no changes in rendering, behavior or printing.
Note that I intentionally used multiple commits to make reviewing easier and to split renaming code and moving code as much as possible.