Bug 1803465 - extend isURLLenient to match IPv6 literals #4090

pmarks-net · 2023-10-14T18:04:05Z

This pull request lets Firefox for Android accept IPv6 literals typed into the address bar.

For example: https://[2606:4700:4700::1111]/

The bug was originally reported as mozilla-mobile/fenix#4343, but @YoRyan's mozilla-mobile/android-components#5546 was rejected because it grew a big slow regex.

The big regex no longer exists (yay!) leaving this 1-line isURLLenient regex as the final roadblock.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1803465
https://bugzilla.mozilla.org/show_bug.cgi?id=1807752

pmarks-net · 2023-10-14T19:36:51Z

@csadilek and @mcomella originally wrote this regex.

pmarks-net · 2023-10-14T19:41:23Z

I tested this using https://firefox-source-docs.mozilla.org/mobile/android/fenix.html and ./gradlew support-utils:test.

I'm not sure if the 3 workflow failures are my fault, nor how to rerun them.
Edit: Fixed the failures by wrapping a long line in WebURLFinder.kt.

github-actions · 2023-10-14T19:46:48Z

🚧 Commit message is using the wrong format: Fix [MaxLineLength] code style failure.

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

Djfe · 2023-10-15T07:26:47Z

great, thank you for working on this 👍
Would it make sense to also introduce some assertFalse?
like for misspelled ipv6 style urls?

edit: also there is no positive test for a full URL without a double colon, like this one:
http://[2a03:2260:3006:115:3e37:12ff:fe82:cbab]/

pmarks-net · 2023-10-15T14:44:12Z

@Djfe this regex has no concept of a "misspelled" IP address. It's not looking at the address itself, just very broad character classes to distinguish possible URLs from garbage.

pmarks-net · 2023-10-19T18:55:44Z

Added more test cases. When I type these into the UI, it either shows "Invalid Address" or a completely blank tab, depending on the mood of the downstream URL parser.

Note that both of these states are already reachable in Firefox 118:

http://x:x -> "Invalid Address"
x://x -> blank tab

github-actions · 2023-10-24T16:57:21Z

🚧 Commit message is using the wrong format: Import translations from android-l10n

The comment message should look like:

    Bug xxxx - Short description of your change
    Optionally, a longer description of the change.

mergify · 2023-10-24T16:57:43Z

This pull request has conflicts when rebasing. Could you fix it @pmarks-net? 🙏

Also known as Bug 1807752

This lets the "Fill link from clipboard" feature accept IPv6 literals like https://[2606:4700:4700::1111]

mergify · 2023-11-03T21:18:57Z

This pull request has conflicts when rebasing. Could you fix it @pmarks-net? 🙏

pmarks-net · 2023-11-08T21:49:16Z

I'm still waiting for a reviewer. @csadilek perhaps?

titooan

Hi pmarks-net,
Thanks a lot for this PR and sorry for the late review.

While we were reviewing this, we tried to think of all the cases and here are some suggestions we came up with, that you can find in this patch file.

Please feel free to ask for clarifications is anything is not clear enough.

...omponents/support/utils/src/test/java/mozilla/components/support/utils/URLStringUtilsTest.kt

.../components/support/utils/src/test/java/mozilla/components/support/utils/WebURLFinderTest.kt

...omponents/support/utils/src/test/java/mozilla/components/support/utils/URLStringUtilsTest.kt

.../components/support/utils/src/test/java/mozilla/components/support/utils/WebURLFinderTest.kt

...omponents/support/utils/src/test/java/mozilla/components/support/utils/URLStringUtilsTest.kt

titooan

Thanks a lot for the corrections you made!
I added a few suggestions, please feel free to ask if anything is unclear.

pmarks-net · 2023-11-17T20:30:05Z

GitHub says "Merging is blocked - Merging can be performed automatically once the requested changes are addressed", so I'm going to try the "Re-request review" button.

titooan

Thanks a lot for those fixes!

Let's just fix one last comment, and it's good to ship.

titooan · 2023-11-20T10:50:21Z

.../components/support/utils/src/test/java/mozilla/components/support/utils/WebURLFinderTest.kt

+        // All cases that behave differently are annotated with INVERT().
+        val INVERT: (Boolean) -> Boolean = { !it }


Sorry, I forgot to specify that in my previous comment: we are adding complexity with this annotation-like function, that I think we could replace with clear comments around the tests.

We could replace the use of assertTrue(INVERT(isURLLike(xxx))) by assertFalse(isURLLike(xxx)). Those asserts could be gathered with a comment above explaining that the ideal regex should have the same result as the one in UrlStringUtils.kt, but the current one gives a different result. Also explaining that we allow it for now, until bug 1685152 is fixed.

What do you think?

replace the use of assertTrue(INVERT(isURLLike(xxx))) by assertFalse(isURLLike(xxx)). Those asserts could be gathered with a comment

I intentionally kept assertTrue/assertFalse the same as the original tests, and replaced the comments with a programmatic annotation, in order to reduce the cognitive effort required to maintain a second copy. Your suggestion would increase complexity when both functions are considered.

I do understand the idea and the value of trying to keep both tests as similar as possible.
Nevertheless, this is starting a new pattern in the codebase, and maybe we should try to follow the existing conventions instead?

Maybe we could get a second opinion from someone else? cc: @jonalmeida

Hey! I agree with @titooan, I too would prefer avoiding new patterns. Adding these cases that should pass/fail can be in a separate section with a well-worded comment to explain that these should be have differently when the linked bug is fixed.

I intentionally kept assertTrue/assertFalse the same as the original tests, and replaced the comments with a programmatic annotation, in order to reduce the cognitive effort required to maintain a second copy. Your suggestion would increase complexity when both functions are considered.

The felt complexity of both solutions are indeed subjective. In cases like this, I would recommend the advice of the code maintainers to decide what is the more preferred way, especially in terms of conventions the codebase follows. 🙂

mergify · 2023-11-21T18:02:36Z

This pull request has conflicts when rebasing. Could you fix it @pmarks-net? 🙏

titooan · 2023-11-22T09:59:05Z

bors try

jonalmeida · 2023-11-22T14:27:22Z

bors try

It looks like the CI we depend on for our contributor PRs is down, so I've pushed a separate branch to run all our CI tests.

jonalmeida · 2023-11-22T14:46:12Z

bors try

It looks like the CI we depend on for our contributor PRs is down, so I've pushed a separate branch to run all our CI tests.

CI has passed! I've pushed a small update to correct the changelog entry since we've cut the previous version already. Let's land this!

titooan

Thanks a lot for your contribution @pmarks-net, it looks pefect to me!
Let's land it!

mergify · 2023-11-22T16:22:13Z

This pull request has conflicts when rebasing. Could you fix it @pmarks-net? 🙏

pmarks-net · 2024-01-25T21:27:58Z

Firefox 122 (with the IPv6 literals fix) is now live on the Play store.

I'm mentioning mozilla-mobile/fenix#4343 to notify the original reporters.

github-actions bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Oct 14, 2023

pmarks-net force-pushed the main branch from 5a566e5 to cd8f8c8 Compare October 14, 2023 19:49

pmarks-net force-pushed the main branch 2 times, most recently from 2288c33 to bb2211f Compare October 24, 2023 17:22

pmarks-net added 5 commits October 24, 2023 14:21

Bug 1803465 - extend isURLLenient to match IPv6 literals

a4bc4e7

Also known as Bug 1807752

Bug 1803465 - Keep autolinkWebUrlPattern in sync with isURLLenient.

6ab5019

This lets the "Fill link from clipboard" feature accept IPv6 literals like https://[2606:4700:4700::1111]

Bug 1803465 - more IPv6 test cases for isURLLenient

5202c23

Bug 1803465 - Update changelog.md

1cd491a

Bug 1803465 - Bump IPv6 literals to Firefox 121

9760f84

pmarks-net force-pushed the main branch from bb2211f to 9760f84 Compare October 24, 2023 18:22

Merge branch 'main' into main

2e679f9

jonalmeida self-requested a review November 9, 2023 10:38

github-actions bot assigned jonalmeida Nov 9, 2023

jonalmeida requested a review from titooan November 9, 2023 10:39

github-actions bot assigned titooan Nov 9, 2023

pmarks-net added 2 commits November 13, 2023 15:37

Merge branch 'main' into main

e5ad176

Merge branch 'main' into main

4e5ca51

titooan requested changes Nov 16, 2023

View reviewed changes

pmarks-net added 2 commits November 16, 2023 16:06

Bug 1803465 - Reorganize unit tests

674be27

Merge branch 'main' into main

329b7a5

Merge branch 'main' into main

8f11276

titooan approved these changes Nov 17, 2023

View reviewed changes

titooan self-requested a review November 17, 2023 16:13

github-actions bot added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 17, 2023

titooan requested changes Nov 17, 2023

View reviewed changes

Bug 1803465 - More unit tests

39f09b2

github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed approved PR that has been approved labels Nov 17, 2023

Merge branch 'main' into main

417bfeb

pmarks-net requested a review from titooan November 17, 2023 20:30

titooan requested changes Nov 20, 2023

View reviewed changes

pmarks-net added 2 commits November 21, 2023 22:09

Bug 1803465 - Replace INVERT() with comments.

2f6a576

Merge branch 'main' into main

90ca11b

github-actions bot mentioned this pull request Nov 22, 2023

CI for #4090 #4566

Closed

4 tasks

Bug 1803465 - Update changelog

9591c66

jonalmeida added 🛬 needs landing (squash) PRs that are ready to land (squashed) and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 22, 2023

titooan approved these changes Nov 22, 2023

View reviewed changes

titooan added the approved PR that has been approved label Nov 22, 2023

pmarks-net and others added 2 commits November 22, 2023 11:23

Merge branch 'main' into main

0c6285a

Merge branch 'main' into main

b9a23a9

mergify bot merged commit ec7b003 into mozilla-mobile:main Nov 22, 2023
126 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1803465 - extend isURLLenient to match IPv6 literals #4090

Bug 1803465 - extend isURLLenient to match IPv6 literals #4090

pmarks-net commented Oct 14, 2023 •

edited by github-actions bot

pmarks-net commented Oct 14, 2023

pmarks-net commented Oct 14, 2023 •

edited

github-actions bot commented Oct 14, 2023

Djfe commented Oct 15, 2023 •

edited

pmarks-net commented Oct 15, 2023

pmarks-net commented Oct 19, 2023

github-actions bot commented Oct 24, 2023

mergify bot commented Oct 24, 2023

mergify bot commented Nov 3, 2023

pmarks-net commented Nov 8, 2023

titooan left a comment

titooan left a comment

pmarks-net commented Nov 17, 2023

titooan left a comment

titooan Nov 20, 2023

pmarks-net Nov 20, 2023

titooan Nov 21, 2023

jonalmeida Nov 21, 2023

pmarks-net Nov 22, 2023

mergify bot commented Nov 21, 2023

titooan commented Nov 22, 2023

jonalmeida commented Nov 22, 2023

jonalmeida commented Nov 22, 2023

titooan left a comment

mergify bot commented Nov 22, 2023

pmarks-net commented Jan 25, 2024

		// All cases that behave differently are annotated with INVERT().
		val INVERT: (Boolean) -> Boolean = { !it }

Bug 1803465 - extend isURLLenient to match IPv6 literals #4090

Bug 1803465 - extend isURLLenient to match IPv6 literals #4090

Conversation

pmarks-net commented Oct 14, 2023 • edited by github-actions bot

Pull Request checklist

After merge

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

pmarks-net commented Oct 14, 2023

pmarks-net commented Oct 14, 2023 • edited

github-actions bot commented Oct 14, 2023

Djfe commented Oct 15, 2023 • edited

pmarks-net commented Oct 15, 2023

pmarks-net commented Oct 19, 2023

github-actions bot commented Oct 24, 2023

mergify bot commented Oct 24, 2023

mergify bot commented Nov 3, 2023

pmarks-net commented Nov 8, 2023

titooan left a comment

Choose a reason for hiding this comment

titooan left a comment

Choose a reason for hiding this comment

pmarks-net commented Nov 17, 2023

titooan left a comment

Choose a reason for hiding this comment

titooan Nov 20, 2023

Choose a reason for hiding this comment

pmarks-net Nov 20, 2023

Choose a reason for hiding this comment

titooan Nov 21, 2023

Choose a reason for hiding this comment

jonalmeida Nov 21, 2023

Choose a reason for hiding this comment

pmarks-net Nov 22, 2023

Choose a reason for hiding this comment

mergify bot commented Nov 21, 2023

titooan commented Nov 22, 2023

jonalmeida commented Nov 22, 2023

jonalmeida commented Nov 22, 2023

titooan left a comment

Choose a reason for hiding this comment

mergify bot commented Nov 22, 2023

pmarks-net commented Jan 25, 2024

pmarks-net commented Oct 14, 2023 •

edited by github-actions bot

pmarks-net commented Oct 14, 2023 •

edited

Djfe commented Oct 15, 2023 •

edited