Bug 109562 – [Qt][WK2] Layer QtDownloadManager on the C API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Layer QtDownloadManager on the C API

[Jocelyn Turcotte]

Created attachment 187828 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 187828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187828&action=review

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:65
> +    QtDownloadManager* q = toQtDownloadManager(clientInfo);

manager?

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:72
> +    downloadItem->d->sourceUrl = WKURLCopyQUrl(WKURLResponseCopyURL(response));
> +    downloadItem->d->mimeType = WKStringCopyQString(WKURLResponseCopyMIMEType(response));

I get confused when I see a Copy method and no adopt :-) but I get it, Qt types!

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:101
> +    QtDownloadManager* q = toQtDownloadManager(clientInfo);

s/q/self/ ? that is what we use for EFL

[Jocelyn Turcotte]

(In reply to comment #2)
> (From update of attachment 187828 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187828&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:65
> > +    QtDownloadManager* q = toQtDownloadManager(clientInfo);
> 
> manager?
> 
> > Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:101
> > +    QtDownloadManager* q = toQtDownloadManager(clientInfo);
> 
> s/q/self/ ? that is what we use for EFL

The goal is to mimic the effect of the Q_Q and Q_D macros, q for a main class, d if clientInfo is a private class. This is the convention we agreed on ATM.

[Jocelyn Turcotte]

Created attachment 188523 [details]
Patch

Add some missing adoptWK(<WK*Copy*()>).get().
Also use the new WKURLCopyQString in QtWebError instead of doing WKStringCopyQString(WKURLCopyString(...)).

[Kenneth Rohde Christiansen]

Comment on attachment 188523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188523&action=review

LGTM

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:63
> +void QtDownloadManager::didReceiveResponse(WKContextRef, WKDownloadRef download, WKURLResponseRef response, const void *clientInfo)

* alignment

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:79
> +void QtDownloadManager::didCreateDestination(WKContextRef, WKDownloadRef download, WKStringRef path, const void *clientInfo)

same

[Simon Hausmann]

Comment on attachment 188523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188523&action=review

LGTM, too

> Source/WebKit2/UIProcess/qt/QtWebError.cpp:60
> -    return WKStringCopyQString(WKURLCopyString(WKErrorCopyFailingURL(error.get())));
> +    return WKURLCopyQString(adoptWK(WKErrorCopyFailingURL(error.get())).get());

Nice catch :)

[Benjamin Poulain]

Comment on attachment 188523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188523&action=review

Good for WebKit2.

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:71
> +    downloadItem->d->sourceUrl = WKURLCopyQUrl(adoptWK(WKURLResponseCopyURL(response)).get());

a new adoptToQURL?

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:72
> +    downloadItem->d->mimeType = WKStringCopyQString(adoptWK(WKURLResponseCopyMIMEType(response)).get());

adoptToQString?

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:74
> +    downloadItem->d->suggestedFilename = WKStringCopyQString(adoptWK(WKURLResponseCopySuggestedFilename(response)).get());

adoptToQString?

> Source/WebKit2/UIProcess/qt/QtWebError.cpp:65
> -    return WKStringCopyQString(WKErrorCopyLocalizedDescription(error.get()));
> +    return WKStringCopyQString(adoptWK(WKErrorCopyLocalizedDescription(error.get())).get());

adoptToQString instead of adoptWK().get()?

[Jocelyn Turcotte]

Created attachment 192459 [details]
Patch

Added adoptToQURL(WKURLRef) and adoptToQString(WKURLRef).

[Jocelyn Turcotte]

Committed r145519: <http://trac.webkit.org/changeset/145519>