Simplify starting a navigation

Simplify starting a navigation

Remove a whole bunch of accumulated technical debt in
DocumentLoader and core/fetch/ related to starting a navigation
and handling redirects.

BUG=

Committed: https://crrev.com/83a52878976eaffc8753993a7689c9c178664fba
Cr-Commit-Position: refs/heads/master@{#358420}

[Nate Chapin, 2015-11-04 22:08:33]

Description was changed from

==========
Simplify starting a navigation

BUG=
==========

to

==========
Simplify starting a navigation

Remove a whole bunch of accumulated technical debt in
DocumentLoader and core/fetch/ related to starting a navigation
and handling redirects.

BUG=
==========

[Nate Chapin, 2015-11-04 22:13:08]

japhet@chromium.org changed reviewers:
+ jochen@chromium.org

[Nate Chapin, 2015-11-04 22:13:10]

jochen, are you up for reviewing this? It's a bit bigger than I intended, but I
think I've explained most of the madness.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:221: // FIXME: It's
unfortunate that the cache layer and below get to know anything about fragment
identifiers.
I remember adding this hack :(

It's relatively straightforward to remove after the repo merge, so doing so.
It's actually only two chromium tests that break, one of which has its
expectations change, and one is fixed with a DocumentLoader.cpp change.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:226: if (request.url() ==
requestCopy.url())
This is a little goofy, and requires passing a non-const reference to
ResourceLoader::create, but I think it's the least wacky way to make sure
everyone gets the final headers for a non-redirect request.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:475:
request.setURL(MemoryCache::removeFragmentIdentifierIfNeeded(request.url()));
Instead of having Resource.cpp mess with the url, just do it here when we're
modifying the ResourceRequest anyway.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (left):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:114:
m_resource->updateRequest(request);
ResourceClients aren't typically attached when init() is called, so this didn't
really do anything.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:277:
m_fetcher->willSendRequest(m_resource->identifier(), newRequest,
redirectResponse, m_options.initiatorInfo);
This requires updating a couple layout test results, but doing the embedder
callback before the Resource callback means the Resource gets all of the
modifications, which matters to DocumentLoader.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (left):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:336: if
(isFormSubmission(m_navigationType) &&
!m_frame->document()->contentSecurityPolicy()->allowFormAction(newRequest.url()))
{
This is the only clause in willSendRequest() that matters for non-redirects, so
move it to FrameLoader::shouldContinueForNavigationPolicy() and don't call
willSendRequest() for non-redirects.

shouldContinueForNavigationPolicy() now has multiple CSP checks in it, so maybe
it should be renamed...

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:725:
m_mainDocumentError = ResourceError();
I have no idea when this line was relevant. Maybe when we supported
multipart/x-mixed-replace main resources?

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:758:
m_applicationCacheHost = ApplicationCacheHost::create(this);
Moved the m_applicationCacheHost->willStartLoadingMainResource(m_request); call
above to FrameFetchContext::willStartLoadingResource(), which makes this
nonsense obsolete.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:102:
timing().markNavigationStart();
As of
https://chromium.googlesource.com/chromium/src/+/b0bae9d815880b722d6fc3833932...,
creating a DocumentLoader means a navigation *will* start, so
markNavigationStart() in the constructor.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:325: // Add the
fragment to the new request url. Note that this is only done on m_request,
Did you know reference fragments are propagated through redirects? I didn't
until debugging why RedirectTest.ServerReference was failing without this block.

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:725: if
(!m_mainResource)
Pre-cached main resources (i.e., data: urls or SubstituteData) can finish
synchronously inside addClient().

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (left):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:108:
documentLoader->detachFromFrame();
I'm not sure exactly which change I made caused this to try to double-detach and
crash, but I'm surprised it wasn't broken before :)

[jochen (gone - plz use gerrit), 2015-11-06 05:28:05]

this code is weird :-/

lgtm anyway

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchContext.h:86: virtual void
willStartLoadingResource(ResourceRequest&, bool isMainResource);
enum would be nicer than bool

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (left):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:108:
documentLoader->detachFromFrame();
On 2015/11/04 at 22:13:09, Nate Chapin wrote:
> I'm not sure exactly which change I made caused this to try to double-detach
and crash, but I'm surprised it wasn't broken before :)

well, you're no longer creating documentLoader just for the test but use the one
from the document.

[Nate Chapin, 2015-11-06 17:42:55]

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1418003006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchContext.h:86: virtual void
willStartLoadingResource(ResourceRequest&, bool isMainResource);
On 2015/11/06 05:28:05, jochen wrote:
> enum would be nicer than bool

Done.

[Nate Chapin, 2015-11-06 17:43:02]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-11-06 17:43:03]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1418003006/#ps100001
(title: "enum-ify")

[commit-bot: I haz the power, 2015-11-06 17:43:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418003006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418003006/100001

[commit-bot: I haz the power, 2015-11-06 20:26:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 20:26:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nate Chapin, 2015-11-06 20:58:26]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-11-06 21:03:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418003006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418003006/100001

[commit-bot: I haz the power, 2015-11-06 22:05:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-06 22:06:39]

Patchset 6 (id:??) landed as
https://crrev.com/83a52878976eaffc8753993a7689c9c178664fba
Cr-Commit-Position: refs/heads/master@{#358420}

[Nate Chapin, 2015-11-16 18:09:20]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1444413002/ by japhet@chromium.org.

The reason for reverting is: This caused several regressions:
https://code.google.com/p/chromium/issues/detail?id=555914
https://code.google.com/p/chromium/issues/detail?id=553078
https://code.google.com/p/chromium/issues/detail?id=553493.