Bug 26655 – ResourceHandle infrastructure is needed to support loading out of an appcache

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Michael Nordman]

This bug was split off of https://bugs.webkit.org/show_bug.cgi?id=25436. Please see the discussion there about the reason for this.

Preliminary patch forth coming.

[Michael Nordman]

Created attachment 31738 [details]
Interceptor (rev1)

Looking for some preliminary feedback on this new piece of webcore resource handle
infrastructure.

These changes allow an 'interceptor' to hijack ResourceHandle requests and to provide response data instead of actually going to the network.

The approach taken in this preliminary patch ripples into the platform
specific resourcehandleXXX.cpp files more than i would like. Its doable, just
very platform specific.

Another approach would be to introduce a new ResourceHandle class into the
hierarchy that 'wraps' the existing class. Something like...
* rename existing ResourceHandle.h cpp --> PlatformResourceHandle.h cpp
* introduce new ResourceHandle.h cpp with same interface as existing class, but
have it use ResourceHandleInterceptorHost  and PlatformResourceHandle for everything under the covers. 

I'm preparing another patch with that hoist usage of the ResourceHandleInterceptorHost into a new wrapper class as described above.

[Michael Nordman]

Created attachment 31813 [details]
Interceptor (rev2)

Uploaded another stab it using the "other approach"... the wrapper class
* renamed existing ResourceHandle.h cpp --> PlatformResourceHandle.h cpp
* introduced new ResourceHandle.h cpp with same interface as existing class, but
have it use the PlatformResourceHandle for everything
* bake ResourceHandleInterceptor usage into the new class

The good thing about this approach is that it only calls for entirely mechanical
changes to the existing platorm specific ResourceHandle impls... simple class
name changes.

Still just looking for preliminary comments right now. An obvious thing thats needed are test cases.

Incidentally, chrome has the moral equivalent, URLRequest::Interceptor, in place for this already... http://codereview.chromium.org/67019... including a decent set of test cases. I'll be looking to implement similar tests for this  ResourceHandle Intercepor stuff.

[Eric Seidel (no email)]

Comment on attachment 31738 [details]
Interceptor (rev1)

It appears that this patch should have been obsoleted.

[Eric Seidel (no email)]

Comment on attachment 31813 [details]
Interceptor (rev2)

This patch needs ChangeLogs to explain what your doing.

http://webkit.org/coding/contributing.html

[Michael Nordman]

I've unobsoleted the initial patch. Patch rev1 and rev2 represent different approaches. I'm looking for preliminary comments including which of these approaches looks more promising.

There's no change log entry yet, just looking for initial comments right now. Please see the discussion in https://bugs.webkit.org/show_bug.cgi?id=25436 for the explanation.

[Michael Nordman]

I've been pushing on the second approach (rev2) since initially uploading since that's the direction that Alexey and I had discussed taking. Here's an update...

* rev2 as uploaded builds and runs in chrome as is (with simple mechanical changes to chrome platform specific files to deal with the rename to PlatformResourceHandle)

* moved this over to a mac webkit client

* did the obvious class name change work in ResourceHandleMac.mm

* did the less obvious class name change work in FormDataStreamMac.h .mm 

* addressed some some gcc vs msvc issues to get things to compile

* fleshed out the PLATFORM(MAC) parts of ResourceHandle.h .cpp to make the linker happy

* build and runs and passes unit tests (modulo few items that i think were also failing prior to patching anything in)

I still don't have good automated tests for this, but manual testing (using the TestInterceptor class in the patch) is looking good. The 'intercept_main', 'intercept_response', 'intercept_redirect', 'intercept_error', and 'restart_wo_interception' cases are working for me.

Cheers

[Michael Nordman]

Created attachment 32128 [details]
Interceptor (rev2)

Here's an updated patch that working for me on Safari/mac.

[Eric Seidel (no email)]

133k is too large for me to review.  But maybe you've been working with a reviewer who can understand this huge patch?

[Michael Nordman]

This is patch is really here more for proof-of-concept purposes, I'm not looking to submit anything like this at this time. Appropriate folks are taking a look.

[David Levin]

Comment on attachment 32128 [details]
Interceptor (rev2)

Clearing r? to move this out of the review queue since Michael Nordman stated "This is patch is really here more for proof-of-concept purposes, I'm not
looking to submit anything like this at this time."

[David Levin]

Comment on attachment 31738 [details]
Interceptor (rev1)

Clearing r? to move this out of the review queue since Michael Nordman stated "This is patch is really here more for proof-of-concept purposes, I'm not
looking to submit anything like this at this time."

[Michael Nordman]

Closing... The webkit team has other plans in mind. We'll have to wait and see what that looks like. In the meantime, the chrome team is going to build its own appcache system as what webcore currently has to offer just plain don't work for us.