The default bug view has changed. See this FAQ.

Add WE API to provide functions of `SessionStore.setWindowValue` and `SessionStore.setTabValue`

ASSIGNED
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: General
P3
normal
ASSIGNED
4 months ago
3 days ago

People

(Reporter: Kevin Jones, Assigned: Kevin Jones, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [design-decision-approved][sessions]triaged)

Attachments

(1 attachment, 11 obsolete attachments)

(Assignee)

Description

4 months ago
I am needing to store data which will be attached to the session for WE support for my addons "Tab Groups Helper" and "All Tabs Helper".  IE, data would be able to be found in sessionstore.js file.  Ideally this would be per tab and per window, just as `SessionStore.setWindowValue` and `SessionStore.setTabValue` currently are.
(Assignee)

Updated

4 months ago
Summary: Add WE API to proved functions of `SessionStore.setWindowValue` and `SessionStore.setTabValue` → Add WE API to provide functions of `SessionStore.setWindowValue` and `SessionStore.setTabValue`
Can you give some more details about the use case?  What do you wish to accomplish with these methods that can't be done using other existing storage APIs?
(Assignee)

Comment 2

4 months ago
I have addons that have specific data associated with individual tabs, that needs to remain intact across session restores.  ie, it is data that needs to be associated with session data.  One example is I have an addon that has an auto unload feature.  However the user has the option to select some tabs to never auto unload.  The same flagged tabs would need to come up as flagged in a session restore.

AFAICT, there wouldn't be a way to do this using existing storage APIs, as there would be no way to absolutely identify tabs on startup to link them to the proper tab identity.

Another reason for having a way to associate and save data with SessionStore data is that some data is session specific.  In this way all data can be saved during shutdown in sessionstore.js, without requiring a user or app to find some of the data elsewhere and attempt to link it up (if it is possible).

While the second case may be more of a (great) convenience factor for developers, the first case is pretty much necessary to have a bug-free (and not extremely hackish) method to always properly associate data for specific tabs on session restore.
Did you check out browser.sessions API [1]?  It got implemented recently in Nightly, and as far as I can tell, it provides unique identifiers for tabs/windows saved in/restored from session storage, which you might be able to use as keys for your storage.

Or if those APIs are not sufficient for your use case, maybe you could suggest how to improve them, as that seems the most logical place where the functionality you are requesting might go.

1) https://developer.chrome.com/extensions/sessions
(Assignee)

Comment 4

4 months ago
(In reply to Tomislav Jovanovic :zombie from comment #3)
> Did you check out browser.sessions API [1]?  It got implemented recently in
> Nightly, and as far as I can tell, it provides unique identifiers for
> tabs/windows saved in/restored from session storage, which you might be able
> to use as keys for your storage.
> 
> Or if those APIs are not sufficient for your use case, maybe you could
> suggest how to improve them, as that seems the most logical place where the
> functionality you are requesting might go.
> 
> 1) https://developer.chrome.com/extensions/sessions

I checked on today's Nightly, apparently the implementation is not complete.  The only properties I get for browser.sessions are:

MAX_SESSION_RESULTS
getRecentlyClosed
restore
onChanged

I am unable to access `browser.sessions.Session` (`undefined`), thus no `browser.sessions.Session.windows` or `browser.sessions.Session.tabs`.

I assume by "unique identifiers for tabs/windows" you are referring to `sessionId`.  I am not sure, but in reading it sounds like this would be an identifier which identifies tabs/windows to belong to a given session, and not unique to the tabs/windows themselves.

But I will just have to wait until the API is complete in Firefox to see what exactly they mean, as checking `tabs.Tab.sessionId` shows `undefined`.  `"tabs"` and `"sessions"` permissions are set.

Checking sessionstore.js, I so no unique identifiers for tabs.

Still, examining `chrome.session` documentation, it does not appear there is anything close to functionality for an WE addon to assign any data to a tab or window which would carry across sessions.

Comment 5

3 months ago
this is under consideration - can you provide a more detailed use case so we make sure to hit the needed requirements.  exactly what you are trying achieve.
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 6

3 months ago
For example:

Tree Style Tabs currently stores data peculiar to its tab arrangements in SessionStore:

"extData":{"treestyletab-id":"tab-<1482441431736-59932>","treestyletab-insert-before":"tab-<1482441431773-32701>","treestyletab-insert-after":"tab-<1482441431699-44104>"}

Currently, by accessing

`SessionStore.getTabValue(tab, "treestyletab-id...etc")`

for each tab, I can obtain data which gives me details which will affect how I display tab items in my All Tabs Helper addon menu.  In this way, I can achieve complimentarity between addons.

Also, by storing my own data this way:

`SessionStore.setTabValue(tab, "athlpr_noAutoUnload"", "1")`

I have data that persists with the session, and can be preserved through session management tools such as Session Manager, or the internal backup manager in my own addon Tab Groups Helper.
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 7

3 months ago
(In reply to :shell escalante from comment #5)
> exactly what you are trying achieve.

Pretty much everything that is achievable with the current `SessionStore.setWindowValue` and `SessionStore.setTabValue` APIs.

Updated

2 months ago
Whiteboard: [design-decision-denied]triaged

Comment 8

2 months ago
Oops, sorry for the typo :)
Whiteboard: [design-decision-denied]triaged → [design-decision-needed]triaged

Updated

2 months ago
Duplicate of this bug: 1280222
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P3
Whiteboard: [design-decision-needed]triaged → [design-decision-approved][sessions]triaged
Kevin, are you interested in trying to implement this yourself, with assistance from me?
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 11

22 days ago
Yes.
Flags: needinfo?(kevinhowjones)
(In reply to Kevin Jones from comment #11)
> Yes.

Great, let me know if you need any help.
Assignee: bob.silverberg → kevinhowjones
(Assignee)

Comment 13

21 days ago
Bob, is there an existing API this should be a part of, or will it have its own API?
Flags: needinfo?(bob.silverberg)
It should be added to the existing sessions API. You can take a crack at designing the API if you like. As you are in need of this feature you're in a good position to discuss how you think it should work. You could suggest what the API should look like, and I and my team can provide feedback on your proposal.

Thanks for offering to work on this!
Flags: needinfo?(bob.silverberg) → needinfo?(kevinhowjones)
(Assignee)

Comment 15

19 days ago
Created attachment 8843712 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V1.diff

Patch ext-sessions.js and sessions.json
Flags: needinfo?(kevinhowjones)
Attachment #8843712 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 16

19 days ago
Created attachment 8843713 [details]
WE_sessions_git.xpi

Addon to test Attachment #8843712 [details] [diff]
Comment on attachment 8843712 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V1.diff

Review of attachment 8843712 [details] [diff] [review]:
-----------------------------------------------------------------

This is great work, Kevin. Thanks for taking the time to do this. I have added a number of comments to the review which need to be addressed, but it's looking good for the most part. 

In addition to addressing the comments, before landing this will need to have some tests added. Please take a look at the existing tests in /browser/components/extensions/test/browser/, for example browser_ext_sessions_getRecentlyClosed.js, to see what some of our tests look like, and let me know if you have any questions about this review or writing some tests.

Also, we tend to use MozReview now for looking at patches, rather than attaching diffs to bugs. It's fine if you want to continue this way, but if you want to look into using MozReview instead that might make things a bit smoother.

::: control/ext-sessions.js
@@ +9,5 @@
>  } = ExtensionUtils;
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
>                                    "resource:///modules/sessionstore/SessionStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionParent",

This isn't needed. See below.

@@ +57,5 @@
>  }
>  
>  extensions.registerSchemaAPI("sessions", "addon_parent", context => {
>    let {extension} = context;
> +  let {tabTracker} = ExtensionParent.apiManager.global;

These are not needed. `tabTracker` and `windowTracker` are available to the API code without importing.

@@ +97,5 @@
>          return createSession(session, extension, closedId);
>        },
>  
> +      setTabValue: function(tabId, key, value) {
> +        let tab = tabTracker.getTab(tabId, null, null);

This should just be `let tab = tabTracker.getTab(tabId)`. The second argument isn't needed and there is no third argument to `getTab`. Same below in `getTabValue`.

::: control/sessions.json
@@ +130,5 @@
> +      {
> +        "name": "setTabValue",
> +        "type": "function",
> +        "description": "Set a key/value pair on a given tab.",
> +        "async": true,

This should be "async": "callback" throughout the file, and you'll need to include a callback parameter for each function as well. See getRecentlyClosed in this file for an example.

@@ +135,5 @@
> +        "parameters": [
> +          {
> +            "type": "number",
> +            "name": "tabId",
> +            "optional": false,

You can omit the "optional" property if it is false. It only needs to be specified when it is true. This applies to all of the properties below.

@@ +136,5 @@
> +          {
> +            "type": "number",
> +            "name": "tabId",
> +            "optional": false,
> +            "description": "The id of the tab we're setting the key/value pair on."

I would change the wording to not say "we're". Perhaps something like "The id of the tab on which the key/value pair is to be set." The same goes for the other descriptions below.
Attachment #8843712 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 18

17 days ago
(In reply to Bob Silverberg [:bsilverberg] from comment #17)
> Comment on attachment 8843712 [details] [diff] [review]
> API_sessions_setgetWindowTabValue_V1.diff
> 
> Review of attachment 8843712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also, we tend to use MozReview now for looking at patches, rather than
> attaching diffs to bugs. It's fine if you want to continue this way, but if
> you want to look into using MozReview instead that might make things a bit
> smoother.

Okay.  If it is all the same to you, I would prefer to just upload patches to the bug for now.

> ::: control/sessions.json
> @@ +130,5 @@
> > +      {
> > +        "name": "setTabValue",
> > +        "type": "function",
> > +        "description": "Set a key/value pair on a given tab.",
> > +        "async": true,
> 
> This should be "async": "callback" throughout the file, and you'll need to
> include a callback parameter for each function as well. See
> getRecentlyClosed in this file for an example.

Interesting, I was told on another bug just the opposite.  I had callbacks, but was told to use `"async": true` because we're not supporting callbacks, unless we're supporting Chrome APIs.

> I would change the wording to not say "we're". Perhaps something like "The
> id of the tab on which the key/value pair is to be set." The same goes for
> the other descriptions below.

Yeah, all of those descriptions were really just stubs.  I figured they would get changed.
(In reply to Kevin Jones from comment #18)
> 
> Okay.  If it is all the same to you, I would prefer to just upload patches
> to the bug for now.
>

Sure, no problem.
 
> > 
> > This should be "async": "callback" throughout the file, and you'll need to
> > include a callback parameter for each function as well. See
> > getRecentlyClosed in this file for an example.
> 
> Interesting, I was told on another bug just the opposite.  I had callbacks,
> but was told to use `"async": true` because we're not supporting callbacks,
> unless we're supporting Chrome APIs.
>

I stand corrected. I didn't know that, but yes, you are correct. Use `"async": true`.
(Assignee)

Comment 20

17 days ago
Created attachment 8844605 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V2.diff

Update per comment 17
Attachment #8843712 - Attachment is obsolete: true
Attachment #8844605 - Flags: review?(bob.silverberg)
Comment on attachment 8844605 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V2.diff

Looks good Kevin, nice work. Now it just needs some tests.
Attachment #8844605 - Flags: review?(bob.silverberg)
(Assignee)

Comment 22

17 days ago
(In reply to Bob Silverberg [:bsilverberg] from comment #17)
> ... and let me know if you have any questions about this review or
> writing some tests.

Hello Bob,

The `background` property passed to ExtensionTestUtils.loadExtension  which references a function is a bit of a mystery to me.  Can you tell me the role that the referenced function plays in the test?

Thanks
(In reply to Kevin Jones from comment #22)
> 
> The `background` property passed to ExtensionTestUtils.loadExtension  which
> references a function is a bit of a mystery to me.  Can you tell me the role
> that the referenced function plays in the test?
>

The `background` property ends up being the contents of the background page for the extension that is loaded for the test. So any API code that you want to test needs to be in that background function and it will be executed by the extension.

Does that clear it up?
(Assignee)

Comment 24

16 days ago
(In reply to Bob Silverberg [:bsilverberg] from comment #23)
> (In reply to Kevin Jones from comment #22)
> > 
> > The `background` property passed to ExtensionTestUtils.loadExtension  which
> > references a function is a bit of a mystery to me.  Can you tell me the role
> > that the referenced function plays in the test?
> >
> 
> The `background` property ends up being the contents of the background page
> for the extension that is loaded for the test. So any API code that you want
> to test needs to be in that background function and it will be executed by
> the extension.
> 
> Does that clear it up?

Yes, that makes sense.
(Assignee)

Comment 25

15 days ago
Created attachment 8845431 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js

Mochitest
Attachment #8845431 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 26

15 days ago
Created attachment 8845715 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js V2

Mochitest - Fixed spacing problem
Attachment #8845431 - Attachment is obsolete: true
Attachment #8845431 - Flags: feedback?(bob.silverberg)
Attachment #8845715 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 27

15 days ago
Created attachment 8845716 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js V3

Ugh!! Corrected typo in test description.

Sorry for all the noise :-/
Attachment #8845715 - Attachment is obsolete: true
Attachment #8845715 - Flags: feedback?(bob.silverberg)
Attachment #8845716 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 28

14 days ago
I realized this should have deleteTabValue and deleteWindowValue as well.
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 29

14 days ago
Created attachment 8845902 [details]
browser_ext_sessions set/get/delete tab/window value

Updated test, includes delete.
Attachment #8844605 - Attachment is obsolete: true
Attachment #8845716 - Attachment is obsolete: true
Attachment #8845716 - Flags: feedback?(bob.silverberg)
Flags: needinfo?(bob.silverberg)
Attachment #8845902 - Flags: feedback?(bob.silverberg)
(Assignee)

Comment 30

14 days ago
Created attachment 8845904 [details] [diff] [review]
API_sessions set/get/delete window/tab value patch V1

Updated patch adds delete.
Attachment #8845904 - Flags: review?(bob.silverberg)
(Assignee)

Comment 31

14 days ago
Created attachment 8845905 [details]
browser_ext_sessions set/get/delete tab/window value test

Updated test, includes delete.
Attachment #8845902 - Attachment is obsolete: true
Attachment #8845902 - Flags: feedback?(bob.silverberg)
Attachment #8845905 - Flags: review?(bob.silverberg)
Comment on attachment 8845904 [details] [diff] [review]
API_sessions set/get/delete window/tab value patch V1

Review of attachment 8845904 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, Kevin, although I thought of another issue. Should all of this be constrained to the current extension? For example, what if both Extension A and Extension B set a tab value for the key `key1`? That could cause issues for either/both extensions. It seems like we should have logic so that set/get/delete only deal with data that "belongs" to the current extension.

Do you agree?
Attachment #8845904 - Flags: review?(bob.silverberg)
Comment on attachment 8845905 [details]
browser_ext_sessions set/get/delete tab/window value test

This is a good start to the tests, Kevin. Nice work. Because it is just a js file that is attached, and not a diff, Bugzilla isn't giving me the review interface I got with the other attachment, so I'm just going to make some general comments in here.

- This shouldn’t be a separate attachment. For the next iteration please include this in the patch which also contains the changes to ext-sessions.js and sessions.json.
- You don’t need to make the background functions in the test async as they are not awaiting anything.
- We should not have tests that rely on each other. Any test should theoretically be able to be run individually and work as well as it does when run with other tests in the file. So the fact that test_sessions_get_tab_value relies on test_sessions_set_tab_value is not good. I suggest that you combine the tests for get, set and delete into a single test, which would resolve this problem. You can use extension.sendMessage and extension.awaitMessage to have the background script do the API calls and also pass information back to the test so that you can verify it via the native SessionStore methods as you are doing currently. I think one test for tabs and one test for windows would be sufficient.

Finally, I am going to be away for the next two weeks, so I will try to find one of my colleagues to take over responsibility for reviewing the code for this bug in my absence. I will update the bug with that info once I know who that person will be.
Attachment #8845905 - Flags: review?(bob.silverberg)
(Assignee)

Comment 34

14 days ago
(In reply to Bob Silverberg [:bsilverberg - on PTO until 03/27] from comment #32)
> Comment on attachment 8845904 [details] [diff] [review]
> API_sessions set/get/delete window/tab value patch V1
> 
> Review of attachment 8845904 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, Kevin, although I thought of another issue. Should all of
> this be constrained to the current extension? For example, what if both
> Extension A and Extension B set a tab value for the key `key1`? That could
> cause issues for either/both extensions. It seems like we should have logic
> so that set/get/delete only deal with data that "belongs" to the current
> extension.
> 
> Do you agree?

One of the use cases I outlined in comment 6 was to expand compatibility with other addons, in that case Tree Style Tabs, where it was important to access the TST tab value data in order to make decisions in my own addon so as not to conflict with TST.  In cases such as these it is important that addons be able to access the data from other addons.  So I would actually be in favor of not putting on those constraints.  In the current XUL implementation those constraints don't exist.  It would be very unlikely that overwriting would occur accidentally as addons tend to use names that contain segments unique to their extension.

In any case I am happy to yield to your opinion.
To be honest I'm not sure which is best, although I am leaning towards extensions only having access to their own data.

Andrew and/or Kris, what do you think? Also, would one of you be willing to take over helping with this patch until I return in a couple weeks time?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)

Comment 36

13 days ago
A lot of add-ons are working together with each other today, please don't prevent such things in the future (or make them extremly complicated).

Maybe prefix the data via default with some add-on ID, but give the option to access other add-on's values when explicitly wanted (by an optional parameter specifying the other add-on id?). By doing so you prevent add-ons accidentally overwriting other add-on's data, but still give the option if wanted.
(Assignee)

Comment 37

12 days ago
Created attachment 8846505 [details] [diff] [review]
API_sessions_window_tab_value_V5.diff
Attachment #8845904 - Attachment is obsolete: true
Attachment #8845905 - Attachment is obsolete: true
Attachment #8846505 - Flags: feedback?(kmaglione+bmo)
Attachment #8846505 - Flags: feedback?(aswan)
(Assignee)

Comment 38

12 days ago
I have uploaded a new patch that does not contain the restrictions discussed in comment 34, comment 35.  Perhaps we can land the changes without the restrictions, and if a compelling reason ever presents itself in the future we can introduce those later.
I haven't read everything, but if the question is whether extensions should be able to see data stored by other extensions, I would argue for no, extensions should use runtime.sendMessage() to communciate with other extensions.
In that way, an extension can decide exactly what information it wants to share and which extensions it wants to share with, with the safe default being "nothing/nobody"
Flags: needinfo?(aswan)

Comment 40

10 days ago
Comment on attachment 8846505 [details] [diff] [review]
API_sessions_window_tab_value_V5.diff

Review of attachment 8846505 [details] [diff] [review]:
-----------------------------------------------------------------

You'll probably want to add your test to: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser-common.ini#85

So your test actually gets ran on automation.

----

Also, I wonder if this would make more sense as a property on tabs.Tab/windows.Window that can be read/changed using the usual tab/window methods rather than methods on chrome.sessions.

The property could be named "sessionData", and would be restored along with the tabs and windows. Maybe I'm misunderstanding this API though.

::: browser/components/extensions/ext-sessions.js
@@ +101,5 @@
> +        let tab = tabTracker.getTab(tabId);
> +        return Promise.resolve(SessionStore.getTabValue(tab, key));
> +      },
> +
> +      deleteTabValue: function(tabId, key) {

Most WE APIs seem to use "remove" instead of "delete". Can you use that ?

@@ +116,5 @@
> +        let window = windowTracker.getWindow(windowId, context);
> +        return Promise.resolve(SessionStore.getWindowValue(window, key));
> +      },
> +
> +      deleteWindowValue: function(windowId, key) {

same here.
(Assignee)

Comment 41

9 days ago
(In reply to Tim Nguyen :ntim from comment #40)
> Comment on attachment 8846505 [details] [diff] [review]
> API_sessions_window_tab_value_V5.diff
> 
> Also, I wonder if this would make more sense as a property on
> tabs.Tab/windows.Window that can be read/changed using the usual tab/window
> methods rather than methods on chrome.sessions.
> 
> The property could be named "sessionData", and would be restored along with
> the tabs and windows. Maybe I'm misunderstanding this API though.

The API relates to SessionStore methods and familiar as such to those who have been using them.  Currently SessionStore is not imported into `tabs` or `windows` API and would need to be.  This API relates to storing data that is preserved for session restore and not so much to do with handling and tracking tabs and windows throughout a session.  I was even wondering if we should include `set/get/deleteGlobalValue` in this API.

`sessions` seems to be the more natural choice to me.  This was the API Bob suggested in comment 14.

> Most WE APIs seem to use "remove" instead of "delete". Can you use that ?

Sure.  I used those names in keeping with the familiar SessionStore methods, but I can use "remove" instead.

Comment 42

9 days ago
(In reply to Kevin Jones from comment #41)
> (In reply to Tim Nguyen :ntim from comment #40)
> > Comment on attachment 8846505 [details] [diff] [review]
> > API_sessions_window_tab_value_V5.diff
> > 
> > Also, I wonder if this would make more sense as a property on
> > tabs.Tab/windows.Window that can be read/changed using the usual tab/window
> > methods rather than methods on chrome.sessions.
> > 
> > The property could be named "sessionData", and would be restored along with
> > the tabs and windows. Maybe I'm misunderstanding this API though.
> 
> The API relates to SessionStore methods and familiar as such to those who
> have been using them.  Currently SessionStore is not imported into `tabs` or
> `windows` API and would need to be.  This API relates to storing data that
> is preserved for session restore and not so much to do with handling and
> tracking tabs and windows throughout a session.  I was even wondering if we
> should include `set/get/deleteGlobalValue` in this API.
> 
> `sessions` seems to be the more natural choice to me.  This was the API Bob
> suggested in comment 14.

The reason why I mentioned this is that the data is associated to the tab/the window. So it would make more sense if this was a property of tabs.Tab/windows.Window. That property will get preserved for session restored.

So maybe a property named "savedSessionData" ?

Setting savedSessionData calls SessionStore.set{Window/Tab}Value
Getting savedSessionData calls SessionStore.get{Window/Tab}Value

I personally don't mind what we go with, but I think it makes more sense as a property of Tab/Window, since it's data associated with the tab/window.
(Assignee)

Comment 43

9 days ago
Ugh... I have a lot of work in this and would need to start over :-/  I can see the point in either paradigm, but if you feel strongly about it, I will change it.
(Assignee)

Comment 44

9 days ago
Created attachment 8847598 [details] [diff] [review]
API_sessions_window_tab_value_V6.diff

My updated patch was ready to upload so I went ahead and uploaded it.  If you would rather that I go with tabs/windows, let me know and I'll start on a new one.
Attachment #8846505 - Attachment is obsolete: true
Attachment #8846505 - Flags: feedback?(kmaglione+bmo)
Attachment #8846505 - Flags: feedback?(aswan)
Attachment #8847598 - Flags: review?(ntim.bugs)

Comment 45

9 days ago
Comment on attachment 8847598 [details] [diff] [review]
API_sessions_window_tab_value_V6.diff

Review of attachment 8847598 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, you might want to run ESLint (the javascript linter):

./mach eslint --setup

then

./mach eslint browser/components/extensions/ --fix


Someone from the WE team should review this.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +1,4 @@
> +"use strict";
> +
> +add_task(async function test_sessions_set_tab_value() {
> +  let background = function() {

You might want to add a call to info everytime you test something new.
info("Testing browser.sessions.setTabValue");
info("Testing browser.sessions.getTabValue");
...

@@ +1,5 @@
> +"use strict";
> +
> +add_task(async function test_sessions_set_tab_value() {
> +  let background = function() {
> +    browser.tabs.query({active: true}).then((tabs) => {

You could probably use an async function here.

let background = async function() {
  let [tab] = await browser.tabs.query({active: true});
  let currentTabId = tab.id;
  ...
}

@@ +21,5 @@
> +  await extension.startup();
> +
> +  let key = await extension.awaitMessage("tabkey");
> +  let value = SessionStore.getTabValue(gBrowser.selectedTab, key);
> +  is(value, "Tab Value", "Value for key '"+key+"' should be 'Tab Value'");

You could use a template string here:

`Value for key ${key} should be "Tab Value"`);

@@ +33,5 @@
> +      let promise = browser.sessions.getTabValue(currentTabId, key);
> +      promise.then(function(value) {
> +        browser.test.sendMessage("tabvalue", value);
> +      });
> +    });

Could be an async function too:

background = async function() {
  let [tab] = await browser.tabs.query({active: true});
  let currentTabId = tab.id;
  ...
  let tabValue = await browser.getTabValue(currentTabId, key);
  browser.test.sendMessage(...);
}

same comment applies below
Attachment #8847598 - Flags: review?(ntim.bugs) → review?(kmaglione+bmo)

Updated

9 days ago
Attachment #8847598 - Flags: review?(aswan)
Comment on attachment 8847598 [details] [diff] [review]
API_sessions_window_tab_value_V6.diff

Review of attachment 8847598 [details] [diff] [review]:
-----------------------------------------------------------------

Good start, but needs a bit of work.

::: browser/components/extensions/ext-sessions.js
@@ +93,5 @@
>        },
>  
> +      setTabValue: function(tabId, key, value) {
> +        let tab = tabTracker.getTab(tabId);
> +        SessionStore.setTabValue(tab, key, value);

We need these values to be specific to the extension, which means we either need to mangle the keys, or store all values in an object specific to the extension.

Also need to sanitize the value the way we do for the storage APIs.

::: browser/components/extensions/schemas/sessions.json
@@ +134,5 @@
> +        "async": true,
> +        "parameters": [
> +          {
> +            "type": "number",
> +            "name": "tabId",

This should be something like `"type": "integer", "minimum": 0`

Similar for "windowId", only `"minimum": -2`

@@ +143,5 @@
> +            "name": "key",
> +            "description": "The key which corresponds to the value being set."
> +          },
> +          {
> +            "type": "string",

We should allow arbitrary JSON-compatible data here.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +1,5 @@
> +"use strict";
> +
> +add_task(async function test_sessions_set_tab_value() {
> +  let background = function() {
> +    browser.tabs.query({active: true}).then((tabs) => {

`currentWindow: true, active: true`

@@ +70,5 @@
> +  await extension.startup();
> +
> +  key = await extension.awaitMessage("tabkey");
> +  value = SessionStore.getTabValue(gBrowser.selectedTab, key);
> +  is(value, "", "Value for key '"+key+"' should be an empty string");

Nit: Please use a template string.

@@ +97,5 @@
> +  await extension.startup();
> +
> +  let key = await extension.awaitMessage("winkey");
> +  let value = SessionStore.getWindowValue(window, key);
> +  is(value, "Window Value", "Value for key '"+key+"' should be 'Tab Value'");

Template string.

@@ +146,5 @@
> +  await extension.startup();
> +
> +  key = await extension.awaitMessage("winkey");
> +  value = SessionStore.getWindowValue(window, key);
> +  is(value, "", "Value for key '"+key+"' in window should be an empty string");

Template string.
Attachment #8847598 - Flags: review?(kmaglione+bmo)
Attachment #8847598 - Flags: review?(aswan)
Attachment #8847598 - Flags: review-
(Assignee)

Comment 47

6 days ago
(In reply to Kris Maglione [:kmag] from comment #46)
> Comment on attachment 8847598 [details] [diff] [review]
> API_sessions_window_tab_value_V6.diff
> 
> Review of attachment 8847598 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks, Kris.

> @@ +143,5 @@
> > +            "name": "key",
> > +            "description": "The key which corresponds to the value being set."
> > +          },
> > +          {
> > +            "type": "string",
> 
> We should allow arbitrary JSON-compatible data here.

To be clear, are you saying that we should be able to pass a JSON-able object here which will get stringified?  If so, use a parameter eg `isJSON`, or automatic detection?

And, if we do this here, then I assume we'll need to invoke a parse mechanism for getTab/WindowValue using a flag?
(Assignee)

Comment 48

6 days ago
(In reply to Kris Maglione [:kmag] from comment #46)
> ::: browser/components/extensions/ext-sessions.js
> @@ +93,5 @@
> >        },
> >  
> > +      setTabValue: function(tabId, key, value) {
> > +        let tab = tabTracker.getTab(tabId);
> > +        SessionStore.setTabValue(tab, key, value);
> 
> We need these values to be specific to the extension, which means we either
> need to mangle the keys, or store all values in an object specific to the
> extension.

Then are you okay with get from other extensions?
(In reply to Kevin Jones from comment #47)
> To be clear, are you saying that we should be able to pass a JSON-able
> object here which will get stringified?  If so, use a parameter eg `isJSON`,
> or automatic detection?
>
> And, if we do this here, then I assume we'll need to invoke a parse
> mechanism for getTab/WindowValue using a flag?

Yes, we should probably just store it as a JSON string, and then parse it when
the extension retrieves it.

(In reply to Kevin Jones from comment #48)
> > We need these values to be specific to the extension, which means we either
> > need to mangle the keys, or store all values in an object specific to the
> > extension.
>
> Then are you okay with get from other extensions?

I'd rather not, unless there's a particularly strong argument for doing so.
And definitely not unless the extension that writes the values explicitly asks
for them to be shareable.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 50

5 days ago
(In reply to Kris Maglione [:kmag] from comment #49)
> I'd rather not, unless there's a particularly strong argument for doing so.
> And definitely not unless the extension that writes the values explicitly
> asks
> for them to be shareable.

My argument would be the first part of comment 6.  I don't have a problem with allowing devs to choose if their data is shareable.
Flags: needinfo?(kmaglione+bmo)

Comment 51

5 days ago
We should perhaps implement chrome shared modules if we want to share data across add-ons: https://developer.chrome.com/extensions/shared_modules
(Assignee)

Comment 52

3 days ago
Created attachment 8849561 [details] [diff] [review]
API_sessions_window_tab_value_V8.diff

Updated with recommendations.  I used strict privacy in this patch, as at this point I don't know what Tree Style Tabs intends in a WebExtension.
Attachment #8843713 - Attachment is obsolete: true
Attachment #8847598 - Attachment is obsolete: true
Attachment #8849561 - Flags: review?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.