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

Add a new FirefoxCom.requestAsync method, to simplify the code in web/firefoxcom.js #12802

Merged
merged 2 commits into from
Jan 2, 2021

Conversation

Snuffleupagus
Copy link
Collaborator

Please note: I've tested this patch in a local Firefox build, to ensure that I didn't break anything here :-)

  • Convert FirefoxCom to a class, with static methods

    Please note: It's highly recommended to ignore whitespace-only changes when looking at this patch.

    Besides modernizing this code, by converting it to a standard class, the existing JSDoc comments are updated to actually agree better with the way that this functionality is used now. (The next patch will reduce usage of FirefoxCom.request significantly, hence the JSDocs for the optional callback is removed to not unnecessarily advertise that functionality.)

    Finally, the unnecessary/unused return statement at the end of FirefoxCom.request is also removed.

  • Add a new FirefoxCom.requestAsync method, to simplify the code in web/firefoxcom.js

    There's a fair number of cases where FirefoxCom.request-calls are manually wrapped in a Promise to make it asynchronous. We can reduce the amount of boilerplate code in these cases by introducing a new FirefoxCom.requestAsync method instead.

    Furthermore, a couple of FirefoxCom.request-calls in the DownloadManager are also changed to be asynchronous rather than using callback-functions.
    With this patch, we're thus able to replace a lot of direct usages of FirefoxCom.request with the new FirefoxCom.requestAsync method instead.

*Please note:* It's highly recommended to ignore whitespace-only changes when looking at this patch.

Besides modernizing this code, by converting it to a standard class, the existing JSDoc comments are updated to actually agree better with the way that this functionality is used now. (The next patch will reduce usage of `FirefoxCom.request` significantly, hence the JSDocs for the optional `callback` is removed to not unnecessarily advertise that functionality.)

Finally, the unnecessary/unused `return` statement at the end of `FirefoxCom.request` is also removed.
…web/firefoxcom.js`

There's a fair number of cases where `FirefoxCom.request`-calls are manually wrapped in a Promise to make it asynchronous. We can reduce the amount of boilerplate code in these cases by introducing a new `FirefoxCom.requestAsync` method instead.

Furthermore, a couple of `FirefoxCom.request`-calls in the `DownloadManager` are also changed to be asynchronous rather than using callback-functions.
With this patch, we're thus able to replace a lot of *direct* usages of `FirefoxCom.request` with the new `FirefoxCom.requestAsync` method instead.
@Snuffleupagus Snuffleupagus changed the title Firefox com request async Add a new FirefoxCom.requestAsync method, to simplify the code in web/firefoxcom.js Jan 1, 2021
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Jan 2, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/3ebaf942738a8d9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 2, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/3ebaf942738a8d9/output.txt

Total script time: 4.21 mins

Published

@timvandermeij timvandermeij merged commit c3e02b3 into mozilla:master Jan 2, 2021
@timvandermeij
Copy link
Contributor

Thank you for modernizing this code!

@Snuffleupagus Snuffleupagus deleted the FirefoxCom-requestAsync branch January 2, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants