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] Move the addDefaultProtocolToUrl/tryConvertUrlEncoding functionality into the createValidAbsoluteUrl function #14081

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

Having recently worked with, and reviewed patches touching, this code it seemed that it's probably not a bad idea to move that functionality into createValidAbsoluteUrl as new options instead.

For the addDefaultProtocolToUrl functionality in particular, the existing helper function was not only moved but slightly improved as well. Looking at the code, I realized that there's a small risk that it would incorrectly match a relative URL-string too.

With these changes, the createValidAbsoluteUrl call-sites in the src/core/-code can be simplified a little bit.

Please note: This patch may, indirectly, change the format of the unsafeUrl-property returned with relevant Annotations and OutlineItems; hence the api-minor tag.
However, I'd argue that it's actually more correct this way since the whole purpose of unsafeUrl is/was to return the URL data as-is without any parsing done.

…` functionality into the `createValidAbsoluteUrl` function

Having recently worked with, and reviewed patches touching, this code it seemed that it's probably not a bad idea to move that functionality into `createValidAbsoluteUrl` as new options instead.

For the `addDefaultProtocolToUrl` functionality in particular, the existing helper function was not only moved but slightly improved as well. Looking at the code, I realized that there's a small risk that it would incorrectly match a *relative* URL-string too.

With these changes, the `createValidAbsoluteUrl` call-sites in the `src/core/`-code can be simplified a little bit.

*Please note:* This patch may, indirectly, change the format of the `unsafeUrl`-property returned with relevant Annotations and OutlineItems; hence the `api-minor` tag.
However, I'd argue that it's actually more correct this way since the whole purpose of `unsafeUrl` is/was to return the URL data as-is without any parsing done.
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/04f819f5bbe8542/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/5ec109e1caafb11/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/04f819f5bbe8542/output.txt

Total script time: 2.77 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/5ec109e1caafb11/output.txt

Total script time: 6.72 mins

  • Unit Tests: Passed

@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/c5e2368148259c1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/c5e2368148259c1/output.txt

Total script time: 4.20 mins

Published

@timvandermeij timvandermeij merged commit 93ed4bf into mozilla:master Sep 26, 2021
@timvandermeij
Copy link
Contributor

Good idea; thanks!

@Snuffleupagus Snuffleupagus deleted the createValidAbsoluteUrl-options branch September 26, 2021 13:11
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