Summary: | [iOS] WebKit2 Quicklook. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yongjun Zhang <yongjun_zhang> | ||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, ap, commit-queue, ddkilzer, japhet, mitz, sam, thorton | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Yongjun Zhang
2014-03-17 14:11:01 PDT
Created attachment 226972 [details]
Patch.
Created attachment 226976 [details]
Fixed a conflict in WebKit2.xcodeproj/project.pbxproj.
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 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. (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! Created attachment 227157 [details]
Patch; add QuickLook stuff into WebResourceLoader instead of sub-classing.
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.
Created attachment 227162 [details]
Fix style issues.
Created attachment 227165 [details]
Build fix for gtk port.
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? Created attachment 227205 [details]
Address review comments before landing.
Comment on attachment 227205 [details] Address review comments before landing. Clearing flags on attachment: 227205 Committed r165908: <http://trac.webkit.org/changeset/165908> All reviewed patches have been landed. Closing bug. |