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

[droid] "Uri too long" in WebView.NavigateToString remediation #2023

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

pkar70
Copy link
Contributor

@pkar70 pkar70 commented Nov 5, 2019

GitHub Issue (If applicable):
#1527

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

When WebView.NavigateToString(long_string)
exception is thrown: "uri too long".

What is the new behavior?

no exception is thrown

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code on Android 8.1 Oreo Go (with current [supported SDKs])
  • Docs have been added/updated which fit documentation template (for bug fixes / features)
  • Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
  • [Wasm UI Tests] (not applicable)
  • Contains NO breaking changes
  • Updated the Release Notes
  • Associated with an issue (GitHub or internal)

Other information

Recreated PR 1732 (as probably only of squashing way available to me).
Many thanks to Sébastien Goyette for helping me in creating tests for this PR.

CI is failing for WASM, as error I corrected on Android still exist on WASM.

Copy link
Contributor

@davidjohnoliver davidjohnoliver left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! A few tweaks and it's ready.

src/Uno.UI/UI/Xaml/Controls/WebView/WebView.Android.cs Outdated Show resolved Hide resolved
// step 1: generate long string
_app.Tap(startButton);

// _app.Wait(TimeSpan.FromSeconds(300)); // Android: 20 is sufficient
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete the commented code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented code is left by purpose - as for WASM (which is interpreter, not compiler) default timeout can be too short, and maybe adding _Wait would be necessary for this.
But OK, I will delete it (but it means that I have to re-create PR, as I was told that it had to be single commit in PR, and I don't know how to make changes without adding new commit to PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to recreate the PR. It's fine to add additional commits that address PR feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't have multiple extraneous commits - commit #1 'Fix X', commit #2 'X still not working, try something else', commit #3 'Fixed a typo' etc. There's no rule against multiple commits per se.

The rule of thumb is that a commit should represent a single complete, logical unit of work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, one commit per one changed file is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also on the subject of commits - for the future please try to give each one a meaningful commit message. Don't just use the default 'Updated file.cs' when you're using the GitHub web interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All corrections are made.
So, can this PR be approved/merged? :)

@davidjohnoliver davidjohnoliver merged commit caaee11 into unoplatform:master Nov 7, 2019
@pkar70 pkar70 deleted the UriTooLong2 branch November 7, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants