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

(android) New 'enablethirdpartycookies' option and code clean-ups #967

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fquirin
Copy link

@fquirin fquirin commented Jul 19, 2022

Platforms affected

Android.

Motivation and Context

  • Third party cookies should be configurable and not just simply on by default
  • I've done some additional code improvements to better handle resources
  • Fixed a type and some potentially confusing, redundant code

Description

  • Third party cookies are often used for tracking and thus should at least be configurable.
  • The code for loading drawables kept repeating so I decided to introduce a function to handle it and making it easier to adapt in case of changes. In addition I thought it was useful to handle all required resources as static variables at the top.
  • 'hardwarebackbutton' was misspelled, so I fixed that.
  • 'showWebPage' redefined some defaults that were already defined at the top which could be very confusing if the default has to be changed in the future.
  • Minor code clean-ups for better readability.

Note: I noticed there is a lot more potential for optimizing the code. Android Studio itself is already pointing out several simplifications. If there is enough interest I'd consider creating another PR :-).

Testing

  • Built the app with the new version
  • Checked if browser loads as expected
  • Changed third party cookie support with new option
  • Testes results with https://www.whatismybrowser.com/detect/are-third-party-cookies-enabled
  • All working as expected :-)

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants