1731967 - Rename watched data entries to session data

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Alexandre Poirot [:ochameau]]

We originaly introduced "watched data" in bug 1620243 to only convey the list of watched resources. Now, we built a significant framework layer around this data set, and use it for a much broader set of data. This naming is confusing now that we also store breakpoints and target configurations.

Here is the list of data we currently store via this API:

• the list of observed resource types (list of strings: console-message, thread-state, sources, ...)
• the list of observed target types (list of strings: frame, content-process, worker, webextension)
• the list of breakpoints (list of objects with source ID, url, line column attributes)
• the list of XHR breakpoints (list of objects with path and method attributes)
• the list of target and thread configurations (list of arbitrary attributes refering to primitive data types. Ex: cacheDisabled, javascriptEnabled, pauseOnExceptions, skipBreakpoints,... )

This data is meant to be easily available:

• anywhere. From the parent process, but also from all content processes as well as any worker thread. More generaly, anywhere where DevTools code runs.
• anytime. By that I especially mean being available early on, before a debuggable context starts running. This data typically has to be available very early when a new content process starts. Or before a worker thread starts running.

Because this data has to be synchronized across processes and threads, it should be efficient. We should typically avoid passing the whole dataset on each incremental change.
This is why we currently have an API to add and remove a precise list of items for each data type. This is also why we have this convoluted "key" mechanism for each list in order to know which element is being added or removed.

Given this constraint, data can be split into three distinct codepaths:

• any primitive data (boolean, string, number, ...) can be passed as-is, via an API as simple as set(type, value). This can be used for target and thread configurations.
• list of primitive data can also have a simple API: addListItems(type, newEntries) and removeListItems(type, removedEntries). This can be used for resources and target types, which are list of strings.
• list of objects with attributes being primitive data. This can use same API as list of primitive data, but each object need a unique identifier. This can be used for breakpoints which are a list of objects with string attributes.

We do not support more complex data. Typically: list of lists, or list of objects with attribute other than primitive types.

Now, the current implementation only exposes one API which fits these three usecases.
The first case, simple primitive data is actually implemented via a list of objects, where we store a { key: type, value: value} object for each data. And where key attribute is used as the unique identifier.
And the second case, list of primitive data is implemented with an identity key function, as each primitive value is actually a unique identifier.

Because of this, the current implementation introduces some confusion regarding when to introduce a new list/type.
For example:

• Should we introduce a new type for each new primitive data?
• Should we instead group some primitive data into groups that makes sense altogether (this is what we do with target versus thread configurations)
• Should we group all primitive data into a shared list? May be we can keep this as an implementation detail.

Having said all that, we can probably improve things incrementally:

• First rename all occurences "watch*" (watched, watch, watcher) data to "Session data". Rename modules, function and variable names. Note that WatcherActor is independant of this and shouldn't be changed here.
• Revise WatcherActor's addDataEntry/removeDataEntry APIs.

About the naming "Session Data". This sounds like a good fit as this data is unique per toolbox. You have only one data set per toolbox. So we could easily describe the WatcherActor as creating a new "session" being specific to a given debugging context (one tab or the whole browser for now. But it will also support one webextension or one worker). And this data is shared across all processes and threads for the current debugging session.
Then, it may be benefitial to rename the WatcherActor to SessionActor or something along these lines? As this will be the top class which coordinates and defines the session.

About the API revision. What about doing this:

• setSessionDataPrimitive(type, value) For any boolean, string, number,... Pass undefined to delete it? (alternative: setSessionDataAttribute, setSessionDataPrimitiveAttribute, ...)
• updateSessionDataList(type, toAddOrUpdateItems, toDeleteItems) For any data which is made or a list. If items aren't primitive types, a key function should be provided to compute a key object out of the item object. -or- we expect all objects to come with an id attribute. (alternative: updateSessionDataListAttribute)
• clearSessionDataList(type) to clear out a data type which is a list. (alternative: deleteSessionData and work against any type, not only lists)

It would look like this:

// Disable JS
watcherActor.setSessionDataPrimitive("javascriptEnabled", false);
// Add a breakpoint
watcherActor.updateSessionDataList("breakpoints", [{sourceUrl: "http://...", ...}], []);
// Remove a breakpoint
watcherActor.updateSessionDataList("breakpoints", [], [{sourceUrl: "http://...", ...}]);
// Clear all breakpoints
watcherActor.deleteSessionData("breakpoints");
```

Comment 1

[Julian Descottes [:jdescottes]]

Thanks for the write up. The naming changes sound good, and they should be aligned with bidi.

Regarding the API change, I am not convinced about having first class support for primitives. I would prefer having a consistent API and data structure rather than having shortcuts for primitives.

Not too far from what you proposed, I could see a set of 3 methods:

• setSessionData(key, [value1, value2]) -> fully replaces the current data for key.
• updateSessionData(key, { add: [value1, value2], remove: [value3, value4] }) -> adds (or updates if the key is identical) some items, removes some items. Would initially have suggested addSessionData/removeSessionData, but I imagine we want to have the option to perform both at once?
• deleteSessionData(key)

Ultimately if you prefer to split the APIs, I would still suggest to have a setSessionDataList, and to use named arguments for the add/remove arrays in updateSessionDataList.

The API will always be a bit odd, because we ask for arrays, but we turn them into sets (or maps) using custom id functions.

I would love to see SessionData documented in a single spot. It would be great if you could look at how the current data is structured. Maybe there is already a data entry which would fit for your new feature. Maybe you can draw inspiration from another existing data entry, and use a similar structure.

Comment 2

[Julian Descottes [:jdescottes]]

For info, the patch for adding session data support to BiDi is currently under review at https://phabricator.services.mozilla.com/D127698

The API for now is a basic:

• getSessionData(type)
• addSessionData(type, values, context)
• removeSessionData(type, values, context)
• _setSessionData(type, sessionDataItems)

The context notion is bidi specific, because whereas in DevTools all the data for a given session is scoped to a given "context" (either whole browser or a browserId), in BiDi each command can target a different context. For instance you can subscribe to one event globally and to another event just for a tab.

I might update the patch to replace add/removeSessionData with an updateSessionData({ add, remove }) (it slipped my mind).

Comment 3

[Hubert Boma Manilla (:bomsy)]

Thanks alot for this Alex. This makes a lot of sense.

I just have some thoughts around the api naming as well

1. I'm not sure about naming based on the data types , rather i would think that just naming based on the actions performed by the api feels better.
   In a way the type of data will determine which api should be used

2. Also these might feel confusing (to me) and the arrays can be mixed up

// Add a breakpoint
watcherActor.updateSessionDataList("breakpoints", [{sourceUrl: "http://...", ...}], []);
// Remove a breakpoint
watcherActor.updateSessionDataList("breakpoints", [], [{sourceUrl: "http://...", ...}]);

My suggestions would be something like these below :-

This api is likely used for

• setting primitive types
• fully replaces lists (as also suggested by julian)

watcherActor.setSessionData(key, boolean | itemsToReplaced);

These are used for list of items
watcherActor.addSessionData(key, toAddItems);
watcherActor.removeSessionData(key, toRemoveItems);
watcherActor.clearSessionData(key)

This feels a lot kinda simpler, and someone can intuitively guess what the api does and how it is used.

Comment 4

[Alexandre Poirot [:ochameau]]

I'll use this bug to first rename Watched/Watcher data to session data everywhere and I'll open a followup to change the API and its parameters.
Some more thoughts and a final call should be made regarding the API change, while the renaming can be made right away.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1731967 - [devtools] Renamed all "watched data" to "session data".

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1731967 - [devtools] Renamed all "watcher data" to "session data".

Comment 7

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=102f548f232d3fbbdd66dfcc9f1be48d47bc8894

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3170bd67b4ad
[devtools] Renamed all "watched data" to "session data". r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/845d11ccb435
[devtools] Renamed all "watcher data" to "session data". r=nchevobbe

Comment 9

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/3170bd67b4ad
https://hg.mozilla.org/mozilla-central/rev/845d11ccb435

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)

I'll use this bug to first rename Watched/Watcher data to session data everywhere and I'll open a followup to change the API and its parameters.
Some more thoughts and a final call should be made regarding the API change, while the renaming can be made right away.

Hi Alex! Did you end up filing a followup for the API change? We are discussing this at length in https://phabricator.services.mozilla.com/D127698

Comment 11

[Alexandre Poirot [:ochameau]]

Done, opened bug 1740537 to tweak the API itself.