Bug 130360 – [iOS] WebKit2 Quicklook.

• Component: WebKit2
• Status: RESOLVED FIXED

[Yongjun Zhang]

Quicklook.

[Yongjun Zhang]

<rdar://problem/15260214>

[Yongjun Zhang]

Created attachment 226972 [details]
Patch.

[Yongjun Zhang]

Created attachment 226976 [details]
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj.

[Tim Horton]

Comment on attachment 226976 [details]
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj.

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

> Source/WebCore/WebCore.exp.in:2689
> +__ZN7WebCore15QuickLookHandleD1Ev

Did you run sort-export-file?

> Source/WebCore/loader/ResourceLoader.h:39
> +#include "QuickLook.h"

Perhaps we should put this guard around the header instead of around each include?

> Source/WebCore/loader/ResourceLoader.h:162
> +    QuickLookHandle *quickLookHandle() const { return m_quickLookHandle.get(); }

Star on the wrong side.

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:150
> +    m_webResourceLoaders.set(identifier, WebResourceLoaderIOS::create(resourceLoader));

maybe WebResourceLoader::create should make the right thing on each platform?

> Source/WebKit2/WebProcess/ios/WebResourceLoaderIOS.h:35
> +class WebResourceLoaderIOS : public WebResourceLoader {

this class can be 'final'

[Sam Weinig]

Comment on attachment 226976 [details]
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj.

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

>> Source/WebKit2/WebProcess/ios/WebResourceLoaderIOS.h:35
>> +class WebResourceLoaderIOS : public WebResourceLoader {
> 
> this class can be 'final'

I am not sure it is worth subclassing here.  I would just throw the QuickLook stuff right into WebResourceLoader.  I assume you didn't do that because the QuickLook stuff uses Objective-C. To get around that, I would add an C++ wrapper class (like we do for the content filtering stuff with the WebCore::ContentFilter class.

[Yongjun Zhang]

(In reply to comment #5)
> (From update of attachment 226976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226976&action=review
> 
> >> Source/WebKit2/WebProcess/ios/WebResourceLoaderIOS.h:35
> >> +class WebResourceLoaderIOS : public WebResourceLoader {
> > 
> > this class can be 'final'
> 
> I am not sure it is worth subclassing here.  I would just throw the QuickLook stuff right into WebResourceLoader.  I assume you didn't do that because the QuickLook stuff uses Objective-C. To get around that, I would add an C++ wrapper class (like we do for the content filtering stuff with the WebCore::ContentFilter class.

I was trying to avoid sprinkling USE(QUICK_LOOK) in WebResourceLoader.cpp.  I agree there is not real need for subclassing if we just add QL stuff in WebResourceLoader.  We don't even need a new wrapper for ObjC stuff since QuickLookHandle in WebCore already wraps qlConverter.  I will submit a new patch for this.  thanks!

[Yongjun Zhang]

Created attachment 227157 [details]
Patch; add QuickLook stuff into WebResourceLoader instead of sub-classing.

[WebKit Commit Bot]

Attachment 227157 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:139:  Use 0 instead of NULL.  [readability/null] [5]
ERROR: Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:152:  Declaration has space between type name and * in QuickLookHandle *quickLookHandle  [whitespace/declaration] [3]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yongjun Zhang]

Created attachment 227162 [details]
Fix style issues.

[Yongjun Zhang]

Created attachment 227165 [details]
Build fix for gtk port.

[Tim Horton]

Comment on attachment 227165 [details]
Build fix for gtk port.

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

Sam gives his blessing as well, but bugzilla is not cooperating for him.

> Source/WebCore/platform/network/ios/QuickLook.mm:399
> +        return adoptPtr(new QuickLookHandle(NULL, nil, response, delegate));

nullptr for the first arg, which is a C++ type?

> Source/WebKit2/WebProcess/ios/WebResourceLoaderIOS.mm:45
> +- (id)initWithWebResourceLoader:(PassRefPtr<WebKit::WebResourceLoader>)loader

instancetype for the return value?

[Yongjun Zhang]

Created attachment 227205 [details]
Address review comments before landing.

[WebKit Commit Bot]

Comment on attachment 227205 [details]
Address review comments before landing.

Clearing flags on attachment: 227205

Committed r165908: <http://trac.webkit.org/changeset/165908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.