Merge lp:~mandel/ubuntu-download-manager/content-disposition into lp:ubuntu-download-manager

Proposed by Manuel de la Peña
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
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.

To post a comment you must log in.
317. By Manuel de la Peña

Fix small issue in the parser.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
318. By Manuel de la Peña

Update the change log.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
319. By Manuel de la Peña

Extra logging.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
320. By Manuel de la Peña

Removed destructors from symbols.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
321. By Manuel de la Peña

Ensure thar we remove spaces from the filename key.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
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://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-utopic-armhf-ci/71/artifact/work/output/*zip*/output.zip
1 - https://bugs.launchpad.net/ubuntu-download-manager/+bug/1205355/comments/1

review: Approve
322. By Manuel de la Peña

Merged with trunk.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michael Sheldon (michael-sheldon) wrote :

Looks good to me!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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)

Subscribers

People subscribed via source and target branches