Add support for saving forms #12137

calixteman · 2020-07-28T17:50:27Z

No description provided.

calixteman · 2020-07-28T17:55:59Z

@timvandermeij, @Snuffleupagus, @brendandahl, I'll add some tests tomorrow but it'd be nice to have a first feedback asap.

Snuffleupagus

My general comment would be that you'll need to be very careful to not accidentally create a corrupt PDF document when editing it. (Given how much trouble corrupt PDF documents have caused us over the years, we really ought to avoid creating such ones ourselves.)

Furthermore, you'll probably want to test this not only with "simple" and/or well-formed documents, to make sure that things still work with e.g. PDF documents that use XRefStms (i.e. cross-reference streams) and/or those where the XRef table is corrupt.

Also, I'm guessing that you may also want to hide the toolbar buttons (or at least have them be disabled), until editing has occurred!?

Finally, note that as I've remarked inline, you'll need to split the XRef writing part into proper main/worker-thread parts.

web/pdf_save_service.js

src/core/writer.js

src/core/worker.js

src/core/writer.js

brendandahl · 2020-07-29T05:41:44Z

Looking good overall, but as Jonas mentioned we need to split up the worker/main thread code more. Or we could move most everything to the worker. What about exposing a higher level API, something where the viewer would do:

Collect all the annotation storage data in a serializable format
Pass that data into a new function that lives in src/display/api.js on the PDFDocumentProxy (e.g. save(annotationStorages) which would in turn:
1. Send it to the worker which would do all the xref updating
2. Return updated PDF data, that would resolve the promise

calixteman · 2020-07-29T14:54:01Z

My general comment would be that you'll need to be very careful to not accidentally create a corrupt PDF document when editing it. (Given how much trouble corrupt PDF documents have caused us over the years, we really ought to avoid creating such ones ourselves.)
I don't want to write corrupted PDF!
I opened newly created PDF with evince successfully and I hope that you help me to find out errors if any.

Furthermore, you'll probably want to test this not only with "simple" and/or well-formed documents, to make sure that things still work with e.g. PDF documents that use XRefStms (i.e. cross-reference streams) and/or those where the XRef table is corrupt.

Do you know where I can find these kind of PDF docs with a form inside ?

Also, I'm guessing that you may also want to hide the toolbar buttons (or at least have them be disabled), until editing has occurred!?

Done in my last push.
For now I just added a smiley for the icon: I'm not the guy to draw a new icon.

Finally, note that as I've remarked inline, you'll need to split the XRef writing part into proper main/worker-thread parts.
Addressed too.

Snuffleupagus

Do you know where I can find these kind of PDF docs with a form inside ?

Besides scraping the test-suite for all PDF documents with forms (grep-ing for something like "/AcroForm" might work), I don't really know unfortunately.

src/display/api.js

src/core/writer.js

calixteman · 2020-08-07T10:51:49Z

This patch is ready for review.
It'd be nice to have a review for the beginning of next week, I'll address the issues you'll find and then we can merge it wednesday to have this patch in next firefox version.
@reviewers thx for your help.

timvandermeij

Phew, that was quite a large review. Despite the number of review comments, this looks really good code-wise (not tested yet)! I'm only not really sure about the XRef parts of the patch since I don't know too much about that to judge if the code is correct, and that must be correct because we don't want to make malformed documents. I would really appreciate a review from @Snuffleupagus and/or @brendandahl for that part of the patch.

src/core/annotation.js

web/pdf_save_service.js

timvandermeij

I only have two new comments left now that all changes have been made, and the following three comments from above are still open:

Once these five comments are addressed, this looks good to me code-wise. It's only pending manual testing then, and any checks from @brendandahl and/or @Snuffleupagus in particular for the XRef part of the patch.

Really nice work so far!

src/core/annotation.js

src/core/writer.js

brendandahl

r+ with the last comments from Tim. There's a few minor things I'd like to tweak/add, but they can be addressed in follow ups.

Snuffleupagus

Unfortunately I don't have nearly enough time to properly review the most complicated/important parts, which is the actual document/XRef writing.

However, I've left a couple of higher-level comments about API and/or default viewer issues that should be addressed; note e.g. that currently this is broken once you've actually built the viewer.

src/display/api.js

src/core/annotation.js

src/core/worker.js

web/app.js

Snuffleupagus · 2020-08-11T06:51:12Z

web/app.js

+    eventBus._on(
+      "download",
+      AppOptions.get("renderInteractiveForms")
+        ? webViewerSave
+        : webViewerDownload
+    );


Sorry, but this won't work correctly with the event removal since the renderInteractiveForms option may have been changed by then.
Please revert this, and instead change

pdf.js/web/app.js

Lines 2268 to 2270 in 7fb01f9

function webViewerDownload() {

PDFViewerApplication.download();

}

to e.g.

function webViewerDownload() { if (AppOptions.get("renderInteractiveForms")) { PDFViewerApplication.save(); } else { PDFViewerApplication.download(); } }

However, ideally you'd actually check if the document has actually been edited first, which once PR #12192 lands could be done easily with:

function webViewerDownload() { if ( PDFViewerApplication.pdfDocument && PDFViewerApplication.pdfDocument.annotationStorage.size > 0 ) { PDFViewerApplication.save(); } else { PDFViewerApplication.download(); } }

#12192 is now merged, so the size check can now be added.

web/app.js

src/core/worker.js

web/app.js

Snuffleupagus · 2020-08-11T09:47:48Z

web/viewer.js

+    ]).then(function ([
+      app,
+      appOptions,
+      genericCom,
+      pdfPrintService,
+      pdfSaveService,
+    ]) {


Please revert this change, since it's now unused.

timvandermeij

Code-wise this looks good to me when the final comments are addressed, namely:

Add support for saving forms #12137 (comment) (the size check should be added now that the other PR is merged)
Add support for saving forms #12137 (comment) (this line should be removed)

Functionality-wise Brendan will check the XRef part, do some manual tests and merge it since I'll be away for a few days. Indeed, any other points can be picked up in a follow-up.

You've clearly put a lot of work into this, and it shows. Thank you for your work!

Snuffleupagus requested changes Jul 28, 2020

View reviewed changes

web/pdf_save_service.js Outdated Show resolved Hide resolved

src/core/writer.js Outdated Show resolved Hide resolved

src/core/worker.js Outdated Show resolved Hide resolved

src/core/writer.js Outdated Show resolved Hide resolved

lundjordan added the Gecko 81 Tracking work planned for Form Fill in Shirley 81 release label Jul 28, 2020

timvandermeij added form-acroform annotations labels Jul 28, 2020

calixteman force-pushed the save branch from 2a8f40b to 21396c0 Compare July 29, 2020 14:26

Snuffleupagus requested changes Jul 29, 2020

View reviewed changes

src/display/api.js Outdated Show resolved Hide resolved

src/core/writer.js Outdated Show resolved Hide resolved

src/core/writer.js Outdated Show resolved Hide resolved

src/core/writer.js Outdated Show resolved Hide resolved

calixteman force-pushed the save branch 8 times, most recently from a5062fc to 2dc13a2 Compare August 4, 2020 11:10

calixteman force-pushed the save branch from 2dc13a2 to 4f97a5f Compare August 7, 2020 10:48

calixteman force-pushed the save branch from 4f97a5f to 6881752 Compare August 9, 2020 16:06

timvandermeij reviewed Aug 9, 2020

View reviewed changes

calixteman force-pushed the save branch from 6881752 to 937398f Compare August 10, 2020 16:46

timvandermeij reviewed Aug 10, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

src/core/writer.js Show resolved Hide resolved

brendandahl approved these changes Aug 11, 2020

View reviewed changes

Snuffleupagus requested changes Aug 11, 2020

View reviewed changes

Snuffleupagus reviewed Aug 11, 2020

View reviewed changes

web/app.js Outdated Show resolved Hide resolved

calixteman force-pushed the save branch from 937398f to 5039741 Compare August 11, 2020 09:09

Snuffleupagus reviewed Aug 11, 2020

View reviewed changes

calixteman force-pushed the save branch from 5039741 to cdc4ffa Compare August 11, 2020 10:40

timvandermeij approved these changes Aug 11, 2020

View reviewed changes

calixteman force-pushed the save branch from cdc4ffa to cb0e4f6 Compare August 12, 2020 08:26

Add support for saving forms

1a6816b

calixteman force-pushed the save branch from cb0e4f6 to 1a6816b Compare August 12, 2020 08:33

brendandahl merged commit 7edc5cb into mozilla:master Aug 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for saving forms #12137

Add support for saving forms #12137

calixteman commented Jul 28, 2020

calixteman commented Jul 28, 2020

Snuffleupagus left a comment

brendandahl commented Jul 29, 2020

calixteman commented Jul 29, 2020

Snuffleupagus left a comment

calixteman commented Aug 7, 2020

timvandermeij left a comment •

edited

Loading

timvandermeij left a comment •

edited

Loading

brendandahl left a comment

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Aug 11, 2020

timvandermeij Aug 11, 2020

Snuffleupagus Aug 11, 2020

timvandermeij left a comment •

edited

Loading

	function webViewerDownload() {
	PDFViewerApplication.download();
	}

Add support for saving forms #12137

Add support for saving forms #12137

Conversation

calixteman commented Jul 28, 2020

calixteman commented Jul 28, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

brendandahl commented Jul 29, 2020

calixteman commented Jul 29, 2020

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Aug 7, 2020

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

brendandahl left a comment

Choose a reason for hiding this comment

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Aug 11, 2020

Choose a reason for hiding this comment

timvandermeij Aug 11, 2020

Choose a reason for hiding this comment

Snuffleupagus Aug 11, 2020

Choose a reason for hiding this comment

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

timvandermeij left a comment •

edited

Loading

timvandermeij left a comment •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

timvandermeij left a comment •

edited

Loading