Bug 130360 - [iOS] WebKit2 Quicklook.
Summary: [iOS] WebKit2 Quicklook.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-17 14:11 PDT by Yongjun Zhang
Modified: 2014-03-19 12:25 PDT (History)
8 users (show)

See Also:


Attachments
Patch. (24.55 KB, patch)
2014-03-17 15:39 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj. (24.69 KB, patch)
2014-03-17 16:09 PDT, Yongjun Zhang
sam: review-
Details | Formatted Diff | Diff
Patch; add QuickLook stuff into WebResourceLoader instead of sub-classing. (18.78 KB, patch)
2014-03-18 23:39 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Fix style issues. (18.78 KB, patch)
2014-03-18 23:46 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Build fix for gtk port. (18.77 KB, patch)
2014-03-19 00:25 PDT, Yongjun Zhang
thorton: review+
Details | Formatted Diff | Diff
Address review comments before landing. (18.72 KB, patch)
2014-03-19 11:45 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2014-03-17 14:11:01 PDT
Quicklook.
Comment 1 Yongjun Zhang 2014-03-17 15:38:43 PDT
<rdar://problem/15260214>
Comment 2 Yongjun Zhang 2014-03-17 15:39:10 PDT
Created attachment 226972 [details]
Patch.
Comment 3 Yongjun Zhang 2014-03-17 16:09:32 PDT
Created attachment 226976 [details]
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj.
Comment 4 Tim Horton 2014-03-18 13:53:41 PDT
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'
Comment 5 Sam Weinig 2014-03-18 19:26:29 PDT
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.
Comment 6 Yongjun Zhang 2014-03-18 23:20:38 PDT
(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!
Comment 7 Yongjun Zhang 2014-03-18 23:39:54 PDT
Created attachment 227157 [details]
Patch; add QuickLook stuff into WebResourceLoader instead of sub-classing.
Comment 8 WebKit Commit Bot 2014-03-18 23:40:59 PDT
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.
Comment 9 Yongjun Zhang 2014-03-18 23:46:08 PDT
Created attachment 227162 [details]
Fix style issues.
Comment 10 Yongjun Zhang 2014-03-19 00:25:48 PDT
Created attachment 227165 [details]
Build fix for gtk port.
Comment 11 Tim Horton 2014-03-19 11:03:24 PDT
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?
Comment 12 Yongjun Zhang 2014-03-19 11:45:44 PDT
Created attachment 227205 [details]
Address review comments before landing.
Comment 13 WebKit Commit Bot 2014-03-19 12:24:58 PDT
Comment on attachment 227205 [details]
Address review comments before landing.

Clearing flags on attachment: 227205

Committed r165908: <http://trac.webkit.org/changeset/165908>
Comment 14 WebKit Commit Bot 2014-03-19 12:25:03 PDT
All reviewed patches have been landed.  Closing bug.