Closed
Bug 1265729
Opened 9 years ago
Closed 9 years ago
Decouple fronts from actors in storage inspector.
Categories
(DevTools :: Storage Inspector, enhancement, P1)
DevTools
Storage Inspector
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: jsnajdr)
References
Details
Attachments
(1 file, 2 obsolete files)
30.10 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•9 years ago
|
Severity: normal → enhancement
Whiteboard: [devtools-html]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jsnajdr
Assignee | ||
Comment 1•9 years ago
|
||
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.
Attachment #8745261 -
Flags: review?(mratcliffe)
Attachment #8745261 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 2•9 years ago
|
||
Depends on: 1205123
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.
Attachment #8745261 -
Flags: review?(mratcliffe) → feedback+
Reporter | ||
Comment 4•9 years ago
|
||
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!
Reporter | ||
Comment 5•9 years ago
|
||
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?
Attachment #8745261 -
Flags: review?(ejpbruel) → review+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P3
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Keywords: checkin-needed
Assignee | ||
Comment 9•9 years ago
|
||
Adapted to the final API - use protocol.generateActorSpec
Attachment #8747019 -
Flags: review?(ejpbruel)
Assignee | ||
Updated•9 years ago
|
Attachment #8745261 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Fixed eslint errors, new try.
Attachment #8747024 -
Flags: review?(ejpbruel)
Assignee | ||
Updated•9 years ago
|
Attachment #8747019 -
Attachment is obsolete: true
Attachment #8747019 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 12•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8747024 -
Flags: review?(ejpbruel) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Keywords: checkin-needed
Comment 14•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•9 years ago
|
Iteration: --- → 49.1 - May 9
Priority: P3 → P1
Updated•8 years ago
|
Whiteboard: [devtools-html]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•