1110485 - refactor and clean up Service Worker Cache code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

There are a number of things to refactor and clean up in the Service Worker Cache code.  We don't want to keep the code off on maple any longer, though, as it prevents people from being able to test the current functionality.  Therefore the current idea is to land without this clean up work and fix it before the feature can be enabled by default.

The clean up work includes:

- Remove excessive boiler plate and code duplication in the IPC plumbing.
- Rename IPC types to not have a 'P' prefix.  I've learned thats just for protocols.
- Only export header files needed by external directories.
- Consider removing mozilla::dom::cache namespace for externally exposed classes.
- Smaller refactoring TODOs noted in the code.

I would also put any larger style or structural action items from the review here as well.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Consider replacing the RequestId with a separate actor per request.

Comment 2

[Jan Varga [:janv]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> Consider replacing the RequestId with a separate actor per request.

Yeah, this was my review comment in https://bugzilla.mozilla.org/show_bug.cgi?id=940273#c31
The "actor per request" pattern is used all over the code.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

See bug 940273 comment 207 for additional style and refactor suggestions.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm going to build this on top of the patches in bug 1110814.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Refactor Cache IPC requests and types.

Work-in-progress of the refactor.  This compiles, but does not pass tests yet.

Before patch:    740K    dom/cache/
After patch:     716K    dom/cache/

A net reduction of about 24KB of boilerplate code.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Refactor Cache IPC requests to use a separate actor. r=baku

Andrea, this patch is a bit ugly because a lot of code got replaced.  Let me try to highlight the important bits:

1) Look at the .ipdl changes.  The PCacheOp protocol is essentially replaces all of the methods on PCache and PCacheStorage.
2) The type of operation and their results are defined in PCacheTypes.ipdl as unions.
3) CacheOpChild and CacheOpParent are the actor implementations.  These are created for each operation requested by content.
4) The union for the op is pushed all the way into Manager which has just a few entry points now.  ExecuteCacheOp(), ExecuteStorageOp(), and ExecutePutAll().
5) Because of the way everything is sent in either CacheOpArgs or CacheOpResult, the AutoUtils.cpp code basically just has one auto type for each of these unions now.

As I mentioned in a previous comment this patch removes a ton of boilerplate and nets a reduction of about 24KB worth of code.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 interdiff 001 fix compile on clean build

Interdiff for the first patch to fix a few references to a removed header I only noticed after a clobber build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=139336b7cb7f

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8587661 [details] [diff] [review]
P1 Refactor Cache IPC requests to use a separate actor. r=baku

Review of attachment 8587661 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/CacheChild.cpp
@@ +72,5 @@
>    if (!listener) {
>      return;
>    }
>  
> +  // TODO: only destroy if there are no ops or push streams still running

Note, I intend to do this in a later patch in this bug.  I figured I should start splitting things into separate patches as this was getting quite big.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove 'P' prefix from non-protocol IPC types in Cache API. r=baku

This is a mostly mechanical patch to remove the P prefix from the Cache IPC types.  For example, PCacheRequest becomes CacheRequest, etc.  I think I also removed some stale header includes I noticed while doing this.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Modify Fetch IPC HeaderEntry not to use a 'P' prefix in its type name. r=nsm

Another mechanical patch to fix non-protocol IPC type names.  This one switches the fetch PHeadersEntry to just HeadersEntry.  Also InternalHeaders::GetPHeaders() changes to InternalHeaders::GetIPCHeaders().

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8587777 [details] [diff] [review]
P3 Modify Fetch IPC HeaderEntry not to use a 'P' prefix in its type name. r=nsm

Review of attachment 8587777 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalHeaders.h
@@ +91,5 @@
>    static already_AddRefed<InternalHeaders>
>    CORSHeaders(InternalHeaders* aHeaders);
>  
>    void
> +  GetIPCHeaders(nsTArray<HeadersEntry>& aHeadersOut) const;

Or would you prefer that we just move the IPC type into Cache?  No one else is using it right now.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8587777 [details] [diff] [review]
P3 Modify Fetch IPC HeaderEntry not to use a 'P' prefix in its type name. r=nsm

After thinking about it, I just want to move the type.  I'll reflag later today.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Move Fetch IPC PHeaderEntry type to Cache. Rename HeadesEntry. r=nsm

This moves all the IPC gunk out of Fetch and into Cache.  It also does the type rename as per the original patch.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Keep Cache Actors alive during async operations. r=baku

This patch completes the actor lifetime changes I made in P1.  It keeps the actors alive during async operations even when the worker has Notify()'d its WorkerFeatures.  This is something Ben Turner recommended in previous review.

Note, I only hold the worker open for a PCacheStreamControlChild actor if the streams are actively being read.  If the stream has never been read, then we go ahead and close.  This is to handle the case where some gets a Response from the Cache, but only looks at headers and not the body.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8588038 [details] [diff] [review]
P3 Move Fetch IPC PHeaderEntry type to Cache. Rename HeadesEntry. r=nsm

Review of attachment 8588038 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/TypeUtils.cpp
@@ +135,5 @@
> +ToHeadersEntryList(nsTArray<HeadersEntry>& aOut, InternalHeaders* aHeaders)
> +{
> +  MOZ_ASSERT(aHeaders);
> +
> +  nsAutoTArray<InternalHeaders::Entry, 64> entryList;

64 seems really big for headers. How about 16? My completely unscientific glossing of a few entries here https://github.com/http2/http_samples seems to suggest ~10 headers usually.

@@ +423,5 @@
> +
> +  for (uint32_t i = 0; i < aHeadersEntryList.Length(); ++i) {
> +    const HeadersEntry& headersEntry = aHeadersEntryList[i];
> +    entryList.AppendElement(InternalHeaders::Entry(headersEntry.name(),
> +                                                   headersEntry.value()));

Could you use the non-copying form?
InternalHeaders::Entry* entry = entryList.AppendElement();
entry->mName = headersEntry.name();
entry->mValue = headersEntry.value();

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8587661 [details] [diff] [review]
P1 Refactor Cache IPC requests to use a separate actor. r=baku

Review of attachment 8587661 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing it. The code is much more readable and easy to follow now!

::: dom/cache/AutoUtils.cpp
@@ +325,5 @@
> +
> +      break;
> +    }
> +    default:
> +      MOZ_CRASH("Cache args type cannot send a Requesst/Response pair!");

Request

@@ +452,5 @@
> +      break;
> +    }
> +    default:
> +    {
> +      MOZ_CRASH("Cache result type cannot handle returning a Response!");

you don't have {} in the other default: MOZ_CRASH...

@@ +482,5 @@
> +      break;
> +    }
> +    default:
> +    {
> +      MOZ_CRASH("Cache result type cannot handle returning a Request!");

here too.

::: dom/cache/Cache.cpp
@@ +104,2 @@
>    nsRefPtr<InternalRequest> ir = ToInternalRequest(aRequest, IgnoreBody, aRv);
>    if (aRv.Failed()) {

NS_WARN_IF here too?

@@ +113,3 @@
>  
> +  args.Add(ir, IgnoreBody, PassThroughReferrer, IgnoreInvalidScheme, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/cache/CacheOpParent.cpp
@@ +62,5 @@
> +
> +  Execute(manager);
> +}
> +
> +

2 lines. Usually you have 1 single line between methods.

::: dom/cache/CacheStorage.cpp
@@ +52,5 @@
>  NS_INTERFACE_MAP_END
>  
> +// We cannot reference IPC types in a webidl binding implementation header.  So
> +// define this in the .cpp and use heap storage in the mPendingRequests list.
> +struct CacheStorage::Entry

final

@@ +184,5 @@
> +  }
> +
> +  nsRefPtr<InternalRequest> request = ToInternalRequest(aRequest, IgnoreBody,
> +                                                        aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/cache/CacheStorageParent.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/dom/cache/CacheStorageParent.h"
>  
> +#include "mozilla/unused.h"

should this goes after line 14?

::: dom/cache/Manager.h
@@ +119,3 @@
>  
>    protected:
>      ~Listener() { }

virtual ?

::: dom/cache/PCache.ipdl
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  include protocol PBackground;
>  include protocol PCachePushStream;
> +include protocol PCacheOp;

alphabetic order.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #16)
> ::: dom/cache/CacheStorageParent.cpp
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "mozilla/dom/cache/CacheStorageParent.h"
> >  
> > +#include "mozilla/unused.h"
> 
> should this goes after line 14?

I like to sort on dir before file.  So I put all the files directly in mozilla/ first.

> ::: dom/cache/Manager.h
> @@ +119,3 @@
> >  
> >    protected:
> >      ~Listener() { }
> 
> virtual ?

No, because this is never deleted through a base pointer.  This is only declared so that its protected to help ensure that invariant.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 interdiff 002 address review feedback

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #15)
> @@ +423,5 @@
> > +
> > +  for (uint32_t i = 0; i < aHeadersEntryList.Length(); ++i) {
> > +    const HeadersEntry& headersEntry = aHeadersEntryList[i];
> > +    entryList.AppendElement(InternalHeaders::Entry(headersEntry.name(),
> > +                                                   headersEntry.value()));
> 
> Could you use the non-copying form?
> InternalHeaders::Entry* entry = entryList.AppendElement();
> entry->mName = headersEntry.name();
> entry->mValue = headersEntry.value();

It seems nsTArray::AppendElement() takes T&&.  An immediate T() is of type T&&, so the compiler can just optimize the temporary away with a move.

I think you only need the other form when you are appending a long lived object that can't be moved.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Refactor Cache IPC requests to use a separate actor. r=baku

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Move Fetch IPC PHeaderEntry type to Cache. Rename HeadesEntry. r=nsm

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b72f60d8b3a0

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Replace useless DBSchema class type with namespace. r=ehsan

This removes the useless DBSchema type.  I still use a separate "db" namespace, though, to avoid conflicting on symbols like CacheMatch.

Note, I purposely left the methods in the .cpp in the existing order to make the patch easier to read.  I may do a follow-up to reorder them so that the declarations aren't needed in the .cpp, but I don't think that's necessary right now.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Replace useless cache::FileUtils type with cache::file namespace. r=ehsan

Similar patch, but for FileUtils.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Rename DeleteCache() to DeleteCacheId() better distinguish it from CacheDelete(). r=ehsan

Fix an old TODO to clarify the difference between DeleteCache() and CacheDelete() in DBSchema.h.  I chose rename things so that we have:

  CreateCacheId()
  DeleteCacheId()

To distinguish functions that clearly operate on the life cycle of a Cache ID.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Try up to P7:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9689026af2de

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Landed first three patches to avoid bit rot:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/12deb121d126
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fa29dbb49a66
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bd3bf404ff

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fb25d83dcaa0 for wpt-3 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=8790122&repo=mozilla-inbound

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

These WPT tests are new as of this weekend... Also I would note this was a case of a test passing.

I'll look at it tomorrow.

Comment 30

[(no longer active)]

Comment on attachment 8591886 [details] [diff] [review]
P6 Replace useless cache::FileUtils type with cache::file namespace. r=ehsan

Review of attachment 8591886 [details] [diff] [review]:
-----------------------------------------------------------------

Do we really gain anything by adding a new nested namespace for these functions?  As far as I can tell, there are no name clashes if you just put all of these in mozilla::dom::cache, and that way it would be a bit less painful to work with this code (fewer guesses as to which namespace you need to type out in the code/debugger, etc.)  But r=me either way, I can also later change this to remove this namespace if you don't want to do it right now.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3.1 Note WPT cache-storage-keys test passes after refactor fixed op queueing bug. r=jgraham

The refactor fixed a bug in CacheStorageParent::RecvKeys() where it accidentally queued an OP_DELETE instead of OP_KEYS when verification was still happening.

The refactor fixes this.  So the test should expect to pass now.

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8588185 [details] [diff] [review]
P4 Keep Cache Actors alive during async operations. r=baku

Review of attachment 8588185 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

::: dom/cache/ReadStream.cpp
@@ +287,5 @@
>  {
>    // stream ops can happen on any thread
>    MOZ_ASSERT(aNumReadOut);
>  
> +  if (aCount) {

can it really happen that we have aCount == 0 ?

@@ +308,5 @@
>  {
>    // stream ops can happen on any thread
>    MOZ_ASSERT(aNumReadOut);
>  
> +  if (aCount) {

same here.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #32)
> can it really happen that we have aCount == 0 ?

Hmm, I thought it could happen here via NS_InputStreamIsBuffered(), but I guess that passes 1.  I'll re-work that bit:

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#714

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Remove useless cache::FileUtils type. r=ehsan

Updated to remove the file:: namespace.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Keep Cache Actors alive during async operations. r=baku

Updated to check bytes read in ReadSegments() instead of aCount before setting "ever read" flag.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Updated try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce508304f8cd

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40f48cd100a2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/114377b11793
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/565246a02797
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ebaae8b6007f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/31c018015bd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/55723c780549
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/974c5c109a49
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b9517e3d50

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for frequent windows mochitest-1 failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90ec95ca90ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d77e793ba7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9cb307aee4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2899cc3ef94
https://hg.mozilla.org/integration/mozilla-inbound/rev/534c40d78591
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1ca73a3213
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f02912121ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2613f8fed5

https://treeherder.mozilla.org/logviewer.html#?job_id=8846041&repo=mozilla-inbound

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

I think that assert is a race that was revealed by the test.  To avoid continually breaking everyones patches as we back and forth this refactor, I'm landing with the assert disabled.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/19f871364039
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a74f0b2545ab
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbb1ec38652
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/82f7795a806b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f37dc22f4c4f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de417ee861be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fdca92fa4d55
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e8de5745c5c3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf98851529e

Follow-up to fix the assert in bug 1154531.

Comment 40

[Carsten Book [:Tomcat]]

sorry had to back this out for causing frequent windows xp bc1 memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=8868557&repo=mozilla-inbound

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

I'll look, but as far as I know this code doesn't run in bc1 at all.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Add an ErrorResult constructor that takes nsresult. r=bz

I need to rebase on top of bug 1149987 where the IPC nsresult for one operation was converted to an ErrorResult. Since my refactor unifies error reporting I now need to convert all my nsresult values into an ErrorResult.

This patch makes that a bit more convenient by letting me do:

  // in header
  void OnOpComplete(ErrorResult&& aRv);

  // code using OnOpComplete
  OnOpComplete(ErrorResult(NS_ERROR_FAILURE));

I also de-duped some code in ErrorResult.h here.  I believe this should be inlined by the compiler and have no performance impact.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Add an ErrorResult constructor that takes nsresult. r=bz

Bad hg fold in the last patch.  This one has the missing hunks to init values in the new constructor.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Refactor Cache IPC requests to use a separate actor. r=baku

Painful refactor as I needed to propagate the ErrorResult type far enough out for the FetchPut to pass return its TypeError.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove 'P' prefix from non-protocol IPC types in Cache API. r=baku

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Keep Cache Actors alive during async operations. r=baku

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Replace useless DBSchema class type with namespace. r=ehsan

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Correctly set the Feature on the stream control child actor. r=baku

Andrea, this patch is much smaller than I thought when we talked on IRC.  If you could prioritize it, this is blocking bug 1151892 which people keep asking for.

Side note, I originally wanted to make a error type in the union to make it clear you either get an error or a real result.  Unfortunately, the ErrorResult cannot be placed in an IPDL union because it doesn't support copy construction or assignment.  So instead I just enforce that the result is void_t() when an error is passed.

Thanks!

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=010875e3f1bb

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593143 [details] [diff] [review]
P0 Add an ErrorResult constructor that takes nsresult. r=bz

The new ctor does not init mResult before calling AssignErrorCode(), but AssignErrorCode asserts various stuff about mResult not having certain values.  This might happen to work by accident, but it's clearly not good.

I think you either need to set mResult = NS_OK in the new ctor before calling AssignErrorCode, or move mResult = NS_OK into Init(), or go ahead and just have the new ctor delegate to the no-arg one, so you don't need Init at all.  And then you can either do *this = aRv, or keep the AssignErrorCode thing, which I admit does reduce the code duplication between Throw() and operator=.

Also, I would prefer that all the private methods you're adding go above the private members.  I know the IPC friend decl and methods are already misplaced; please move them there too.

r=me with those fixed

Comment 51

[Andrea Marchesini [:baku]]

Comment on attachment 8593165 [details] [diff] [review]
P8 Correctly set the Feature on the stream control child actor. r=baku

Review of attachment 8593165 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

::: dom/cache/CacheOpChild.cpp
@@ +20,5 @@
> +namespace {
> +
> +void
> +AddFeatureToStreamChild(const CacheReadStream& aReadStream, Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);

@@ +30,5 @@
> +}
> +
> +void
> +AddFeatureToStreamChild(const CacheResponse& aResponse,
> +                        Feature* aFeature)

this can stay on the same line. probably..

@@ +31,5 @@
> +
> +void
> +AddFeatureToStreamChild(const CacheResponse& aResponse,
> +                        Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);

@@ +41,5 @@
> +}
> +
> +void
> +AddFeatureToStreamChild(const CacheRequest& aRequest,
> +                        Feature* aFeature)

this can stay on the same line. probably..

@@ +42,5 @@
> +
> +void
> +AddFeatureToStreamChild(const CacheRequest& aRequest,
> +                        Feature* aFeature)
> +{

MOZ_ASSERT(aFeature);

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Add an ErrorResult constructor that takes nsresult. r=bz

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Correctly set the Feature on the stream control child actor. r=baku

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/913ce28d0196
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94bc6eb2b5d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c670dd7488
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b250bae9fb6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35ce6f6ef1f2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb26492727ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9013c5c61f34
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37f7aa609a22
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/12bd6399af4c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/563ec088fb14

Try from comment 49 should still apply.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/913ce28d0196
https://hg.mozilla.org/mozilla-central/rev/94bc6eb2b5d4
https://hg.mozilla.org/mozilla-central/rev/a8c670dd7488
https://hg.mozilla.org/mozilla-central/rev/5b250bae9fb6
https://hg.mozilla.org/mozilla-central/rev/35ce6f6ef1f2
https://hg.mozilla.org/mozilla-central/rev/eb26492727ea
https://hg.mozilla.org/mozilla-central/rev/9013c5c61f34
https://hg.mozilla.org/mozilla-central/rev/37f7aa609a22
https://hg.mozilla.org/mozilla-central/rev/12bd6399af4c
https://hg.mozilla.org/mozilla-central/rev/563ec088fb14