When updating, write the xref table in the same format as the previous one (bug 1878916)#17636
Conversation
Snuffleupagus
left a comment
There was a problem hiding this comment.
Leaving a couple of comments based on an initial (quick) look.
Please run all tests, and I'll try to finish reviewing this ASAP.
529a6ab to
b4ebb20
Compare
|
FYI, in Edge with the pdf viewer from Acrobat, they're saving files thanks to an incremental update and they're doing exactly what this patch is aiming to do: use the same xref table kind as the last one. |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/7b51e7a315407fc/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/5d3d0df684ff262/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/5d3d0df684ff262/output.txt Total script time: 24.91 mins
Image differences available at: http://54.241.84.105:8877/5d3d0df684ff262/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/7b51e7a315407fc/output.txt Total script time: 41.94 mins
Image differences available at: http://54.193.163.58:8877/7b51e7a315407fc/reftest-analyzer.html#web=eq.log |
b4ebb20 to
1c3f487
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/b68aa67cc55ec9a/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/51ce1826ac09946/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/b68aa67cc55ec9a/output.txt Total script time: 24.77 mins
Image differences available at: http://54.241.84.105:8877/b68aa67cc55ec9a/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/51ce1826ac09946/output.txt Total script time: 42.04 mins
Image differences available at: http://54.193.163.58:8877/51ce1826ac09946/reftest-analyzer.html#web=eq.log |
|
Chiming in here, qpdf recognizes this as a valid file, and my manual decoding of the file also suggests that its valid. I am also able to open this file with firefox, chrome, evince, okular, atril, ghostscript, and Adobe Acrobat, though the mac preview application fails to open the file. My opinion is that there's nothing wrong with mixing xref tables and xref streams, though clearly there are some readers that can't handle it, which may be a good reason not to mix and match as a rule. It is curious that the file in the referenced qpdf issue qpdf/qpdf#1148 has /Prev pointing to white space preceding the original startxref from the file prior to being appended, but that's not technically incorrect. |
|
(My opinion, not that it's relevant here since I'm an outsider, is that it would be better not to mix and match.) |
Snuffleupagus
left a comment
There was a problem hiding this comment.
This is slightly complicated code, but I think it makes sense (and the updated patch is much easier to follow.)
r=me, thank you!
…s one (bug 1878916) The specs are unclear about what kind of xref table format must be used. In checking the validity of some pdfs in the preflight tool from Acrobat we can guess that having the same format is the correct way to do. The pdf in the mentioned bug, after having been changed, wasn't correctly displayed in neither Chrome nor Acrobat: it's now fixed.
1c3f487 to
2133da1
Compare
This patch removes some previous fixes which are now likely fixed by mozilla#17636. Fixes mozilla#20302.
This patch removes some previous fixes which are now likely fixed by mozilla#17636. Fixes mozilla#20302.
The specs are unclear about what kind of xref table format must be used.
In checking the validity of some pdfs in the preflight tool from Acrobat we can guess that having the same format is the correct way to do.
The pdf in the mentioned bug, after having been changed, wasn't correctly displayed in neither Chrome nor Acrobat: it's now fixed.