Note: There are a few cases of duplicates in user autocompletion which are being worked on.

No contextualIdentity events

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
5 months ago
21 hours ago

People

(Reporter: andym, Assigned: jkt)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [contextualIdentities], triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
Currently I can query to find out details about a contextualIdentity, but I can't tell when they change in a webExtension. In my extension I list all the tabs and show the colour of a container. However if a user goes to "Manage containers" and changes the colour from yellow to orange... the WebExtension won't know without querying again.

The standard way to do this is some events like:

contextualIdentities.onCreated
contextualIdentities.onChanged
contextualIdentities.onRemoved

The extension can then addListeners if they'd like to find out when they change.

Updated

4 months ago
Priority: -- → P3
Whiteboard: [contextualIdentities] → [contextualIdentities], triaged
(Assignee)

Updated

3 months ago
Blocks: 1191418
(Assignee)

Comment 1

2 months ago
There isn't any implications in adding this is there?
This would be useful in other extensions outside the Test Pilot.
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request)
No particular issue. BTW, are they needed for Test Pilot?
Flags: needinfo?(amarchesini) → needinfo?(jkt)
(Assignee)

Comment 4

2 months ago
No as we are in charge of all the creation points, so it's not needed. (if users have managed to add through the about:preferences panel then they have a bug as we disable it).
Flags: needinfo?(jkt)
(Assignee)

Comment 5

2 months ago
Assuming I add a test or 5 does the current attached patch look ok?
Flags: needinfo?(amarchesini)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review142600

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:175
(Diff revision 1)
>    },
>  
>    create(name, icon, color) {
> +    let userContextId = ++this._lastUserContextId;
>      let identity = {
> -      userContextId: ++this._lastUserContextId,
> +      userContextId: userContextId,

just: userContextId,
Attachment #8867511 - Flags: review?(amarchesini)

Comment 7

2 months ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review142602
Attachment #8867511 - Flags: review?(amarchesini) → review+
Yes, it looks correct. A test is needed, and also a proper review from a webextension peer.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 days ago
Blocks: 1354601
(Assignee)

Comment 9

2 days ago
As we are likely to launch this on AMO this will cause issues with our addon not being the only crud end points for containers. We will need these events to ensure menus are updated correctly and others will need it to reduce calling the API repeatedly.
(Assignee)

Comment 10

2 days ago
So my current thinking is:
- Use onChanged like cookies or use onUpdated like tabs?
- Should we flatten the contextual identity to be the return value or should we nest it under a "contextualIdentity" key like cookies do
- We don't have the full object for removed, should I flatten this differently?

Curently:

browser.contextualIdentities.onCreated.addListener((e) => {console.log(e);});

// {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}}


browser.contextualIdentities.onChanged.addListener((e) => {console.log(e);});

// {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}}


browser.contextualIdentities.onRemoved.addListener((e) => {console.log(e);});

// {cookieStoreId: 12}


My rationale to nest in the "contextualIdentity" key is if we needed reason etc. However this feels a little excessive for remove. I could possibly make the event pass the deleted data for the container?
Flags: needinfo?(amckay)
(Assignee)

Comment 11

a day ago
I'm going to make the following changes and hope they are ok:

- onUpdated as we have contextualIdentities.update rather than change.
- Return the full object to be consistent on onRemoved
- Serialise the JSON structure of the current state of the container in ContextualIdentitiesService and unserialize in the extension code rather than query again.

Should we care about the user manually flipping the container prefs outside adding our own containers changes? We could file a follow up bug to trigger onUpdated and onRemoved listeners on pref change to the current list of containers.
Flags: needinfo?(amckay)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

21 hours ago
I made a demo extension as part of testing this:
https://github.com/jonathanKingston/extension-debugging/tree/master/container-events

Feel free to use this code in the documentation process or update the one Andy had.

Comment 14

21 hours ago
mozreview-review
Comment on attachment 8867511 [details]
Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved

https://reviewboard.mozilla.org/r/139050/#review164622

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:195
(Diff revision 2)
>        name
>      };
>  
>      this._identities.push(identity);
>      this.saveSoon();
> +    Services.obs.notifyObservers(null, "contextual-identity-created", this.getIdentityObserverOutput(identity));

indentation.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:213
(Diff revision 2)
>        delete identity.l10nID;
>        delete identity.accessKey;
>  
> -      Services.obs.notifyObservers(null, "contextual-identity-updated", userContextId);
>        this.saveSoon();
> +      Services.obs.notifyObservers(null, "contextual-identity-updated", this.getIdentityObserverOutput(identity));

indentation.

::: toolkit/components/extensions/ext-contextualIdentities.js:23
(Diff revision 2)
>  
>    return result;
>  };
>  
> +const convertIdentityFromObserver = identityJSON => {
> +  let identity = JSON.parse(identityJSON);

This seems fragile. Would be nice if we create the object passing 1 by 1 the parameter we want from the JSON object. Sometimes I wish JS had MOZ_ASSERT()
Attachment #8867511 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Updated

21 hours ago
Assignee: nobody → jkt
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.