Summary: | [Qt][WK2] Don't send download requests through PageClient::handleDownloadRequest | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jocelyn Turcotte <jturcotte> | ||||||
Component: | New Bugs | Assignee: | Jocelyn Turcotte <jturcotte> | ||||||
Status: | RESOLVED INVALID | ||||||||
Severity: | Normal | CC: | abecsi, ap, benjamin, cmarcelo, hausmann, jesus, jturcotte, menard, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 112155 | ||||||||
Attachments: |
|
Description
Jocelyn Turcotte
2013-03-12 07:55:22 PDT
Created attachment 192740 [details]
Patch
Comment on attachment 192740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192740&action=review Overall this looks like a nice change! Lots of red and reduced #ifdefs > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:68 > +void QtDownloadManager::didStart(WKContextRef context, WKDownloadRef download, const void *clientInfo) * placement for clientInfo parameter > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:73 > + QString requestURL = WKURLCopyQString(adoptWK(WKURLRequestCopyURL(request.get())).get()); I suppose you could use adoptToQUrl(WKURLRequestCopyURL(request.get()) here? > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:78 > + ASSERT(q->m_downloadSourceView.size() && q->m_downloadSourceView.first().first == requestURL); Perhaps a struct instead of a QPair would increase the readability of the code here? :) (In reply to comment #1) > Created an attachment (id=192740) [details] > Patch LWBTM! (Looks way better to me! tm.) I've never really liked that handleDownloadRequest code path... Created attachment 193881 [details]
Patch
Address Simon's comment.
Comment on attachment 193881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193881&action=review > Source/WebKit2/ChangeLog:22 > + - Send the didStart signal to be catched from the UI process Signal? > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:185 > QScopedPointer<QQuickWebPage> pageView; > QQuickWebView* q_ptr; > QQuickWebViewExperimental* experimental; > - WebKit::QtWebContext* context; > + WebKit::QtWebContext* m_context; This is the only "m_" prefixed attribute. Wouldn't it be better to rename them all at once in a separate patch? I think Alexey would be a better reviewer than me for this. Comment on attachment 193881 [details]
Patch
Qt has been removed, clearing review flags.
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines. |