Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Scripting] Try to ensure that the WillPrint/DidPrint respectively DidSave events are always dispatched #12771

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

  • [Scripting] Try to ensure that the WillPrint/DidPrint respectively DidSave events are always dispatched

    Note that currently the DidSave event is not guaranteed to actually be dispatched if there's any errors during saving, which is easily fixed by simply moving it to occur in the finally-handler in PDFViewerApplication.save method.

    For the WillPrint/DidPrint events, things are unfortunately more complicated. Currently these events will only be dispatched iff the printing request comes from within the viewer itself (e.g. by the user clicking on the "Print" toolbar button), however printing can be triggered in a few additional ways:

    • In the GENERIC viewer:
      • By the Ctrl+P keyboard shortcut.
    • In the MOZCENTRAL viewer, i.e. the Firefox built-in viewer:
      • By the Ctrl+P keyboard shortcut.
      • By the "Print" item, as found in either the Firefox "Hamburger menu" or in the browser-window menu.

    In either of the cases described above, no WillPrint/DidPrint events will be dispatched. In order to guarantee that things work in the general case, we thus have to move the dispatchEventInSandbox calls to the "beforeprint"/"afterprint" event handlers instead.

  • [Scripting] Await manually triggered dispatchEventInSandbox calls in the viewer

    Given that the dispatchEventInSandbox method (on the scripting-classes) is asynchronous, there's a very real risk that the events won't be dispatched/handled until after their associated functionality has actually run (with the "Will..." events being particularily susceptible to this issue).
    To reduce the likelihood of that happening, we can simply await the dispatchEventInSandbox calls as necessary. A couple of methods are now marked as async to support these changes, however that shouldn't be a problem as far as I can tell.

    Please note: Given that the browser "beforeprint"/"afterprint" events are synchronous, we unfortunately cannot await the WillPrint/DidPrint event dispatching. To fix this properly the web-platform would need support for asynchronous printing, and we'll thus have to hope that things work correctly anyway.

/cc @calixteman

…y `DidSave` events are always dispatched

Note that currently the `DidSave` event is not *guaranteed* to actually be dispatched if there's any errors during saving, which is easily fixed by simply moving it to occur in the `finally`-handler in `PDFViewerApplication.save` method.

For the `WillPrint`/`DidPrint` events, things are unfortunately more complicated. Currently these events will *only* be dispatched iff the printing request comes from within the viewer itself (e.g. by the user clicking on the "Print" toolbar button), however printing can be triggered in a few additional ways:
 - In the GENERIC viewer:
   - By the <kbd>Ctrl</kbd>+<kbd>P</kbd> keyboard shortcut.
 - In the MOZCENTRAL viewer, i.e. the Firefox built-in viewer:
   - By the <kbd>Ctrl</kbd>+<kbd>P</kbd> keyboard shortcut.
   - By the "Print" item, as found in either the Firefox "Hamburger menu" or in the browser-window menu.

In either of the cases described above, no `WillPrint`/`DidPrint` events will be dispatched. In order to *guarantee* that things work in the general case, we thus have to move the `dispatchEventInSandbox` calls to the "beforeprint"/"afterprint" event handlers instead.
@Snuffleupagus Snuffleupagus force-pushed the viewer-dispatchEventInSandbox-fixes branch from 256d603 to 7e35a8c Compare December 23, 2020 11:02
…n the viewer

Given that the `dispatchEventInSandbox` method (on the scripting-classes) is asynchronous, there's a very real risk that the events won't be dispatched/handled until *after* their associated functionality has actually run (with the "Will..." events being particularily susceptible to this issue).
To reduce the likelihood of that happening, we can simply `await` the `dispatchEventInSandbox` calls as necessary. A couple of methods are now marked as `async` to support these changes, however that shouldn't be a problem as far as I can tell.

*Please note:* Given that the browser "beforeprint"/"afterprint" events are *synchronous*, we unfortunately cannot await the `WillPrint`/`DidPrint` event dispatching. To fix this properly the web-platform would need support for asynchronous printing, and we'll thus have to hope that things work correctly anyway.
@Snuffleupagus Snuffleupagus force-pushed the viewer-dispatchEventInSandbox-fixes branch from 7e35a8c to a4786c9 Compare December 23, 2020 11:03
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://3.101.106.178:8877/08ff9f0af4470b6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7b1b5445fe690da/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/7b1b5445fe690da/output.txt

Total script time: 2.63 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/08ff9f0af4470b6/output.txt

Total script time: 3.61 mins

  • Integration Tests: Passed

@calixteman
Copy link
Contributor

I added some integration tests here: #12774
to test print/save actions.
Anyway this patch lgtm, thanks a lot for doing this: your help is very useful and valuable.

@calixteman calixteman merged commit 0a7d594 into mozilla:master Dec 24, 2020
@Snuffleupagus Snuffleupagus deleted the viewer-dispatchEventInSandbox-fixes branch December 24, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants