crossSite navs

Transfer appcache state during cross site navigations too.

The AppCacheHost is an object that persist across the lifetime of a document. It's created prior to the document and prior to the initiation of the resource load for the documents main resource. Today, when a cross-site xfer occurs the host is deleted and a new host is created that knows nothing of whats going on... so not surprisingly things are broken.

Either state of the old host that gets deleted needs to be saved and restored into a new host that gets created... or the old host should not get deleted and instead get transferred along as well.

I chose the latter because there's some state that can't be saved/restored so easily and moving the whole object is less tedious.

BUG=331270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248631

[michaeln, 2014-01-31 23:52:35]

ptal

[Charlie Reis, 2014-02-01 00:15:49]

I know the cross-site transfer logic very well but not the AppCache code.  Can
you help me understand what needs to be updated for AppCache during a transfer?

(I don't expect to fully understand the appcache/ code to review it, just enough
to conceptually know why the update is needed.)

https://codereview.chromium.org/137883005/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/137883005/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:988:
appcache::AppCacheInterceptor::CompleteCrossSiteTransfer(
This line would fit better inside UpdateRequestForTransfer (which has a bunch of
stuff like this).

https://codereview.chromium.org/137883005/diff/20001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_backend_impl.h (right):

https://codereview.chromium.org/137883005/diff/20001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_backend_impl.h:61: scoped_ptr<AppCacheHost>
TransferHostOut(AppCacheHost* host);
I'm not familiar with any of the AppCache code.  What does it mean to transfer
in or out?  Can you explain the need for it a bit more in the comment?

[michaeln, 2014-02-01 00:41:58]

see the updated cl description for your meta question

https://codereview.chromium.org/137883005/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/137883005/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:988:
appcache::AppCacheInterceptor::CompleteCrossSiteTransfer(
On 2014/02/01 00:15:49, Charlie Reis wrote:
> This line would fit better inside UpdateRequestForTransfer (which has a bunch
of
> stuff like this).

Done.

https://codereview.chromium.org/137883005/diff/20001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_backend_impl.h (right):

https://codereview.chromium.org/137883005/diff/20001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_backend_impl.h:61: scoped_ptr<AppCacheHost>
TransferHostOut(AppCacheHost* host);
On 2014/02/01 00:15:49, Charlie Reis wrote:
> I'm not familiar with any of the AppCache code.  What does it mean to transfer
> in or out?  Can you explain the need for it a bit more in the comment?

Done.

[Charlie Reis, 2014-02-01 00:49:38]

Thanks, that helps a lot.  content/ (and the overall concept) LGTM.  I'll defer
to hashimoto@ for webkit/browser/appcache.

[hashimoto, 2014-02-03 10:59:10]

lgtm, thanks!

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
webkit/browser/appcache/appcache_backend_impl.cc:151: AppCacheHost* host) {
nit: A function which takes a raw pointer and returns a scoped_ptr of the same
object looks a bit confusing.
How about changing the argument to host ID?

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
webkit/browser/appcache/appcache_backend_impl.cc:155: return
scoped_ptr<AppCacheHost>();  // shouldn't happen
nit: How about having DLOG or NOTREACHED here to detect when it happens?
The same goes for TransferHostIn().

[michaeln, 2014-02-03 21:22:18]

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_backend_impl.cc (right):

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
webkit/browser/appcache/appcache_backend_impl.cc:151: AppCacheHost* host) {
On 2014/02/03 10:59:11, hashimoto wrote:
> nit: A function which takes a raw pointer and returns a scoped_ptr of the same
> object looks a bit confusing.
> How about changing the argument to host ID?

Done.

https://codereview.chromium.org/137883005/diff/120001/webkit/browser/appcache...
webkit/browser/appcache/appcache_backend_impl.cc:155: return
scoped_ptr<AppCacheHost>();  // shouldn't happen
On 2014/02/03 10:59:11, hashimoto wrote:
> nit: How about having DLOG or NOTREACHED here to detect when it happens?
> The same goes for TransferHostIn().

Done.

[michaeln, 2014-02-03 21:32:47]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-02-03 21:35:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/137883005/230001

[commit-bot: I haz the power, 2014-02-04 00:34:21]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:34:21]

Change committed as 248631

[commit-bot: I haz the power, 2014-02-04 00:34:22]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:34:22]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:34:23]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-04 00:34:27]

CQ bit was unchecked on CL. Ignoring.