Bug 150898 - [WK2] Extract networking code out of NetworkResourceLoader class to improve reusability
Summary: [WK2] Extract networking code out of NetworkResourceLoader class to improve r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 150937
Blocks: 150856
  Show dependency treegraph
 
Reported: 2015-11-04 10:40 PST by Chris Dumez
Modified: 2015-11-05 10:56 PST (History)
5 users (show)

See Also:


Attachments
WIP Patch (73.15 KB, patch)
2015-11-04 10:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (72.42 KB, patch)
2015-11-04 11:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (72.80 KB, patch)
2015-11-04 11:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (75.13 KB, patch)
2015-11-04 11:24 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (74.92 KB, patch)
2015-11-04 13:50 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2015-11-04 16:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-11-04 10:40:46 PST
Extract networking code out of NetworkResourceLoader class to improve reusability (will be used for speculative revalidation) and simplify the NetworkResourceLoader.
Comment 1 Chris Dumez 2015-11-04 10:59:53 PST
Created attachment 264799 [details]
WIP Patch
Comment 2 Chris Dumez 2015-11-04 11:01:54 PST
Created attachment 264800 [details]
WIP Patch
Comment 3 Chris Dumez 2015-11-04 11:08:43 PST
Created attachment 264801 [details]
WIP Patch
Comment 4 Chris Dumez 2015-11-04 11:24:20 PST
Created attachment 264802 [details]
Patch
Comment 5 Alex Christensen 2015-11-04 12:33:34 PST
Comment on attachment 264802 [details]
Patch

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

r=me with a few comments:

> Source/WebKit2/ChangeLog:9
> +        reusability (will be used for speculative revalidation) and simplify

The speculative revalidator will be a NetworkLoadClient, right?

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:209
> -    loader->handle()->continueDidReceiveResponse();
> +    loader->networkLoad()->handle()->continueDidReceiveResponse();

I guess the assumption that the loader has a networkLoad and the networkLoad has a handle is just as good as the old assumption that the we have a loader with a handle.

> Source/WebKit2/NetworkProcess/NetworkLoadParameters.cpp:44
> +NetworkLoadParameters::NetworkLoadParameters()
> +    : webPageID(0)
> +    , webFrameID(0)
> +    , sessionID(SessionID::emptySessionID())
> +    , contentSniffingPolicy(SniffContent)
> +    , allowStoredCredentials(DoNotAllowStoredCredentials)
> +    , clientCredentialPolicy(DoNotAskClientForAnyCredentials)
> +    , shouldClearReferrerOnHTTPSToHTTPRedirect(true)
> +    , defersLoading(false)
> +    , needsCertificateInfo(false)

If this constructor is needed, you should have webPageID { 0 }; etc. in the header instead of here.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)

This should be an actual constructor.
Comment 6 Chris Dumez 2015-11-04 13:02:43 PST
Comment on attachment 264802 [details]
Patch

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

>> Source/WebKit2/ChangeLog:9
>> +        reusability (will be used for speculative revalidation) and simplify
> 
> The speculative revalidator will be a NetworkLoadClient, right?

Yes.

>> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:209
>> +    loader->networkLoad()->handle()->continueDidReceiveResponse();
> 
> I guess the assumption that the loader has a networkLoad and the networkLoad has a handle is just as good as the old assumption that the we have a loader with a handle.

That's right. handle() could probably return a reference now though. I'll see if I can make this change.

>> Source/WebKit2/NetworkProcess/NetworkLoadParameters.cpp:44
>> +    , needsCertificateInfo(false)
> 
> If this constructor is needed, you should have webPageID { 0 }; etc. in the header instead of here.

Right, I'll move the initializations inline.

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
>> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)
> 
> This should be an actual constructor.

You mean have a NetworkLoadParameters constructor that takes a NetworkResourceLoadParameters in argument?
Comment 7 Alex Christensen 2015-11-04 13:03:32 PST
(In reply to comment #6)
> >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
> >> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)
> > 
> > This should be an actual constructor.
> 
> You mean have a NetworkLoadParameters constructor that takes a
> NetworkResourceLoadParameters in argument?
Yes.
Comment 8 Chris Dumez 2015-11-04 13:50:41 PST
Created attachment 264808 [details]
Patch
Comment 9 WebKit Commit Bot 2015-11-04 14:40:09 PST
Comment on attachment 264808 [details]
Patch

Clearing flags on attachment: 264808

Committed r192038: <http://trac.webkit.org/changeset/192038>
Comment 10 WebKit Commit Bot 2015-11-04 14:40:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Dumez 2015-11-04 16:28:27 PST
(In reply to comment #11)
> This change seems to have caused some new crashes:
> 
> Crashlog
> <https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> r192038%20(8097)/fast/inspector-support/matchedrules-crash-log.txt>
> 
> Run:
> <https://build.webkit.org/builders/
> Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/8097>

It looks like the SandBoxExtension has a non-zero useCount when it is destroyed and therefore hits an assertion. I am trying to figure out how this is possible. NetworkResourceLoader::cleanup() is called and it should revoke the sandbox extension.
Comment 13 Chris Dumez 2015-11-04 16:41:21 PST
Created attachment 264826 [details]
Patch
Comment 14 Chris Dumez 2015-11-04 16:42:36 PST
Comment on attachment 264808 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-211
> -        return;

We used to return early after calling setDefersLoading() on the handle here and I stopped doing this by mistake after my change.
Comment 15 Chris Dumez 2015-11-04 16:55:18 PST
Comment on attachment 264826 [details]
Patch

Clearing flags on attachment: 264826

Committed r192044: <http://trac.webkit.org/changeset/192044>
Comment 16 Chris Dumez 2015-11-04 16:55:23 PST
All reviewed patches have been landed.  Closing bug.