If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Allow actor types to share a front type

NEW
Unassigned

Status

()

Firefox
Developer Tools
3 years ago
3 years ago

People

(Reporter: dcamp, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8496233 [details] [diff] [review]
protocol-frontclass.diff

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]
protocol-frontclass.diff

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)
(Reporter)

Comment 2

3 years ago
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.
(Reporter)

Comment 4

3 years ago
Created attachment 8498299 [details] [diff] [review]
v2
Attachment #8498299 - Flags: review?(jryans)
(Reporter)

Updated

3 years ago
Attachment #8496233 - Attachment is obsolete: true
Comment on attachment 8498299 [details] [diff] [review]
v2

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-
You need to log in before you can comment on or make changes to this bug.