Open Bug 1740537 Opened 3 years ago Updated 3 years ago

Tweak SessionData APIs to better support replace and clarify lists maintenance

Categories

(DevTools :: Framework, task)

Product:
DevTools
Component:
Framework
Type:
task

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

(Blocks 1 open bug)

Details

Bug 1731967 started renaming watcher data entries to session data, but the API methods may benefit from being tweaked.

The current code exposes:

Some discussion already happened in bug 1731967 to revise this API.

The current API faces two challenges:

  1. it only allows to remove and add items in the Set of objects stored for each type. i.e. we can't easily replace the whole Set with a new set of objects. You need to remove each previously registered items. We workaround this while implementing Event breakpoints:
    https://searchfox.org/mozilla-central/rev/a48e21143960b383004afa9ff9411c5cf6d5a958/devtools/server/actors/breakpoint-list.js#81-92
  2. it only allows to store Set of values for each type. This is powerful, but also a bit complex to understand when we want to store primitive values for a given type. We workaround this while implementing target and thread configurations:
    https://searchfox.org/mozilla-central/rev/a48e21143960b383004afa9ff9411c5cf6d5a958/devtools/server/actors/target-configuration.js#169-180

To address the first point, it is easy, we need to expose a function that allows to do a full replace. For example: WatcherActor.setDataEntries(type, entries) and WatcherRegistry.setSessionDataEntries(type, entries).
For the second, it is slightly more complex as we might need to complexify the API to support simple and complex independantly.

Option A

In bug 1731967, I suggested to expose two APIs, one for Set of values and another one for primitive values.
So that breakpoints, which are set of values, could use a set-oriented API:

watcherActor.updateSessionDataSet("breakpoints", {
  toAddOrUpdate: [{sourceUrl: "http://foo.com", ...}], 
  toRemove: [{sourceUrl: "http://bar.com", ...]
});

While configurations could use a simplier API:

watcherActor.setSessionData("target.disableJavascript", true);

And we might reuse the same method to replace a Set content

watcherActor.setSessionData("breakpoints", [{sourceUrl: "http://foo.com", ...}]);

Option B

But the feedback gathered from bug 1731967 highlighted that it wasn't necessarily simplifying things.
We may stick to only one external API, which would only allow storing Set of values in Session Data. i.e. we wouldn't address point 2).
A first method would replicate the existing implementation, while exposing only one method instead of two (add + remove). The benefit of having one is that you can add and remove elements in one call. So if you have to add and remove you would only do one RPC call and only communicate to all the targets once.

updateSessionData(type, {
  toAddOrUpdate: [...], // List of values to add to the set or update if already present
  toRemove: [...], // List of values to remove from the set
);

Otherwise, we may keep two methods if we don't think we will have cases doing add and remove at the same time:

addSessionData(type, [...]);
removeSessionData(type, [...])

Note that for now, we only add and remove for updating EventBreakpoints, but that's because we don't have a "setSessionData" API which would replace the whole Set.
Also note, that you always have to pass an array in all these methods.
If you have to store a Set of booleans, you have to do something similar to target configurations.

I'm still not bound to one particular option.
Option B is mostly a renaming, with the addition of a "replace" method. Otherwise we wouldn't change much compared to what we have today.

At the end, we are at the opposite position of Bidi patches.
We support lists/sets, but not primitives.
Option B stays on this ground, while A tries to bend toward a better support of primitives.

You need to log in before you can comment on or make changes to this bug.