Split out shared networking code into net_traits crate

[jdm]

There are parts of the net crate that are independent of the rest of Servo (like the implementation of the loaders, the forthcoming HTTP cache, the forthcoming cookie code, etc.) and it is really frustrating to recompile the script crate every time the code in net is touched. We can do better than this if we split the bits that script and layout need into a net_traits crate that the net crate depends upon.

[jdm]

Look at the existing X_traits crates for how this should look for the networking code.

[jamougha]

I'm working on this.

[jdm]

Great!

[jdm]

Any news, @jamougha?

[jdm]

Looks like this is available for taking once more.

[gilles-leblanc]

I'd like to work on this please.

[jdm]

Please do :)

[gilles-leblanc]

I started working on this and I am making progress. Just a head's up, this one requires more code to be pulled into the new traits crate than the previous ones.

[gilles-leblanc]

You mention the implementation of the loaders as being independent. Actually they are used by resource_task.rs which is in turn exposed as a type of one of the fields of ImageCache and also used in ImageCacheTask, new and new_sync methods which are used outside of net (in gonk and components/servo) which had to be exposed.

I'm currently at the point where I would need to pull the loaders which seem to be simply functions and a lot of the cookie code. At that point I would have pulled more than 85% of net into net_traits and maybe will have to pull more still.

I have a feeling:

• I am not on the right track or
• net doesn't split as easily as the previous _traits

I will push my local repo to my fork so it is referenced here. At the point I am at right now, I would need to move the loaders, cookie code and sniffer_task to make resource_task compile in the net_traits crate.

[jdm]

Thanks for doing that investigation! The way I usually break these tasks down is by starting with the list of dependencies from http://mxr.mozilla.org/servo/search?string=use%20net%3A%3A - it's the dependencies for script/, layout/, and gfx/ that we really want to break. Things like ResourceTask, ControlMsg, ImageCacheTask, and LocalImageCache task will need to be moved; it's possible that all of the image-related code should be moved to net_traits for now, if it's a lot of work to split it up. I believe focusing on just those dependencies should avoid many of the problems that you were encountering in your branch.

[gilles-leblanc]

I started again with the link you gave me starting at the top of the list, taking everything one step at a time, doing one commit per dependency to extract in a new branch.

I'm doing script/, layout/, and gfx/ only for now.

Things are going more smootly now, thanks!

[gilles-leblanc]

@jdm I have completed removing the net/ dependency from gfx/, layout/, and script/.

I have done this in 7 commits (one for each specific dependency). Do you want me to merge squash them in one big commit before I submit a pull request?

Once it's done, I'll rebase and submit a pull request. This is a pretty big one, I will probably have some merging to do again after the review modifications are completed.

[jdm]

I think separate commits sound fine to me!

[jdm]

Well, under the assumption that they all build independently. If that's not the case, squashing makes more sense.

[jdm]

Also, I notice the that implementation of the ImageCache and LocalImageCache has moved into net_traits. Is that necessary? It looks to me like it should be possible to keep those in the net crate, and just move the definition of ImageCacheTask into net_traits.

[gilles-leblanc]

LocalImageCache is used by layout/context.rs, layout/layout_task.rs and layout/fragments so I moved it to traits.

As for ImageCache I will move it back to net as it's only used in net_traits.

I think I will squash the commits, will probably try out both and see with seems the easiest to rebase.

[gilles-leblanc]

I started moving back ImageCache to net/ and saw it needed to be in net_traits because ImageCacheTask's implementation refers to it and that would create a circular reference.

[jdm]

Here's my suggestion to avoid the ImageCacheTask problem - make ImageCacheTask::new/new_sync free functions in net/ (create_image_cache_task), and call that instead. Now ImageCacheTask is no more than a wrapper for a Sender.

[gilles-leblanc]

I've followed your advice for create_image_cache_task, squashed everything together and rebased.

Sadly I still have 2 things to take care off.

./mach test-wpt gives me this:

 0:00.00 LOG: MainThread INFO Closing logging queue
Traceback (most recent call last):
  File "tests/wpt/run.py", line 35, in <module>
    sys.exit(0 if main() else 1)
  File "tests/wpt/run.py", line 32, in main
    return run_tests(**kwargs)
  File "tests/wpt/run.py", line 19, in run_tests
    return wptrunner.run_tests(**kwargs)
  File "/home/bleakcabal/Documents/servo/tests/wpt/_virtualenv/local/lib/python2.7/site-packages/wptrunner/wptrunner.py", line 369, in run_tests
    do_delayed_imports(serve_root)
  File "/home/bleakcabal/Documents/servo/tests/wpt/_virtualenv/local/lib/python2.7/site-packages/wptrunner/wptrunner.py", line 78, in do_delayed_imports
    import serve
  File "/home/bleakcabal/Documents/servo/tests/wpt/web-platform-tests/tools/serve/__init__.py", line 1, in <module>
    import serve
  File "/home/bleakcabal/Documents/servo/tests/wpt/web-platform-tests/tools/serve/serve.py", line 16, in <module>
    from .. import localpaths
ValueError: Attempted relative import beyond toplevel package

and ./mach test-unit fails on tests which load test.jpeg (formerly in net/image/, now in net/) with one of these two messages:

ERROR:net_traits::image::base: stb_image failed: stbi_load_from_memory failed

or

thread 'ImageCacheTask (sync)' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError'

I'm thinking it's probably not finding the file correctly since I moved it.

I'll probably turn to the mailing list for help if I don't find anything on those errors.

[jdm]

Use rm -rf tests/wpt/_virtualenv to get rid of the WPT problem. As for the unit test error, you're probably right - you should be able to adjust http://mxr.mozilla.org/servo/source/components/net/image/base.rs#15 to use a new path as necessary.

[gilles-leblanc]

I corrected the errors and then rebased on latest master and submitted the pull request.