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

[api-minor] Further modernize the ProgressBar class (PR 14918 follow-up) #15121

Merged
merged 1 commit into from
Jul 2, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

  • Simplify how we look-up the DOM-element, which should also be a tiny bit more efficent.

  • Use private class-fields, rather than property-names prefixed with underscores.

  • Inline the #updateBar helper-method directly in the percent-setter, since having a separate method doesn't seem necessary in this case.

  • Set the indeterminate-class on the ProgressBar DOM-element, to simplify the code.

Finally, also (slightly) re-factors the PDFViewerApplication.progress-method to make it a bit smaller.

…w-up)

 - Simplify how we look-up the DOM-element, which should also be a tiny bit more efficent.

 - Use private class-fields, rather than property-names prefixed with underscores.

 - Inline the `#updateBar` helper-method directly in the `percent`-setter, since having a separate method doesn't seem necessary in this case.

 - Set the `indeterminate`-class on the ProgressBar DOM-element, to simplify the code.

Finally, also (slightly) re-factors the `PDFViewerApplication.progress`-method to make it a bit smaller.
@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/22c9ce6375eccb5/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

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/1568272d7014980/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/1568272d7014980/output.txt

Total script time: 4.73 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jul 1, 2022

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/22c9ce6375eccb5/output.txt

Total script time: 7.63 mins

  • Integration Tests: FAILED

@@ -718,7 +718,7 @@ const PDFViewerApplication = {
},

get loadingBar() {
const bar = new ProgressBar("#loadingBar");
const bar = new ProgressBar("loadingBar");
return shadow(this, "loadingBar", bar);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this patch, does it make sense to shadow the value ?
I suppose that once the document is loaded we don't need it anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to this patch, does it make sense to shadow the value ?

Doing that simplifies the code overall, and also allows us to initialize it lazily, since we're accessing it from multiple spots throughout the viewer code; see e.g.

I suppose that once the document is loaded we don't need it anymore.

Correct; however if we'd remove it at the end of loading that'd (as far as I can tell) either mean having to initialize it eagerly or possibly require adding more code.

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you

@Snuffleupagus Snuffleupagus merged commit 03c6feb into mozilla:master Jul 2, 2022
@Snuffleupagus Snuffleupagus deleted the loadingBar-cleanup branch July 2, 2022 14:47
This pull request was closed.
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