Separated storage client fronts from server actors. There is one root storage actor that has only method listStores, and several child actors for different storage types. The child actors have a lot in common, so their specs are generated by function createStorageSpec. To apply this patch cleanly, you need: - the front/actor decouple patch from bug 1265429 - my patches for bug 1205123 - I've been working on top of them. They are waiting for checkin, so until they are in fx-team or m-c, you need to apply them manually - if you have applied patches from bug 1265722 (the inspector decoupling), you'll have to solve some trivial conflicts in moz.build files. Storage mochitests are green locally, going to do a full try run.

Comment on attachment 8745261 [details] [diff] [review] Decouple fronts from actors in storage inspector Review of attachment 8745261 [details] [diff] [review]: ----------------------------------------------------------------- I have wanted to do this for a long time... it helps us avoid a host of ugly hacks and makes our codebase make more sense. Because Eddy wrote the decoupling patch itself he is the best person to review this but it looks good to me.

I didn't get to this patch today. Tomorrow is a holiday in the Netherlands, so I won't be able to get to this patch until thursday. Sorry about that!

Comment on attachment 8745261 [details] [diff] [review] Decouple fronts from actors in storage inspector Review of attachment 8745261 [details] [diff] [review]: ----------------------------------------------------------------- I can find nothing wrong with this patch. Make sure that all the tests still pass on the try server, and then go ahead and land it. Thank you very much for taking this work off our plate! ::: devtools/server/actors/storage.js @@ -51,5 @@ > /** > - * Gets an accumulated list of all storage actors registered to be used to > - * create a RetVal to define the return type of StorageActor.listStores method. > - */ > -function getRegisteredTypes() { I assume this function is no longer necessary because we now generate the "storelist" type directly from the child specs? @@ -168,2 @@ > */ > -StorageActors.defaults = function(typeName, observationTopic, storeObjectType) { I assume now storeObjectType no longer needs to passed here because the actor can now grab it from its specification instead? @@ -449,5 @@ > actorObject[key] = overrides[key]; > } > > - let actor = protocol.ActorClass(actorObject); > - protocol.FrontClass(actor, { I assume this is now called in a loop in devtools/client/fronts/storage.js?

(In reply to Eddy Bruel [:ejpbruel] from comment #5) > I can find nothing wrong with this patch. Make sure that all the tests still > pass on the try server, and then go ahead and land it. I need your patch from bug 1265429 to land first. I see that it's already r+ and try run is green, so I assume it will be landed within a few hours. My try run has already passed green on Tuesday. > > -function getRegisteredTypes() { > > I assume this function is no longer necessary because we now generate the > "storelist" type directly from the child specs? Yes, this work is now performed by the types.addDictType("storelist", Object.keys(childSpecs).reduce(...)) call in the specs/storage.js file. > > -StorageActors.defaults = function(typeName, observationTopic, storeObjectType) { > > I assume now storeObjectType no longer needs to passed here because the > actor can now grab it from its specification instead? Yes, it's now part of the spec. The storeObjectType value is used to define the return value of the getStoreObjects method - see the createActorSpec function in specs/storage.js. Different actors return different store objects: 'cookiestoreobject', 'idbstoreobject' etc. > > @@ -449,5 @@ > > actorObject[key] = overrides[key]; > > } > > > > - let actor = protocol.ActorClass(actorObject); > > - protocol.FrontClass(actor, { > > I assume this is now called in a loop in devtools/client/fronts/storage.js? Exactly. This is the server script you're looking at: the protocol.ActorClass call is replaced with ActorClassWithSpec, and definition of the frontends has no place on the server. It has moved to the fronts/storage.js file.

Jarda, before you check this in, did you notice the last minute change in the protocol.js patch? We changed the actorSpec function to generateActorSpec. This will likely break your patch, so make sure to update it before landing it.

(In reply to Eddy Bruel [:ejpbruel] from comment #7) > Jarda, before you check this in, did you notice the last minute change in > the protocol.js patch? We changed the actorSpec function to > generateActorSpec. This will likely break your patch, so make sure to update > it before landing it. I didn't. Thanks for noticing me, I'll cancel the checkin and update the patch.

Adapted to the final API - use protocol.generateActorSpec

Fixed eslint errors, new try.