Replace Google with DuckDuckGo as the optional fallback favicon fetcher #2309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kneitinger
force-pushed
the
feature/duckduckgo-for-favicon-fallback
branch
2 times, most recently
from
265d530 to
8f18515
Compare
droidmonkey
requested changes
Sep 20, 2018
src/core/Config.cpp
Outdated
| @@ -148,7 +148,7 @@ void Config::init(const QString& fileName) | |||
| m_defaults.insert("security/passwordscleartext", false); | |||
| m_defaults.insert("security/hidepassworddetails", true); | |||
| m_defaults.insert("security/autotypeask", true); | |||
| m_defaults.insert("security/IconDownloadFallbackToGoogle", false); | |||
| m_defaults.insert("security/IconDownloadFallbackToDuckDuckGo", false); | |||
There was a problem hiding this comment.
It might be best to name this "security/IconDownloadFallback" in case we change the service again in the future or allow for a choice in services.
src/gui/EditWidgetIcons.cpp
Outdated
| if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool()) { | ||
| QUrl urlGoogle = QUrl("https://www.google.com/s2/favicons"); | ||
| // Try to use DuckDuckGo fallback, if enabled | ||
| if (config()->get("security/IconDownloadFallbackToDuckDuckGo", false).toBool()) { |
There was a problem hiding this comment.
Again, let's keep this generic: "QUrl fallbackUrl = ..."
droidmonkey
approved these changes
Sep 20, 2018
Modify the work initially done in keepassxreboot#36, and most recently modified in keepassxreboot#1786, to use DuckDuckGo's https://icons.duckduckgo.com/ip3/www.example.com.ico favicon endpoint. Fixes keepassxreboot#2258
Name the UrlFetchProgressDialog() with the corresponding URL in order to be identified by name by its parent when the failed request is handeled in EditWidgetIcons::fetchFinished(). fetchFinished() retrieves the relevant UrlFetchProgressDialog() and calls close() on it. Fixes: keepassxreboot#2265
droidmonkey
force-pushed
the
feature/duckduckgo-for-favicon-fallback
branch
from
54519fe to
b4ae46f
Compare
|
Whoops, something crazy happened. I rebased your changes onto develop and lost your last commit. Please reapply your fixes to the current head, thanks. |
Change variables that reference the current external favicon fallback service, DuckDuckGo, to instead generically reference fallback URLs generically, to facilitate ease of any potential future service switch
droidmonkey
approved these changes
Sep 21, 2018
|
Great work, looking forward to more PR's! |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Transition to DuckDuckGo as Fallback Favicon Fetcher
Modified the fallback HTTP request to use DuckDuckGo (which uses paths to specify the site whose favicon should be retrieved) instead of Google which used params to specify the target site). Additionally, changed references to "Google" to "DuckDuckGo" in ui elements and variables.
Close Failed Favicon Fetch Progress Bars
Prior to my fix, when a fetch failed, it was being hidden (and not ever closed) immediately after failing, only to be unhidden by the timeout specified by
setMinimumDuration()being reached. InEditWidgetIcons::fetchFinished(), the error state of the fetch is established, so it seemed like the correct place to close theUrlFetchProgressDialog. The only issue is that the progress widget is only ever bound to a variable locally in a different function. TheEditWidgetIconsobject is the parent of the progress widget, so we can get a reference to the progress widget viafindChild(), however we need a name to lookup to use it. Since the url being queried is known at widget create time and destroy time, that seemed like the ideal choice to uniquely name the object.Motivation and context
Transition to DuckDuckGo as fallback fetcher fixes #2258
Closing of progress bars of failed fetches fixes #2265
How has this been tested?
First, ran all existing unit test successfully. Then, manually tested functionality:
https://fedex.comis a URL that I know will always fall through to the fallback fetcher if enabled, so I deleted it's icon, and tried to fetch:Screenshot shows that DuckDuckGo is the optional fallback that can be enabled according to the message.
/favicon.icofile, and DuckDuckGo) fail and their progress bars disappear. An error message is correctly displayed:Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]