Bug 51090 - WebKit2: WebPageWin needs implementations of hasLocalDataForURL and canHandleRequest
Summary: WebKit2: WebPageWin needs implementations of hasLocalDataForURL and canHandle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-12-14 20:29 PST by Brian Weinstein
Modified: 2010-12-15 11:35 PST (History)
2 users (show)

See Also:


Attachments
[PATCH] Fix (3.30 KB, patch)
2010-12-14 20:34 PST, Brian Weinstein
aroben: review-
Details | Formatted Diff | Diff
[PATCH] Take 2 (6.94 KB, patch)
2010-12-15 10:43 PST, Brian Weinstein
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-12-14 20:29:14 PST
Mac has these implementations, Windows needs them as well.
Comment 1 Brian Weinstein 2010-12-14 20:29:47 PST
<rdar://problem/8770831>
Comment 2 Brian Weinstein 2010-12-14 20:34:56 PST
Created attachment 76622 [details]
[PATCH] Fix

I'm not sure why the #include of ArchiveResource is needed, but it doesn't compile without it.
Comment 3 Adam Roben (:aroben) 2010-12-15 08:43:21 PST
(In reply to comment #2)
> I'm not sure why the #include of ArchiveResource is needed, but it doesn't compile without it.

What error do you get when you omit it?
Comment 4 Adam Roben (:aroben) 2010-12-15 08:46:13 PST
Comment on attachment 76622 [details]
[PATCH] Fix

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

> WebKit2/ChangeLog:14
> +        (WebKit::WebPage::hasLocalDataForURL): See if the URL is a file URL, then see if it's in the WebCore cache,
> +            and then fall back to the CFURL cache.
> +        (WebKit::WebPage::canHandleRequest): Return the value of CFURLProtocolCanHandleRequest and add a FIXME
> +            for improving this function if needed.

Did this code come from somewhere else? You should mention it if so.

> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:256
> +    if (url.isLocalFile())
> +        return true;
> +
> +    FrameLoader* frameLoader = m_page->mainFrame()->loader();
> +    DocumentLoader* documentLoader = frameLoader ? frameLoader->documentLoader() : 0;
> +    if (documentLoader && documentLoader->subresource(url))
> +        return true;

Seems like this code could all be cross-platform. Why is it sitting in this platform-specific file?

> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:258
> +    RetainPtr<CFMutableURLRequestRef> request(AdoptCF, CFURLRequestCreateMutable(0, url.createCFURL(), kCFURLRequestCachePolicyReloadIgnoringCache, 60, 0));

Looks like you're leaking the CFURL here.

> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:259
> +    CFURLRequestSetHTTPHeaderFieldValue(request.get(), CFSTR("User-Agent"), userAgent().createCFString());

Looks like you're leaking the CFString here.
Comment 5 Brian Weinstein 2010-12-15 09:48:05 PST
Comment on attachment 76622 [details]
[PATCH] Fix

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

>> WebKit2/ChangeLog:14
>> +            for improving this function if needed.
> 
> Did this code come from somewhere else? You should mention it if so.

The line of code from canHandleRequest was from WebView::canHandleRequest in old WebKit. The code in hasLocalDataForURL was ported from WebPageMac, but WebPageMac uses NSURL functions, instead of CFURL.

>> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:256
>> +        return true;
> 
> Seems like this code could all be cross-platform. Why is it sitting in this platform-specific file?

I could put this code in a cross-platform hasLocalDataForURL, then call platformHasLocalDataForURL to do the platform-specific checks if you think that is better.

>> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:258
>> +    RetainPtr<CFMutableURLRequestRef> request(AdoptCF, CFURLRequestCreateMutable(0, url.createCFURL(), kCFURLRequestCachePolicyReloadIgnoringCache, 60, 0));
> 
> Looks like you're leaking the CFURL here.

I'll adopt in a previous line.

>> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:259
>> +    CFURLRequestSetHTTPHeaderFieldValue(request.get(), CFSTR("User-Agent"), userAgent().createCFString());
> 
> Looks like you're leaking the CFString here.

Ditto about adopting.
Comment 6 Brian Weinstein 2010-12-15 09:51:48 PST
(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure why the #include of ArchiveResource is needed, but it doesn't compile without it.
> 
> What error do you get when you omit it?

I was getting an error coming from PassRefPtr, about not knowing about a type.

5>WebPageWin.cpp
5>c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(59) : error C2027: use of undefined type 'WebCore::ArchiveResource'
5>        C:\cygwin\home\bweinstein\WebKitBuild\Include\WebCore/DocumentLoader.h(44) : see declaration of 'WebCore::ArchiveResource'
5>        c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(74) : see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
5>        with
5>        [
5>            T=WebCore::ArchiveResource
5>        ]
5>        c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(74) : while compiling class template member function 'WTF::PassRefPtr<T>::~PassRefPtr(void)'
5>        with
5>        [
5>            T=WebCore::ArchiveResource
5>        ]
5>        ..\WebProcess\WebPage\win\WebPageWin.cpp(254) : see reference to class template instantiation 'WTF::PassRefPtr<T>' being compiled
5>        with
5>        [
5>            T=WebCore::ArchiveResource
5>        ]
5>c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(59) : error C2227: left of '->deref' must point to class/struct/union/generic type
Comment 7 Adam Roben (:aroben) 2010-12-15 10:13:04 PST
(In reply to comment #6)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > I'm not sure why the #include of ArchiveResource is needed, but it doesn't compile without it.
> > 
> > What error do you get when you omit it?
> 
> I was getting an error coming from PassRefPtr, about not knowing about a type.
> 
> 5>WebPageWin.cpp
> 5>c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(59) : error C2027: use of undefined type 'WebCore::ArchiveResource'
> 5>        C:\cygwin\home\bweinstein\WebKitBuild\Include\WebCore/DocumentLoader.h(44) : see declaration of 'WebCore::ArchiveResource'
> 5>        c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(74) : see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
> 5>        with
> 5>        [
> 5>            T=WebCore::ArchiveResource
> 5>        ]
> 5>        c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(74) : while compiling class template member function 'WTF::PassRefPtr<T>::~PassRefPtr(void)'
> 5>        with
> 5>        [
> 5>            T=WebCore::ArchiveResource
> 5>        ]
> 5>        ..\WebProcess\WebPage\win\WebPageWin.cpp(254) : see reference to class template instantiation 'WTF::PassRefPtr<T>' being compiled
> 5>        with
> 5>        [
> 5>            T=WebCore::ArchiveResource
> 5>        ]
> 5>c:\cygwin\home\bweinstein\webkitbuild\include\private\javascriptcore\PassRefPtr.h(59) : error C2227: left of '->deref' must point to class/struct/union/generic type

OK, this is not mysterious at all. DocumentLoader::subresource returns a PassRefPtr<ArchiveResource>. That returned PassRefPtr will be destroyed when it goes out of scope, so it will need to call ArchiveResource::deref. Without the definition of ArchiveResource supplied by the .h file, the compiler doesn't know whether that function exists.
Comment 8 Adam Roben (:aroben) 2010-12-15 10:13:30 PST
Comment on attachment 76622 [details]
[PATCH] Fix

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

>>> WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:256
>>> +        return true;
>> 
>> Seems like this code could all be cross-platform. Why is it sitting in this platform-specific file?
> 
> I could put this code in a cross-platform hasLocalDataForURL, then call platformHasLocalDataForURL to do the platform-specific checks if you think that is better.

That sounds good.
Comment 9 Brian Weinstein 2010-12-15 10:43:57 PST
Created attachment 76666 [details]
[PATCH] Take 2
Comment 10 Adam Roben (:aroben) 2010-12-15 11:02:16 PST
Comment on attachment 76666 [details]
[PATCH] Take 2

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

Do you need to update WebPageQt, too?

> WebKit2/ChangeLog:20
> +        (WebKit::WebPage::platformHasLocalDataForURL): Renamed from hasLocalDataForURL and implemented with CFNetwork calls.

If this is the CFNetwork-equivalent of the Mac code, you should say so.
Comment 11 Brian Weinstein 2010-12-15 11:35:08 PST
Landed in r74131.