[Webkit-unassigned] [Bug 130360] [iOS] WebKit2 Quicklook.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130360


Sam Weinig <sam at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #226976|review?                     |review-
               Flag|                            |

Yongjun Zhang <yongjun_zhang at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #227157|                            |review?
               Flag|                            |
 Attachment #227157|0                           |1
        is obsolete|                            |
 Attachment #227157|review?                     |
               Flag|                            |
 Attachment #226976|0                           |1
        is obsolete|                            |
 Attachment #227162|                            |review?
               Flag|                            |




--- Comment #5 from Sam Weinig <sam at webkit.org>  2014-03-18 19:26:29 PST ---
(From update of attachment 226976)
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 from Yongjun Zhang <yongjun_zhang at apple.com>  2014-03-18 23:20:38 PST ---
(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 from Yongjun Zhang <yongjun_zhang at apple.com>  2014-03-18 23:39:54 PST ---
Created an attachment (id=227157)
 --> (https://bugs.webkit.org/attachment.cgi?id=227157&action=review)
Patch; add QuickLook stuff into WebResourceLoader instead of sub-classing.

--- Comment #8 from WebKit Commit Bot <commit-queue at webkit.org>  2014-03-18 23:40:59 PST ---
Attachment 227157 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 from Yongjun Zhang <yongjun_zhang at apple.com>  2014-03-18 23:46:08 PST ---
Created an attachment (id=227162)
 --> (https://bugs.webkit.org/attachment.cgi?id=227162&action=review)
Fix style issues.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.