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

Remove the sourceEventType from the viewer (bug 1757771 follow-up) #14926

Merged
merged 2 commits into from
May 21, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

  • Remove the sourceEventType from the viewer (bug 1757771 follow-up)

    After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the sourceEventType is now completely unused and should thus be removed (in my opinion).

    Furthermore, with these changes, we also no longer need a separate internal "save"-event and can instead just use the older "download"-event everywhere.

  • [Firefox viewer] Stop using FirefoxCom.requestAsync in the DownloadManager

    After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the ChromeActions.download-method will no longer invoke the sendResponse-callback.
    Hence it should no longer be necessary for the DownloadManager, in the MOZCENTRAL viewer, to use FirefoxCom.requestAsync since no response is ever provided.[1] For the allocated BlobURLs, they should (hopefully) be released when navigating away from the viewer.


    [1] Note that that was already the case, for one of the previous code-paths in the ChromeActions.download-method.

After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the `sourceEventType` is now completely unused and should thus be removed (in my opinion).

Furthermore, with these changes, we also no longer need a *separate* internal "save"-event and can instead just use the older "download"-event everywhere.
…dManager`

After the changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1757771, that simplified the MOZCENTRAL downloading code, the `ChromeActions.download`-method will no longer invoke the `sendResponse`-callback.
Hence it should no longer be necessary for the `DownloadManager`, in the MOZCENTRAL viewer, to use `FirefoxCom.requestAsync` since no response is ever provided.[1] For the allocated BlobURLs, they should (hopefully) be released when navigating away from the viewer.

---
[1] Note that that was *already* the case, for one of the previous code-paths in the `ChromeActions.download`-method.
@Snuffleupagus
Copy link
Collaborator Author

/cc @calixteman Should we perhaps wait a few days before landing this PR, to ensure that the patch in bug 1757771 will actually "stick" first?

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/455a75b0eedc9e1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/6e4fe889ad79398/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/455a75b0eedc9e1/output.txt

Total script time: 4.34 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/6e4fe889ad79398/output.txt

Total script time: 7.91 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

/cc @calixteman Should we perhaps wait a few days before landing this PR, to ensure that the patch in bug 1757771 will actually "stick" first?

Yep it's wiser to wait few days.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2bc758ca087f46e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/2bc758ca087f46e/output.txt

Total script time: 2.54 mins

Published

@timvandermeij timvandermeij merged commit 42a6217 into mozilla:master May 21, 2022
@timvandermeij
Copy link
Contributor

Nothing got reverted upstream, so let's do this. Thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants