Bug 51090 – WebKit2: WebPageWin needs implementations of hasLocalDataForURL and canHandleRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brian Weinstein]

Mac has these implementations, Windows needs them as well.

[Brian Weinstein]

<rdar://problem/8770831>

[Brian Weinstein]

Created attachment 76622 [details]
[PATCH] Fix

I'm not sure why the #include of ArchiveResource is needed, but it doesn't compile without it.

[Adam Roben (:aroben)]

(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?

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

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.

[Brian Weinstein]

(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

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

Created attachment 76666 [details]
[PATCH] Take 2

[Adam Roben (:aroben)]

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.

[Brian Weinstein]

Landed in r74131.