Open Bug 1073685 Opened 5 years ago Updated 2 years ago

Allow actor types to share a front type


(DevTools :: General, defect)

Not set


(Not tracked)


(Reporter: dcamp, Unassigned)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

Attached patch protocol-frontclass.diff (obsolete) — Splinter Review
For the adapter plugin, we're implementing new versions of the same actors that have to live alongside the existing actors.  There's no sense sending those extra, identical actor type descriptions over the wire, so this change allows an actor to specify that it shares a front type with another actor.
Attachment #8496233 - Flags: review?(jryans)
Comment on attachment 8496233 [details] [diff] [review]

Review of attachment 8496233 [details] [diff] [review]:

Hmm, so you have to set |frontType| and |useFront|?  Where can I see an example of this in use?

::: toolkit/devtools/server/protocol.js
@@ +917,5 @@
> +  // only one copy of the interface will be sent across in the
> +  // protocolDescription method.
> +  if (actorProto.frontType) {
> +    protoSpec.frontType = types.getType(actorProto.useFront);
> +  }  

Nit: trailing whitespace
Attachment #8496233 - Flags: review?(jryans)
No, I changed from useFront -> frontType and obviously didn't do the change completely.
A test would be nice too, mostly to have as a usage example in the codebase.
Attached patch v2Splinter Review
Attachment #8498299 - Flags: review?(jryans)
Attachment #8496233 - Attachment is obsolete: true
Comment on attachment 8498299 [details] [diff] [review]

Review of attachment 8498299 [details] [diff] [review]:

::: toolkit/devtools/server/protocol.js
@@ +910,5 @@
> +  // There might be multiple implementations for the same basic actor type -
> +  // specifically, our multi-protocol adapter provides different
> +  // implementations of the same actor tree.
> +  // The frontType option allows an implementation to note that it uses the same

Does this get strange as future versions evolve?  What happens if you use this to note you are the "same as" another type, but then either your own actor or the other one you are "noting" have evolved over time to differ from one another?

This seems like it might get confusing, because it makes a claim that, while perhaps true at one point in time, might not remain true for very long.

Perhaps it is better to use a boolean flag like "describable: false" to hide certain types from the protocol dump?  Keep the actual type seems unneeded since you only test if it's truthy.

Alternatively, is it really that bad to just leave them in the dump?  It's more data, but then it's left up to the consumer to process that info as they wish.  When you are dealing with data used to maintain version compatibility, I would err on the side of having more info than less.
Attachment #8498299 - Flags: review?(jryans) → review-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.