Replace qhttp client with curl for favicon downloading #1460
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
droidmonkey
force-pushed
the
refactor/use-curl
branch
from
c656759 to
cf2652c
Compare
phoerious
requested changes
Feb 6, 2018
.travis.yml
Outdated
| @@ -23,7 +23,7 @@ git: | |||
|
|
|||
| before_install: | |||
| - if [ "$TRAVIS_OS_NAME" = "linux" ]; then sudo apt-get -qq update; fi | |||
| - if [ "$TRAVIS_OS_NAME" = "linux" ]; then sudo apt-get -qq install cmake3 libclang-common-3.5-dev libxi-dev qtbase5-dev libqt5x11extras5-dev qttools5-dev qttools5-dev-tools libgcrypt20-dev zlib1g-dev libxtst-dev xvfb libyubikey-dev libykpers-1-dev; fi | |||
There was a problem hiding this comment.
Any specific reason to keep this file at all?
There was a problem hiding this comment.
No, now is a good time to trash it!
src/CMakeLists.txt
Outdated
| add_subdirectory(http/qhttp) | ||
| set(keepassxcnetwork_LIB qhttp Qt5::Network) | ||
| find_package(CURL REQUIRED) | ||
| set(keepassxcnetwork_LIB curl) |
There was a problem hiding this comment.
Use ${CURL_LIBRARIES} instead of hardcoding the library name.
I think you should also remove this line altogether and use ${CURL_LIBRARIES} directly in target_link_libraries. Then we can get rid of ${keepassxchttp_LIB} when we drop KeepassHTTP.
src/gui/EditWidgetIcons.cpp
Outdated
| } | ||
|
|
||
| void EditWidgetIcons::fetchFaviconFromGoogle(const QString& domain) | ||
| #ifdef WITH_XC_NETWORKING | ||
| size_t writeCurlResponse(char* ptr, size_t size, size_t nmemb, void* data) |
There was a problem hiding this comment.
std::size_t
Function should also be wrapped in anonymous namespace.
src/gui/EditWidgetIcons.cpp
Outdated
| } | ||
| QImage image; | ||
| CURL* curl = curl_easy_init(); | ||
| if (curl != nullptr) { |
src/gui/EditWidgetIcons.cpp
Outdated
| QFutureWatcher<CURLcode> watcher; | ||
| connect(&watcher, SIGNAL(finished()), &loop, SLOT(quit())); | ||
| watcher.setFuture(future); | ||
| loop.exec(); |
There was a problem hiding this comment.
We have helper functions for this now.
src/gui/EditWidgetIcons.cpp
Outdated
| m_httpClient = nullptr; | ||
| if (future.result() == CURLE_OK) { | ||
| image.loadFromData(imagedata); | ||
| } |
There was a problem hiding this comment.
Maybe show an error message otherwise?
There was a problem hiding this comment.
This is a worker function, the error messages are handled in the parent caller (downloadFavicon())
src/gui/EditWidgetIcons.cpp
Outdated
| } | ||
| QByteArray* response = static_cast<QByteArray*>(data); | ||
| size_t realsize = size * nmemb; | ||
| response->append(ptr, realsize); |
There was a problem hiding this comment.
Since we know the real size, you can use response->reserve() to pre-allocate memory.
There was a problem hiding this comment.
Actually reserve doesn't make sense in this context because we are adding the new data received to an existing buffer. The append function is accurate.
There was a problem hiding this comment.
True. I somehow added a loop in my head, which isn't actually there.
|
Ready for squash-merge |
phoerious
approved these changes
Feb 7, 2018
|
🎉 |
phoerious
added a commit
that referenced
this pull request
Feb 27, 2018
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]
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
Title says it all. This PR replaces #1169
Motivation and context
qhttpengine is not ready for dependency prime time and is holding up 2.3.0, retaining qhttp for the http server is the best choice at this time.
How has this been tested?
Manually and unit tests.
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]