Merge lp:~mandel/ubuntu-download-manager/content-disposition into lp:ubuntu-download-manager
- content-disposition
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Manuel de la Peña | ||||
Approved revision: | 322 | ||||
Merged at revision: | 318 | ||||
Proposed branch: | lp:~mandel/ubuntu-download-manager/content-disposition | ||||
Merge into: | lp:ubuntu-download-manager | ||||
Diff against target: |
350 lines (+125/-9) 10 files modified
debian/changelog (+7/-0) debian/libubuntu-download-manager-client0.symbols (+0/-9) src/common/priv/ubuntu/transfers/system/network_reply.cpp (+10/-0) src/common/priv/ubuntu/transfers/system/network_reply.h (+2/-0) src/downloads/priv/ubuntu/downloads/file_download.cpp (+52/-0) src/downloads/priv/ubuntu/downloads/file_download.h (+1/-0) src/downloads/priv/ubuntu/downloads/manager.h (+1/-0) src/uploads/priv/ubuntu/uploads/manager.h (+1/-0) tests/common/network_reply.h (+2/-0) tests/downloads/daemon/test_download.cpp (+49/-0) |
||||
To merge this branch: | bzr merge lp:~mandel/ubuntu-download-manager/content-disposition | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Sheldon (community) | Approve | ||
PS Jenkins bot | continuous-integration | Approve | |
Andrew Hayzen (community) | Approve | ||
Review via email: mp+228804@code.launchpad.net |
Commit message
Parse the content disposition header for the file name to use.
Description of the change
The solution is not perfect but it will cover most of the cases (if not all) for the content-disposition header. The implementation is very similar to the one found in webkit.
- 317. By Manuel de la Peña
-
Fix small issue in the parser.
PS Jenkins bot (ps-jenkins) wrote : | # |
- 318. By Manuel de la Peña
-
Update the change log.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:318
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 319. By Manuel de la Peña
-
Extra logging.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:319
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 320. By Manuel de la Peña
-
Removed destructors from symbols.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:320
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
- 321. By Manuel de la Peña
-
Ensure thar we remove spaces from the filename key.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:321
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Andrew Hayzen (ahayzen) wrote : | # |
I can confirm that using the debs provided in [0] and the use case for the music-app [1] that the correct filename is given to the file.
Thanks for the quick fix!
0 - http://
1 - https:/
- 322. By Manuel de la Peña
-
Merged with trunk.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:321
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:322
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Michael Sheldon (michael-sheldon) wrote : | # |
Looks good to me!
Preview Diff
1 | === modified file 'debian/changelog' |
2 | --- debian/changelog 2014-07-25 15:47:28 +0000 |
3 | +++ debian/changelog 2014-08-05 14:39:05 +0000 |
4 | @@ -1,3 +1,10 @@ |
5 | +ubuntu-download-manager (0.8) UNRELEASED; urgency=medium |
6 | + |
7 | + * Read the content-dispostion header so that files downloaded use a more |
8 | + decent filename. |
9 | + |
10 | + -- Manuel de la Pena <mandel@mark-v> Wed, 30 Jul 2014 12:41:35 +0200 |
11 | + |
12 | ubuntu-download-manager (0.7+14.10.20140725-0ubuntu1) utopic; urgency=low |
13 | |
14 | [ Manuel de la Peña ] |
15 | |
16 | === modified file 'debian/libubuntu-download-manager-client0.symbols' |
17 | --- debian/libubuntu-download-manager-client0.symbols 2014-07-07 08:58:05 +0000 |
18 | +++ debian/libubuntu-download-manager-client0.symbols 2014-08-05 14:39:05 +0000 |
19 | @@ -28,9 +28,6 @@ |
20 | (c++)"Ubuntu::DownloadManager::DownloadsList::qt_metacall(QMetaObject::Call, int, void**)@Base" 0.4+14.10.20140618 |
21 | (c++)"Ubuntu::DownloadManager::DownloadsList::qt_metacast(char const*)@Base" 0.4+14.10.20140618 |
22 | (c++)"Ubuntu::DownloadManager::DownloadsList::staticMetaObject@Base" 0.4+14.10.20140618 |
23 | - (c++)"Ubuntu::DownloadManager::DownloadsList::~DownloadsList()@Base" 0.4+14.10.20140618 |
24 | - (c++)"Ubuntu::DownloadManager::DownloadsList::~DownloadsList()@Base" 0.4+14.10.20140618 |
25 | - (c++)"Ubuntu::DownloadManager::DownloadsList::~DownloadsList()@Base" 0.4+14.10.20140618 |
26 | (c++)"Ubuntu::DownloadManager::GroupDownload::qt_metacall(QMetaObject::Call, int, void**)@Base" 0.4+14.10.20140618 |
27 | (c++)"Ubuntu::DownloadManager::GroupDownload::qt_metacast(char const*)@Base" 0.4+14.10.20140618 |
28 | (c++)"Ubuntu::DownloadManager::GroupDownload::staticMetaObject@Base" 0.4+14.10.20140618 |
29 | @@ -62,9 +59,6 @@ |
30 | (c++)"Ubuntu::DownloadManager::Manager::createSystemManager(QString const&, QObject*)@Base" 0.4+14.10.20140618 |
31 | (c++)"Ubuntu::DownloadManager::Manager::createSessionManager(QString const&, QObject*)@Base" 0.4+14.10.20140618 |
32 | (c++)"Ubuntu::DownloadManager::Manager::downloadsWithMetadataFound(QString const&, QString const&, Ubuntu::DownloadManager::DownloadsList*)@Base" 0.4+14.10.20140618 |
33 | - (c++)"Ubuntu::DownloadManager::Manager::~Manager()@Base" 0.4+14.10.20140618 |
34 | - (c++)"Ubuntu::DownloadManager::Manager::~Manager()@Base" 0.4+14.10.20140618 |
35 | - (c++)"Ubuntu::DownloadManager::Manager::~Manager()@Base" 0.4+14.10.20140618 |
36 | (c++)"Ubuntu::DownloadManager::Download::processing(QString const&)@Base" 0.4+14.10.20140618 |
37 | (c++)"Ubuntu::DownloadManager::Download::qt_metacall(QMetaObject::Call, int, void**)@Base" 0.4+14.10.20140618 |
38 | (c++)"Ubuntu::DownloadManager::Download::qt_metacast(char const*)@Base" 0.4+14.10.20140618 |
39 | @@ -76,9 +70,6 @@ |
40 | (c++)"Ubuntu::DownloadManager::Download::canceled(bool)@Base" 0.4+14.10.20140618 |
41 | (c++)"Ubuntu::DownloadManager::Download::finished(QString const&)@Base" 0.4+14.10.20140618 |
42 | (c++)"Ubuntu::DownloadManager::Download::progress(unsigned long long, unsigned long long)@Base" 0.4+14.10.20140618 |
43 | - (c++)"Ubuntu::DownloadManager::Download::~Download()@Base" 0.4+14.10.20140618 |
44 | - (c++)"Ubuntu::DownloadManager::Download::~Download()@Base" 0.4+14.10.20140618 |
45 | - (c++)"Ubuntu::DownloadManager::Download::~Download()@Base" 0.4+14.10.20140618 |
46 | (c++)"Ubuntu::DownloadManager::AuthError::errorString()@Base" 0.4+14.10.20140618 |
47 | (c++)"Ubuntu::DownloadManager::AuthError::qt_metacall(QMetaObject::Call, int, void**)@Base" 0.4+14.10.20140618 |
48 | (c++)"Ubuntu::DownloadManager::AuthError::qt_metacast(char const*)@Base" 0.4+14.10.20140618 |
49 | |
50 | === modified file 'src/common/priv/ubuntu/transfers/system/network_reply.cpp' |
51 | --- src/common/priv/ubuntu/transfers/system/network_reply.cpp 2014-04-08 20:22:22 +0000 |
52 | +++ src/common/priv/ubuntu/transfers/system/network_reply.cpp 2014-08-05 14:39:05 +0000 |
53 | @@ -119,6 +119,16 @@ |
54 | return _reply->errorString(); |
55 | } |
56 | |
57 | +bool |
58 | +NetworkReply::hasRawHeader(const QByteArray& headerName) const { |
59 | + return _reply->hasRawHeader(headerName); |
60 | +} |
61 | + |
62 | +QByteArray |
63 | +NetworkReply::rawHeader(const QByteArray& headerName) const { |
64 | + return _reply->rawHeader(headerName); |
65 | +} |
66 | + |
67 | } // System |
68 | |
69 | } // Transfers |
70 | |
71 | === modified file 'src/common/priv/ubuntu/transfers/system/network_reply.h' |
72 | --- src/common/priv/ubuntu/transfers/system/network_reply.h 2014-02-28 15:56:30 +0000 |
73 | +++ src/common/priv/ubuntu/transfers/system/network_reply.h 2014-08-05 14:39:05 +0000 |
74 | @@ -45,6 +45,8 @@ |
75 | virtual bool canIgnoreSslErrors(const QList<QSslError>& errors); |
76 | virtual QVariant attribute(QNetworkRequest::Attribute code) const; |
77 | virtual QString errorString() const; |
78 | + virtual bool hasRawHeader(const QByteArray& headerName) const; |
79 | + virtual QByteArray rawHeader(const QByteArray& headerName) const; |
80 | |
81 | signals: |
82 | // signals forwarded from the real reply object |
83 | |
84 | === modified file 'src/downloads/priv/ubuntu/downloads/file_download.cpp' |
85 | --- src/downloads/priv/ubuntu/downloads/file_download.cpp 2014-07-21 12:38:37 +0000 |
86 | +++ src/downloads/priv/ubuntu/downloads/file_download.cpp 2014-08-05 14:39:05 +0000 |
87 | @@ -142,6 +142,7 @@ |
88 | const QString AUTH_ERROR = "AUTHENTICATION ERROR"; |
89 | const QString PROXY_AUTH_ERROR = "PROXY_AUTHENTICATION ERROR"; |
90 | const QString UNEXPECTED_ERROR = "UNEXPECTED_ERROR"; |
91 | + const QByteArray CONTENT_DISPOSITION = "Content-Disposition"; |
92 | } |
93 | |
94 | namespace Ubuntu { |
95 | @@ -527,6 +528,34 @@ |
96 | FileDownload::onDownloadCompleted() { |
97 | TRACE << _url; |
98 | |
99 | + // check if we have the content-type header, if we do we are going to change the |
100 | + // file path that will be used by the download, do not do it if the app is |
101 | + // unconfined |
102 | + if ((_reply->hasRawHeader(CONTENT_DISPOSITION) && ( |
103 | + isConfined() || !metadata().contains(Metadata::LOCAL_PATH_KEY)))) { |
104 | + QString contentDisposition = _reply->rawHeader(CONTENT_DISPOSITION); |
105 | + DOWN_LOG(INFO) << "Content-Disposition header" << contentDisposition; |
106 | + |
107 | + if (contentDisposition.contains("filename")) { |
108 | + auto serverName = filenameFromHTTPContentDisposition(contentDisposition); |
109 | + DOWN_LOG(INFO) << "Server name " << serverName; |
110 | + |
111 | + if (!serverName.isEmpty()) { |
112 | + QFileInfo fiContentDisposition(serverName); |
113 | + auto filename = fiContentDisposition.fileName(); |
114 | + // replace the filename of the current _filePath with the new one |
115 | + QFileInfo fiFilePath(_filePath); |
116 | + auto currentFileName = fiFilePath.fileName(); |
117 | + auto newPath = _filePath.replace(currentFileName, filename); |
118 | + |
119 | + // unlock the old path and lock the new one |
120 | + _fileNameMutex->unlockFileName(_filePath); |
121 | + _filePath = _fileNameMutex->lockFileName(newPath); |
122 | + DOWN_LOG(INFO) << "Content disposition based file path is '" << serverName << "'"; |
123 | + } |
124 | + } |
125 | + } |
126 | + |
127 | // if the hash is present we check it |
128 | if (!_hash.isEmpty()) { |
129 | emit processing(filePath()); |
130 | @@ -840,6 +869,29 @@ |
131 | } |
132 | } |
133 | |
134 | +QString |
135 | +FileDownload::filenameFromHTTPContentDisposition(const QString& value) { |
136 | + auto keyValuePairs = value.split(';'); |
137 | + |
138 | + foreach(const QString& valuePair, keyValuePairs) { |
139 | + int valueStartPos = valuePair.indexOf('='); |
140 | + |
141 | + if (valueStartPos < 0) |
142 | + continue; |
143 | + |
144 | + auto pair = valuePair.split('='); |
145 | + if (pair.size() != 2 || pair[0].isEmpty() || pair[0].simplified() != "filename") |
146 | + continue; |
147 | + |
148 | + auto value = pair[1].replace("\"", "") // remove "" |
149 | + .replace("'", "") // remove ' |
150 | + .simplified(); // remove white spaces |
151 | + return value; |
152 | + } |
153 | + |
154 | + return QString(); |
155 | +} |
156 | + |
157 | void |
158 | FileDownload::cleanUpCurrentData() { |
159 | bool success = true; |
160 | |
161 | === modified file 'src/downloads/priv/ubuntu/downloads/file_download.h' |
162 | --- src/downloads/priv/ubuntu/downloads/file_download.h 2014-06-18 11:10:18 +0000 |
163 | +++ src/downloads/priv/ubuntu/downloads/file_download.h 2014-08-05 14:39:05 +0000 |
164 | @@ -131,6 +131,7 @@ |
165 | void initFileNames(); |
166 | void emitFinished(); |
167 | void unlockFilePath(); |
168 | + QString filenameFromHTTPContentDisposition(const QString& value); |
169 | |
170 | private: |
171 | bool _downloading = false; |
172 | |
173 | === modified file 'src/downloads/priv/ubuntu/downloads/manager.h' |
174 | --- src/downloads/priv/ubuntu/downloads/manager.h 2014-07-21 09:02:32 +0000 |
175 | +++ src/downloads/priv/ubuntu/downloads/manager.h 2014-08-05 14:39:05 +0000 |
176 | @@ -26,6 +26,7 @@ |
177 | #include <ubuntu/transfers/queue.h> |
178 | #include <ubuntu/transfers/system/dbus_connection.h> |
179 | #include <ubuntu/download_manager/metatypes.h> |
180 | +#include <functional> |
181 | #include "ubuntu/transfers/base_manager.h" |
182 | #include "ubuntu/transfers/system/application.h" |
183 | #include "download.h" |
184 | |
185 | === modified file 'src/uploads/priv/ubuntu/uploads/manager.h' |
186 | --- src/uploads/priv/ubuntu/uploads/manager.h 2014-07-21 09:02:32 +0000 |
187 | +++ src/uploads/priv/ubuntu/uploads/manager.h 2014-08-05 14:39:05 +0000 |
188 | @@ -28,6 +28,7 @@ |
189 | #include <ubuntu/transfers/system/dbus_connection.h> |
190 | #include <ubuntu/transfers/queue.h> |
191 | #include <ubuntu/transfers/base_manager.h> |
192 | +#include <functional> |
193 | #include "factory.h" |
194 | |
195 | namespace Ubuntu { |
196 | |
197 | === modified file 'tests/common/network_reply.h' |
198 | --- tests/common/network_reply.h 2014-07-16 12:28:32 +0000 |
199 | +++ tests/common/network_reply.h 2014-08-05 14:39:05 +0000 |
200 | @@ -44,6 +44,8 @@ |
201 | MOCK_CONST_METHOD1(attribute, |
202 | QVariant(QNetworkRequest::Attribute code)); |
203 | MOCK_CONST_METHOD0(errorString, QString()); |
204 | + MOCK_CONST_METHOD1(hasRawHeader, bool(const QByteArray&)); |
205 | + MOCK_CONST_METHOD1(rawHeader, QByteArray(const QByteArray&)); |
206 | |
207 | using NetworkReply::downloadProgress; |
208 | using NetworkReply::uploadProgress; |
209 | |
210 | === modified file 'tests/downloads/daemon/test_download.cpp' |
211 | --- tests/downloads/daemon/test_download.cpp 2014-07-16 11:46:55 +0000 |
212 | +++ tests/downloads/daemon/test_download.cpp 2014-08-05 14:39:05 +0000 |
213 | @@ -1244,6 +1244,10 @@ |
214 | .Times(1) |
215 | .WillOnce(Return(QVariant(200))); |
216 | |
217 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
218 | + .Times(1) |
219 | + .WillOnce(Return(false)); |
220 | + |
221 | // file system expectations |
222 | EXPECT_CALL(*_fileManager, createFile(_)) |
223 | .Times(1) |
224 | @@ -1309,6 +1313,10 @@ |
225 | .Times(1) |
226 | .WillOnce(Return(QVariant(200))); |
227 | |
228 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
229 | + .Times(1) |
230 | + .WillOnce(Return(false)); |
231 | + |
232 | // file system expectations |
233 | EXPECT_CALL(*_fileManager, createFile(_)) |
234 | .Times(1) |
235 | @@ -1395,6 +1403,10 @@ |
236 | .Times(1) |
237 | .WillOnce(Return(QVariant(200))); |
238 | |
239 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
240 | + .Times(1) |
241 | + .WillOnce(Return(false)); |
242 | + |
243 | // file system expectations |
244 | EXPECT_CALL(*_fileManager, createFile(_)) |
245 | .Times(1) |
246 | @@ -2113,6 +2125,10 @@ |
247 | .Times(1) |
248 | .WillOnce(Return(QVariant(200))); |
249 | |
250 | + EXPECT_CALL(*reply, hasRawHeader(_)) |
251 | + .Times(1) |
252 | + .WillOnce(Return(false)); |
253 | + |
254 | // file system expectations |
255 | EXPECT_CALL(*_fileManager, createFile(_)) |
256 | .Times(1) |
257 | @@ -2220,6 +2236,10 @@ |
258 | .Times(1) |
259 | .WillOnce(Return(QVariant(200))); |
260 | |
261 | + EXPECT_CALL(*reply, hasRawHeader(_)) |
262 | + .Times(1) |
263 | + .WillOnce(Return(false)); |
264 | + |
265 | // file system expectations |
266 | EXPECT_CALL(*_fileManager, createFile(_)) |
267 | .Times(1) |
268 | @@ -2327,6 +2347,10 @@ |
269 | .Times(1) |
270 | .WillOnce(Return(QVariant(200))); |
271 | |
272 | + EXPECT_CALL(*reply, hasRawHeader(_)) |
273 | + .Times(1) |
274 | + .WillOnce(Return(false)); |
275 | + |
276 | // file system expectations |
277 | EXPECT_CALL(*_fileManager, createFile(_)) |
278 | .Times(1) |
279 | @@ -2416,6 +2440,10 @@ |
280 | .Times(1) |
281 | .WillOnce(Return(QVariant(200))); |
282 | |
283 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
284 | + .Times(1) |
285 | + .WillOnce(Return(false)); |
286 | + |
287 | // file system expectations |
288 | EXPECT_CALL(*_fileManager, createFile(_)) |
289 | .Times(1) |
290 | @@ -2526,6 +2554,10 @@ |
291 | .Times(1) |
292 | .WillOnce(Return(QVariant(200))); |
293 | |
294 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
295 | + .Times(1) |
296 | + .WillOnce(Return(false)); |
297 | + |
298 | // file system expectations |
299 | EXPECT_CALL(*_fileManager, createFile(_)) |
300 | .Times(1) |
301 | @@ -2627,6 +2659,10 @@ |
302 | .Times(1) |
303 | .WillOnce(Return(QVariant(200))); |
304 | |
305 | + EXPECT_CALL(*reply.data(), hasRawHeader(_)) |
306 | + .Times(1) |
307 | + .WillOnce(Return(false)); |
308 | + |
309 | // file system expectations |
310 | EXPECT_CALL(*_fileManager, createFile(_)) |
311 | .Times(1) |
312 | @@ -3115,6 +3151,10 @@ |
313 | .Times(1) |
314 | .WillOnce(Return(QVariant(200))); |
315 | |
316 | + EXPECT_CALL(*reply, hasRawHeader(_)) |
317 | + .Times(1) |
318 | + .WillOnce(Return(false)); |
319 | + |
320 | // file system expectations |
321 | EXPECT_CALL(*_fileManager, createFile(_)) |
322 | .Times(1) |
323 | @@ -3450,6 +3490,10 @@ |
324 | .Times(1) |
325 | .WillOnce(Return(QVariant())); |
326 | |
327 | + EXPECT_CALL(*secondReply.data(), hasRawHeader(_)) |
328 | + .Times(1) |
329 | + .WillOnce(Return(false)); |
330 | + |
331 | // file system expectations |
332 | EXPECT_CALL(*_fileManager, createFile(_)) |
333 | .Times(2) |
334 | @@ -3605,6 +3649,10 @@ |
335 | .Times(1) |
336 | .WillOnce(Return(QVariant(200))); |
337 | |
338 | + EXPECT_CALL(*reply, hasRawHeader(_)) |
339 | + .Times(1) |
340 | + .WillOnce(Return(false)); |
341 | + |
342 | // file system expectations |
343 | EXPECT_CALL(*_fileManager, createFile(_)) |
344 | .Times(1) |
345 | @@ -3783,4 +3831,5 @@ |
346 | verifyMocks(); |
347 | } |
348 | |
349 | + |
350 | QTEST_MAIN(TestDownload) |
FAILED: Continuous integration, rev:317 jenkins. qa.ubuntu. com/job/ ubuntu- download- manager- ci/708/ jenkins. qa.ubuntu. com/job/ ubuntu- download- manager- utopic- amd64-ci/ 67/console jenkins. qa.ubuntu. com/job/ ubuntu- download- manager- utopic- armhf-ci/ 67/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/ubuntu- download- manager- ci/708/ rebuild
http://