-
Notifications
You must be signed in to change notification settings - Fork 929
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
Clear entries from LevelDB #5404
Conversation
9562bfd
to
af0cf12
Compare
37a55d2
to
8bfc686
Compare
5fde4bd
to
e610184
Compare
7590db5
to
627a8e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments but LGTM and tested as expected
app/src/main/java/com/duckduckgo/app/browser/localstorage/LocalStorageManager.kt
Outdated
Show resolved
Hide resolved
Timber.d("LocalStorageManager: Allowed domains: $domains") | ||
Timber.d("LocalStorageManager: Matching regex: $matchingRegex") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mental note, we need to migrate all logs to logcat
. String interpolation will be executed in production even if logs are disabled. Which is wasteful and in hot paths perf can even be noticeable if the strings are big
No need to do anything here
private val jsonAdapter by lazy { buildJsonAdapter() } | ||
|
||
private fun buildJsonAdapter(): JsonAdapter<SettingsJson> { | ||
val moshi = Moshi.Builder().add(KotlinJsonAdapterFactory()).build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I wonder if we should directly add the KotlinJsonAdapterFactory
to the moshi instance that it's in DI.
I see we're creating new instances of moshi everywhere just because the DI instance doens't have that adapter
Also no need to do it in this PR
app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt
Outdated
Show resolved
Hide resolved
@aitorvs I’ve renamed all instances of local storage to web local storage, thanks for the review! 🙏 |
Task/Issue URL: https://app.asana.com/0/488551667048375/1208996865644937/f
Description
Selectively clears entries from the
WebView
’s LevelDB.Steps to test this PR
Point at the JSON blob linked in the task
cnn.com
cnn.com
entries are deleted (And any other sites you may have visited), e.g.