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

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[u462496]

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.

Comment 1

[Tomislav Jovanovic :zombie]

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?

Comment 2

[u462496]

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.

Comment 3

[Tomislav Jovanovic :zombie]

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

Comment 4

[u462496]

(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

[:shell escalante]

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.

Comment 6

[u462496]

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.

Comment 7

[u462496]

(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.

Comment 8

[Andy McKay]

Oops, sorry for the typo :)

Comment 10

[Bob Silverberg [:bsilverberg]]

Kevin, are you interested in trying to implement this yourself, with assistance from me?

Comment 11

[u462496]

Yes.

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #11)
> Yes.

Great, let me know if you need any help.

Comment 13

[u462496]

Bob, is there an existing API this should be a part of, or will it have its own API?

Comment 14

[Bob Silverberg [:bsilverberg]]

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!

Comment 15

[u462496]

Attachment: API_sessions_setgetWindowTabValue_V1.diff

Patch ext-sessions.js and sessions.json

Comment 16

[u462496]

Attachment: WE_sessions_git.xpi

Addon to test Attachment #8843712 [details] [diff]

Comment 17

[Bob Silverberg [:bsilverberg]]

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.

Comment 18

[u462496]

(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.

Comment 19

[Bob Silverberg [:bsilverberg]]

(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`.

Comment 20

[u462496]

Attachment: API_sessions_setgetWindowTabValue_V2.diff

Update per comment 17

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8844605 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V2.diff

Looks good Kevin, nice work. Now it just needs some tests.

Comment 22

[u462496]

(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

Comment 23

[Bob Silverberg [:bsilverberg]]

(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?

Comment 24

[u462496]

(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.

Comment 25

[u462496]

Attachment: browser_ext_sessions_setgetTabValue_setgetWindowValue.js

Mochitest

Comment 26

[u462496]

Attachment: browser_ext_sessions_setgetTabValue_setgetWindowValue.js V2

Mochitest - Fixed spacing problem

Comment 27

[u462496]

Attachment: browser_ext_sessions_setgetTabValue_setgetWindowValue.js V3

Ugh!! Corrected typo in test description.

Sorry for all the noise :-/

Comment 28

[u462496]

I realized this should have deleteTabValue and deleteWindowValue as well.

Comment 29

[u462496]

Attachment: browser_ext_sessions set/get/delete tab/window value

Updated test, includes delete.

Comment 30

[u462496]

Attachment: API_sessions set/get/delete window/tab value patch V1

Updated patch adds delete.

Comment 31

[u462496]

Attachment: browser_ext_sessions set/get/delete tab/window value test

Updated test, includes delete.

Comment 32

[Bob Silverberg [:bsilverberg]]

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?

Comment 33

[Bob Silverberg [:bsilverberg]]

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.

Comment 34

[u462496]

(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.

Comment 35

[Bob Silverberg [:bsilverberg]]

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?

Comment 36

[Croydon]

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.

Comment 37

[u462496]

Attachment: API_sessions_window_tab_value_V5.diff

Comment 38

[u462496]

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.

Comment 39

[Andrew Swan [:aswan]]

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"

Comment 40

[Tim Nguyen :ntim]

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.

Comment 41

[u462496]

(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

[Tim Nguyen :ntim]

(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.

Comment 43

[u462496]

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.

Comment 44

[u462496]

Attachment: 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.

Comment 45

[Tim Nguyen :ntim]

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

Comment 46

[Kris Maglione [:kmag]]

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.

Comment 47

[u462496]

(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?

Comment 48

[u462496]

(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?

Comment 49

[Kris Maglione [:kmag]]

(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.

Comment 50

[u462496]

(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.

Comment 51

[Tim Nguyen :ntim]

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

Comment 52

[u462496]

Attachment: 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.

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8849561 [details] [diff] [review]
API_sessions_window_tab_value_V8.diff

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

Sorry for the delay. We were all traveling for meetings last week.

This looks good, thanks! I mainly just have some concerns about the tests, and I wonder if we should make this API behave a bit more like the browser.storage APIs.

::: browser/components/extensions/ext-sessions.js
@@ +63,5 @@
>  
> +function enforceNoTemporaryAddon(extensionId) {
> +  const EXCEPTION_MESSAGE =
> +        "sessions API storage methods will not work with a temporary addon ID. " +
> +        "Please add an explicit addon ID to your manifest. ";

No need for the constant. Also, please capitalize the first word and remove the last trailing space.

@@ +109,5 @@
>        },
>  
> +      setTabValue: function(tabId, key, value) {
> +        let tab = tabTracker.getTab(tabId);
> +        // Throw if using a temporary id.

Nit: No need for the comment before every invocation. The function name is pretty self-explanatory, so just add a comment to the function itself.

@@ +110,5 @@
>  
> +      setTabValue: function(tabId, key, value) {
> +        let tab = tabTracker.getTab(tabId);
> +        // Throw if using a temporary id.
> +        enforceNoTemporaryAddon(extension.id);

This should probably be the first thing we do in any of these functions. I wonder if we shouldn't just enforce this at the binding level, and just never inject the functions for temporary add-ons... But I suppose the error message this way is more helpful.

@@ +112,5 @@
> +        let tab = tabTracker.getTab(tabId);
> +        // Throw if using a temporary id.
> +        enforceNoTemporaryAddon(extension.id);
> +        if (typeof value == "object") {
> +          value = ExtensionStorage.sanitizeToJsonString(value, context);

Let's just always JSON encode. This is going to lead to weird behavior if someone stores a JSON-compatible string, and we wind up parsing it rather than returning the original string later on.

@@ +115,5 @@
> +        if (typeof value == "object") {
> +          value = ExtensionStorage.sanitizeToJsonString(value, context);
> +        }
> +
> +        let fullKey = extension.id + "_" + key;

Nit: Please use template strings rather than string concatenation. Also, maybe something like `extension:${extension.id}:${key}`

And please make this a helper function, rather than generating the key everywhere we use it.

@@ +129,5 @@
> +        let value = SessionStore.getTabValue(tab, fullKey);
> +        // If value is JSON parse-able, assume the original value was
> +        // JSON-compatible data.
> +        try {
> +          value = JSON.parse(value);

Let's just resolve with `JSON.parse(value)` and let the exception propagate if it fails. We shouldn't wind up with any non-JSON data here.

@@ +149,5 @@
> +        let window = windowTracker.getWindow(windowId, context);
> +         // Throw if using a temporary id.
> +        enforceNoTemporaryAddon(extension.id);
> +        if (typeof value == "object") {
> +          value = ExtensionStorage.sanitizeToJsonString(value, context);

Same here: Please just always serialize.

@@ +166,5 @@
> +        let value = SessionStore.getWindowValue(window, fullKey);
> +        // If value is JSON parse-able, assume the original value was
> +        // JSON-compatible data.
> +        try {
> +          value = JSON.parse(value);

And assume the value is valid JSON here, if it exists.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +10,5 @@
> +    browser.tabs.query({currentWindow: true, active: true}).then((tabs) => {
> +      let currentTabId = tabs[0].id;
> +      let key = "tabkey";
> +      let value = "Tab Value";
> +      browser.sessions.setTabValue(currentTabId, key, value);

Hm. I wonder if we shouldn't behave the same as the storage API here, and accept objects with multiple key/value pairs?

@@ +12,5 @@
> +      let key = "tabkey";
> +      let value = "Tab Value";
> +      browser.sessions.setTabValue(currentTabId, key, value);
> +      browser.test.sendMessage("tabkey", key);
> +    });

Please also test numbers, strings that are valid JSON, non-JSONable values like functions...

And no need to test all of these value types from different tasks. Just iterate over a list of keys and values, set them, retrieve them, and make sure they're the same.

@@ +46,5 @@
> +      promise.then(function(value) {
> +        browser.test.sendMessage("tabvalue", value);
> +      });
> +    });
> +  }

No need to create multiple extension instances, or explicitly set sessionstore values. Please just test that setting and then retrieving works as expected.

And it's generally easier to do as much from withing the background script as possible, rather than sending messages back and forth.

@@ +130,5 @@
> +  is(value, "", `Value for key '${key}' should be an empty string.`);
> +
> +  await extension.unload();
> +});
> +

We should also test that:

- Values are different between different tabs.
- Values persist when tabs are duplicated.
- Values persist when tabs are removed and then restored.
- Values persist when tabs are moved between windows.

Comment 54

[u462496]

(In reply to Kris Maglione [:kmag] from comment #53)
> Comment on attachment 8849561 [details] [diff] [review]
> API_sessions_window_tab_value_V8.diff
> @@ +129,5 @@
> > +        let value = SessionStore.getTabValue(tab, fullKey);
> > +        // If value is JSON parse-able, assume the original value was
> > +        // JSON-compatible data.
> > +        try {
> > +          value = JSON.parse(value);
> 
> Let's just resolve with `JSON.parse(value)` and let the exception propagate
> if it fails. We shouldn't wind up with any non-JSON data here.

If getTabValue is called on a key which doesn't exist, it returns an empty string and JSON.parse() will fail.  I suppose we need to handle this as a special case and show a message that the key doesn't exist?

Also, if JSON fails in getTabValue, the promise won't resolve, and the caller will be left hanging.  Should we do something like:

try {
  value = JSON.parse(value);
} catch(e) {
  console.error(`${e}\n${e.stack}`);
}

Comment 55

[u462496]

Test for this bug depends on bug 1359806.

Comment 56

[u462496]

(In reply to Kris Maglione [:kmag] from comment #53)
> Comment on attachment 8849561 [details] [diff] [review]
> API_sessions_window_tab_value_V8.diff
> We should also test that:
> 
> ...
> - Values persist when tabs are moved between windows.

This is not supported by the SessionStore APIs.

Comment 57

[u462496]

Attachment: API_sessions_window_tab_value_rebased_P2_V1.diff

Updated per requests.  Does not include however test for values persist when tabs are moved between windows, as this is not support by SessionStore.

Comment 58

[u462496]

Attachment: API_sessions_window_tab_value_rebased_P2_V2.diff

oops, the test didn't make it on the last one...

Comment 59

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8863391 [details] [diff] [review]
API_sessions_window_tab_value_rebased_P2_V2.diff

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

Just a quick drive-by as my https://addons.mozilla.org/en-US/firefox/addon/pinned-window-extension-for/ addon really want this API too, so thought I'd see how close it is to landing - happy to see it this advanced!

::: browser/components/extensions/ext-sessions.js
@@ +63,5 @@
> +    throw new ExtensionError(message);
> +  }
> +}
> +
> +function formatExtensionKey(extensionId, key) {

you could consider combining the check above into this function.

Comment 60

[Mark Hammond [:markh] [:mhammond]]

Thinking more about this, I'm not convinced this is the correct API from the POV of SessionStore - it doesn't really want to carry this stuff around for uninstalled addons etc. IMO, a better solution would be a persistent window ID carried by SessionStore, and force each addon to use a storage API to track their own state. ISTM it could be hooked into window.sessionId - that's currently undefined for open windows.

That said though, I, and I assume Kevin, would really prefer not to wait another cycle before our addons can work, so feel free to ignore I said this ;)

Comment 61

[u462496]

(In reply to Mark Hammond [:markh] from comment #60)

> That said though, I, and I assume Kevin, would really prefer not to wait
> another cycle before our addons can work, so feel free to ignore I said this
> ;)

And Kevin really doesn't want to trash a whole lotta work and start over...

Comment 62

[Mark Hammond [:markh] [:mhammond]]

SessionStore also has a promiseInitialized promise that should be resolved before interacting with its API. It's not clear to me if this isn't strictly necessary in a webext world or whether the existing use of session store in ext-sessions is hiding a latent bug, but I thought it worth mentioning.

Comment 63

[Bob Silverberg [:bsilverberg]]

(In reply to Mark Hammond [:markh] from comment #62)
> SessionStore also has a promiseInitialized promise that should be resolved
> before interacting with its API. It's not clear to me if this isn't strictly
> necessary in a webext world or whether the existing use of session store in
> ext-sessions is hiding a latent bug, but I thought it worth mentioning.

I assume that by the time any extensions are loaded the SessionStore has been initialized, but I could be wrong about that. Mike, can you comment on this?

Comment 64

[Bob Silverberg [:bsilverberg]]

Sorry for the delay in reviewing the latest version of this patch. Kris has been really busy so I am going to take over the initial review of this. Kevin, can you please remove Kris and flag me for review?

Comment 65

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #64)
> Sorry for the delay in reviewing the latest version of this patch. Kris has
> been really busy so I am going to take over the initial review of this.
> Kevin, can you please remove Kris and flag me for review?

Thank you, Bob.  I'm sure everyone is pretty swamped right now.

Comment 66

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8863391 [details] [diff] [review]
API_sessions_window_tab_value_rebased_P2_V2.diff

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

Nice work Kevin. Everything is looking pretty good. As I mentioned in my comments for the tests, you should run eslint on all of the files you have touched because there are a bunch of issues, but it's a very nice start on the tests.

::: browser/components/extensions/ext-sessions.js
@@ +63,5 @@
> +    throw new ExtensionError(message);
> +  }
> +}
> +
> +function formatExtensionKey(extensionId, key) {

+1 to Mark's suggestion. That would eliminate a number of function calls.

@@ +142,5 @@
> +          enforceNoTemporaryAddon(extension.id);
> +
> +          let tab = tabTracker.getTab(tabId);
> +          value = JSON.stringify(value);
> +          key = formatExtensionKey(extension.id, key);

I think Kris suggested this before, but what do you think of, instead of storing multiple keys for an extension, we just store a single object which contains all of the keys that the extension has set? I suppose one issue with that is that we'd need to retrieve the object each time we are setting anything, but I'm not sure how much overhead that would be.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +1,1 @@
> +"use strict";

There are *a lot* of eslint issues with this file. Please check it in some way (mach eslint is a good one) and fix the eslint issues.

@@ +2,5 @@
> +
> +add_task(async function test_sessions_tab_value() {
> +  info("Testing set/get/deleteTabValue.");
> +
> +  let background = function() {

We tend to use the syntax `function background()` as opposed to `let background = function()`.

@@ +8,5 @@
> +                 { key: "tabkey2", value: 25 },
> +                 { key: "tabkey3", value: { val: "Tab Value" } },
> +                 { key: "tabkey4", value: function() { let i = 3; } }];
> +
> +    let runNextTest = function() {

As above: `function runNextTest()`.

@@ +16,5 @@
> +        browser.test.sendMessage("testComplete");
> +      }
> +    }
> +
> +    let test = async function(params) {

As above: `function test(params)`.

@@ +22,5 @@
> +      let tabPos = 0;
> +      let tabs = await browser.tabs.query({currentWindow: true, active: true});
> +      let currentTabId = tabs[tabPos].id;
> +
> +      browser.sessions.setTabValue(currentTabId, key, value);

await browser.sessions.setTabValue

@@ +30,5 @@
> +
> +      browser.test.log(`Test that setting, getting and deleting tab value behaves properly when value is type "${valueType}"`);
> +
> +      if (valueType == "string") {
> +        browser.test.assertEq(value, testValue1, `Value for key '${key}' should be '${value}'.`);

We tend to word assertion messages as if we are stating the expected condition. In this case it would be something like `Expected value returned for key: ${key}.` Note that `assertEq`  automatically shows us both the expected and actual values, so it's not necessary to output either of them in the assertion message. Also note that we tend to avoid using single quotes in general.

This comment applies to all of the assertion messages in the test.

@@ +40,5 @@
> +        let innerVal1 = value.val;
> +        let innerVal2 = testValue1.val;
> +        browser.test.assertEq(innerVal1, innerVal2, `Value for key '${key}' should be '${innerVal1}'.`);
> +      } else if (valueType == "function") {
> +        browser.test.assertEq(null, testValue1, `Value for key '${key}' is non-JSON-able and should be 'null'.`);

Should this actually throw an exception? It seems like it would be beneficial for an extension to know it was unsuccessful at setting a value.

@@ +44,5 @@
> +        browser.test.assertEq(null, testValue1, `Value for key '${key}' is non-JSON-able and should be 'null'.`);
> +      }
> +
> +      // Remove the tab key/value.
> +      browser.sessions.removeTabValue(currentTabId, key);

await browser.sessions.removeTabValue

@@ +52,5 @@
> +        let testValue2 = await browser.sessions.getTabValue(currentTabId, key);
> +        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
> +        runNextTest();
> +      } catch(e) {
> +        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);

It might be better to check that the error is what we expect, rather than just passing the test for any exception.

@@ +53,5 @@
> +        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
> +        runNextTest();
> +      } catch(e) {
> +        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);
> +        runNextTest();

Do you need both this `runNextTest` and the one on line 61?

@@ +75,5 @@
> +
> +  await extension.startup();
> +
> +  await extension.awaitMessage("testComplete");
> +  ok(true, "Testing completed for set/get/deleteTabValue.");

This is unnecessary.

@@ +83,5 @@
> +
> +add_task(async function test_sessions_tab_value_persistence() {
> +  info("Testing for persistence of set tab values.");
> +
> +  let background = function() {

function background()

@@ +84,5 @@
> +add_task(async function test_sessions_tab_value_persistence() {
> +  info("Testing for persistence of set tab values.");
> +
> +  let background = function() {
> +    let runTest = async function() {

async function runTest()

@@ +96,5 @@
> +
> +      // newTab will get removed in order to test sessions.restore. Wait until
> +      // newTab load has completed or it will still have about:blank url when it
> +      // gets removed and will not appear in the removed tabs history.
> +      browser.webNavigation.onCompleted.addListener(async function newTabListener(details) {

It seems like this listener should be added *before* the call to `browser.tabs.create` (which is currently on line 95).

@@ +104,5 @@
> +
> +        let tabId_1 = tabs[0].id;
> +        let tabId_2 = tabs[1].id;
> +
> +        browser.sessions.setTabValue(tabId_1, key, value1);

await

@@ +105,5 @@
> +        let tabId_1 = tabs[0].id;
> +        let tabId_2 = tabs[1].id;
> +
> +        browser.sessions.setTabValue(tabId_1, key, value1);
> +        browser.sessions.setTabValue(tabId_2, key, value2);

await

@@ +112,5 @@
> +        let testValue2 = await browser.sessions.getTabValue(tabId_2, key);
> +
> +        browser.test.assertEq(value1, testValue1, `Value for key '${key}' should be '${value1}'.`);
> +        browser.test.assertEq(value2, testValue2, `Value for key '${key}' should be '${value2}'.`);
> +        browser.test.assertTrue((testValue1 != testValue2), `Value for key '${key}' in tabs 1 and 2 should not be the same.`);

Haven't you already proved this with the previous two assertions?

@@ +117,5 @@
> +
> +        browser.test.log("Test that value is copied to duplicated tab for a given key.");
> +
> +        let duptab = await browser.tabs.duplicate(tabId_2);
> +        tabId_3 = duptab.id;

let (although eslint will catch this too)

@@ +125,5 @@
> +        browser.test.assertEq(value2, testValue3, `Value for key '${key}' should be '${value1}'.`);
> +
> +        browser.test.log("Test that restored tab still holds the value for a given key.");
> +
> +        await browser.tabs.remove([tabId_3]);

At lines 97 - 99 you describe how you're adding a listener so that the tab won't get removed too early, but then you don't actually remove *that* tab in the test, you remove the one that you just duplicated. So does that make the listener obsolete? Or should the listener be on a different tab? Or should you be removing a different tab?

@@ +130,5 @@
> +
> +        let sessions = await browser.sessions.getRecentlyClosed({ maxResults: 1 });
> +
> +        let sessionData = await browser.sessions.restore(sessions[0].tab.sessionId);
> +        let restoredTab = sessionData.tab;

You don't really need this interim `restoredTab`.

@@ +147,5 @@
> +
> +      }, {url: [ { hostContains: "example.com" } ] });
> +    }
> +
> +    runTest();

I don't think this runTest() serves any purpose. You can just get rid of it and keep the code in the background function.

@@ +165,5 @@
> +
> +  await extension.startup();
> +
> +  await extension.awaitMessage("testComplete");
> +  ok(true, "Testing completed for persistance of set tab values.");

This is unnecessary.

@@ +170,5 @@
> +
> +  await extension.unload();
> +});
> +
> +add_task(async function test_sessions_window_value() {

It looks like you could combine this test with test_sessions_tab_value as they are doing essentially the same thing, only one is interacting with tabs and the other is interacting with windows. Abstracting the single test so it could work with both might make it a bit ugly, but it would save a lot of code. I suggest you give it a try and see if you like the results.

If you choose to keep it as a separate test, most of the comments I made on test_sessions_tab_value also apply here.

@@ +250,5 @@
> +
> +  await extension.unload();
> +});
> +
> +add_task(async function test_sessions_window_value_persistence() {

As above, most of the comments I made about test_sessions_tab_value_persistence apply here as well.

@@ +276,5 @@
> +      browser.test.assertEq(value2, testValue2, `Value for key '${key}' should be '${value2}'.`);
> +      browser.test.assertTrue((testValue1 != testValue2), `Value for key '${key}' in windows 1 and 2 should not be the same.`);
> +
> +      await browser.windows.remove(window2Id);
> +      browser.test.sendMessage("testComplete");

Should there be a test for restoring a window and checking the value, as there was for restoring a tab?

Comment 67

[Mike de Boer [:mikedeboer]]

(In reply to Bob Silverberg [:bsilverberg] from comment #63)
> I assume that by the time any extensions are loaded the SessionStore has
> been initialized, but I could be wrong about that. Mike, can you comment on
> this?

I'm guessing a bit at this point, but I think extensions are initialized right after delayedStartup of browser, so it would indeed be appropriate to wait for `SessionStore.promiseInitialized` to be resolved!
As a side-note, however; it's _highly_ unlikely for this to race and to ever encounter an uninitialized SessionStore component when using the API. The tests hide this race, however, because we make sure the promise is resolved _before_ running them.
In other words: I think Mark brings up a valid point. However, I do think it's better to add an `await SessionStore.promiseInitialized;` or `SessionStore.promiseInitialized.then(() => {});` at a more strategic, generalized position than this specific API entry point only.

Comment 68

[Bob Silverberg [:bsilverberg]]

Thanks Mike. I have opened bug 1365562 as a follow up.

Comment 69

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #66)
> Comment on attachment 8863391 [details] [diff] [review]
> API_sessions_window_tab_value_rebased_P2_V2.diff
> 
> Review of attachment 8863391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work Kevin. Everything is looking pretty good. As I mentioned in my
> comments for the tests, you should run eslint on all of the files you have
> touched because there are a bunch of issues, but it's a very nice start on
> the tests.

When running mach eslint, most of the errors are:

"There should be no space after/before '{/}'"

Weird, because spaces after/before '{/}' pass with the linter on tryserver and I have landed several patches using this construct.  In fact from looking at Firefox code, this actually seems to be preferred.  And if my memory serves me correctly, I have been doing it this way because a reviewer once asked me to.

:-/

Comment 70

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #66)
> Comment on attachment 8863391 [details] [diff] [review]
> API_sessions_window_tab_value_rebased_P2_V2.diff
> 
> Review of attachment 8863391 [details] [diff] [review]:
> -----------------------------------------------------------------
> await browser.sessions.setTabValue
> 
> await browser.sessions.removeTabValue

What are we await-ing here?  These are synchronous.

Comment 71

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #69)
> 
> When running mach eslint, most of the errors are:
> 
> "There should be no space after/before '{/}'"
> 
> Weird, because spaces after/before '{/}' pass with the linter on tryserver
> and I have landed several patches using this construct.  In fact from
> looking at Firefox code, this actually seems to be preferred.  And if my
> memory serves me correctly, I have been doing it this way because a reviewer
> once asked me to.
> 
> :-/

Could it be that your eslint config is not up to date? Try running mach eslint --setup and then check again with mach eslint.

Comment 72

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #70)
> (In reply to Bob Silverberg [:bsilverberg] from comment #66)
> > Comment on attachment 8863391 [details] [diff] [review]
> > API_sessions_window_tab_value_rebased_P2_V2.diff
> > 
> > Review of attachment 8863391 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > await browser.sessions.setTabValue
> > 
> > await browser.sessions.removeTabValue
> 
> What are we await-ing here?  These are synchronous.

But the API calls are asynchronous, right? They're marked as async: true. I believe this means we need to await them, even if their internal implementation is synchronous. There are other things that can be going on with the API framework that we need to await, and, as Luca pointed out to me, the API code is running in the main process but the extension code is (actually will be) running in the extension process, so the await would be awaiting the outcome of running the code in the main process. Also, it's possible the internal implementation of the API could change to become asynchronous at some point, so we should not assume that an asynchronous API method can be treated as synchronous.

Does that make sense?

Comment 73

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #72)
> (In reply to Kevin Jones from comment #70)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #66)
> But the API calls are asynchronous, right? They're marked as async: true. I
> believe this means we need to await them, even if their internal
> implementation is synchronous. There are other things that can be going on
> with the API framework that we need to await, and, as Luca pointed out to
> me, the API code is running in the main process but the extension code is
> (actually will be) running in the extension process, so the await would be
> awaiting the outcome of running the code in the main process. Also, it's
> possible the internal implementation of the API could change to become
> asynchronous at some point, so we should not assume that an asynchronous API
> method can be treated as synchronous.
> 
> Does that make sense?

Well, sort of.

The fact that when I throw error for null value in browser.sessions.setTabValue, and it doesn't get caught in a try/catch block in the extension seems to support what you are saying.

Comment 74

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #66)
> I think Kris suggested this before, but what do you think of, instead of
> storing multiple keys for an extension, we just store a single object which
> contains all of the keys that the extension has set? I suppose one issue
> with that is that we'd need to retrieve the object each time we are setting
> anything, but I'm not sure how much overhead that would be.

About the only advantage I can think of maintaining a single namespace for each addonId is simply from an organizational standpoint.  And, like you say, it would require a bit more crunching (and code) having to delve into the object not just for setting, but for all operations.  Also, setting would require parsing the addonId object, and for type object values, validating as serialize-able before adding them to the addonId object, serialize it all back again, etc.

But if you feel we should go that way, I am open to it.

> > +        let testValue2 = await browser.sessions.getTabValue(currentTabId, key);
> > +        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
> > +        runNextTest();
> > +      } catch(e) {
> > +        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);
> 
> It might be better to check that the error is what we expect, rather than
> just passing the test for any exception.

Unfortunately, the error reported from the try/catch block in the extension is the ambiguous "An unexpected error occurred" :-/

> > +        let innerVal1 = value.val;
> > +        let innerVal2 = testValue1.val;
> > +        browser.test.assertEq(innerVal1, innerVal2, `Value for key '${key}' should be '${innerVal1}'.`);
> > +      } else if (valueType == "function") {
> > +        browser.test.assertEq(null, testValue1, `Value for key '${key}' is non-JSON-able and should be 'null'.`);
> 
> Should this actually throw an exception? It seems like it would be
> beneficial for an extension to know it was unsuccessful at setting a value.

Note that I have to return a promise in browser.sessions.setTabValue in order for the try/catch block in the extension (test) to catch the error (comment 73), but maybe that is expected due to what you describe in comment 72.

browser.sessions.setTabValue:

setTabValue(tabId, key, value) {
  // Validate value
  if (!value) {
    throw new Error("Browser.sessions.setTabValue: `value` is invalid");
  }
  ...
  return new Promise();
}

And we have a similar situation to getTabValue, the error message as reported from the extension is the ambiguous "An unexpected error occurred".

extension:

try {
  await browser.sessions.setTabValue(tabId, key, value)
} catch(e) {
  // `e` here reports "An unexpected error occurred"
}

> It looks like you could combine this test with test_sessions_tab_value as
> they are doing essentially the same thing, only one is interacting with tabs
> and the other is interacting with windows. Abstracting the single test so it
> could work with both might make it a bit ugly, but it would save a lot of
> code. I suggest you give it a try and see if you like the results.
> 

Well, I was following Kris' suggestion, or at least as what I interpreted as Kris' suggestion.

Comment 76

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #74)
> (In reply to Bob Silverberg [:bsilverberg] from comment #66)
> > I think Kris suggested this before, but what do you think of, instead of
> > storing multiple keys for an extension, we just store a single object which
> > contains all of the keys that the extension has set? I suppose one issue
> > with that is that we'd need to retrieve the object each time we are setting
> > anything, but I'm not sure how much overhead that would be.
> 
> About the only advantage I can think of maintaining a single namespace for
> each addonId is simply from an organizational standpoint.  And, like you
> say, it would require a bit more crunching (and code) having to delve into
> the object not just for setting, but for all operations.  Also, setting
> would require parsing the addonId object, and for type object values,
> validating as serialize-able before adding them to the addonId object,
> serialize it all back again, etc.
> 
> But if you feel we should go that way, I am open to it.
> 

No, I think it's fine to keep it the way it is.

> > > +        let testValue2 = await browser.sessions.getTabValue(currentTabId, key);
> > > +        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
> > > +        runNextTest();
> > > +      } catch(e) {
> > > +        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);
> > 
> > It might be better to check that the error is what we expect, rather than
> > just passing the test for any exception.
> 
> Unfortunately, the error reported from the try/catch block in the extension
> is the ambiguous "An unexpected error occurred" :-/
>

When an exception is encountered during execution of an API method and it is not handled in any way, we end up with this "An unexpected error occurred", so we really should handle any exceptions we can anticipate ourselves, passing along an appropriate message. For example, in the case of getTabValue, there are a couple of options:

1. We can anticipate that sometimes the value returned from SessionStore.getTabValue will be empty, so we can check it and throw our own exception, e.g.:

          let value = SessionStore.getTabValue(tab, key);
          if (value) {
            return Promise.resolve(JSON.parse(value));
          }
          throw new ExtensionError(`No value was found for the key: ${key}.`);

2. We also have the option of rethrowing an exception when we're not sure why one might occur, like so:
 
          try {
            let value = JSON.parse(SessionStore.getTabValue(tab, key));
            return Promise.resolve(value);
          } catch(e) {
            throw new ExtensionError(e.message);
          }

But we generally try to avoid that as we don't want to expose the user to internal exception messages. 

I think if you go back to your API code and make some changes along the lines of option 1 above you'll find you can eliminate the 
"An unexpected error occurred" messages.

> > > +        let innerVal1 = value.val;
> > > +        let innerVal2 = testValue1.val;
> > > +        browser.test.assertEq(innerVal1, innerVal2, `Value for key '${key}' should be '${innerVal1}'.`);
> > > +      } else if (valueType == "function") {
> > > +        browser.test.assertEq(null, testValue1, `Value for key '${key}' is non-JSON-able and should be 'null'.`);
> > 
> > Should this actually throw an exception? It seems like it would be
> > beneficial for an extension to know it was unsuccessful at setting a value.
> 
> Note that I have to return a promise in browser.sessions.setTabValue in
> order for the try/catch block in the extension (test) to catch the error
> (comment 73), but maybe that is expected due to what you describe in comment
> 72.
> 
> browser.sessions.setTabValue:
> 
> setTabValue(tabId, key, value) {
>   // Validate value
>   if (!value) {
>     throw new Error("Browser.sessions.setTabValue: `value` is invalid");
>   }
>   ...
>   return new Promise();
> }
> 
> And we have a similar situation to getTabValue, the error message as
> reported from the extension is the ambiguous "An unexpected error occurred".
> 
> extension:
> 
> try {
>   await browser.sessions.setTabValue(tabId, key, value)
> } catch(e) {
>   // `e` here reports "An unexpected error occurred"
> }
>

I think I've addressed all of these with my comment above. Please let me know if I have not.
 
> > It looks like you could combine this test with test_sessions_tab_value as
> > they are doing essentially the same thing, only one is interacting with tabs
> > and the other is interacting with windows. Abstracting the single test so it
> > could work with both might make it a bit ugly, but it would save a lot of
> > code. I suggest you give it a try and see if you like the results.
> > 
> 
> Well, I was following Kris' suggestion, or at least as what I interpreted as
> Kris' suggestion.

I'm not sure which of Kris' suggestions you are referring to. If you don't think this change would benefit the code that's fine, I can pass the review up the queue to a peer as the test currently stands.

Comment 77

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #76)
> > > It looks like you could combine this test with test_sessions_tab_value as
> > > they are doing essentially the same thing, only one is interacting with tabs
> > > and the other is interacting with windows. Abstracting the single test so it
> > > could work with both might make it a bit ugly, but it would save a lot of
> > > code. I suggest you give it a try and see if you like the results.
> > > 
> > 
> > Well, I was following Kris' suggestion, or at least as what I interpreted as
> > Kris' suggestion.
> 
> I'm not sure which of Kris' suggestions you are referring to. If you don't
> think this change would benefit the code that's fine, I can pass the review
> up the queue to a peer as the test currently stands.

Forgive me, I am not trying to make waves here.  This has passed through 3 different reviewers, and sometimes it seems that each one has a little different view about how things should be done and it does seem frustrating at times when I have spent time trying to get things how I think one would want them.

Nevertheless I am happy to give your suggestion a try.

Comment 78

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #77)
> (In reply to Bob Silverberg [:bsilverberg] from comment #76)
> > > > It looks like you could combine this test with test_sessions_tab_value as
> > > > they are doing essentially the same thing, only one is interacting with tabs
> > > > and the other is interacting with windows. Abstracting the single test so it
> > > > could work with both might make it a bit ugly, but it would save a lot of
> > > > code. I suggest you give it a try and see if you like the results.
> > > > 
> > > 
> > > Well, I was following Kris' suggestion, or at least as what I interpreted as
> > > Kris' suggestion.
> > 
> > I'm not sure which of Kris' suggestions you are referring to. If you don't
> > think this change would benefit the code that's fine, I can pass the review
> > up the queue to a peer as the test currently stands.
> 
> Forgive me, I am not trying to make waves here.  This has passed through 3
> different reviewers, and sometimes it seems that each one has a little
> different view about how things should be done and it does seem frustrating
> at times when I have spent time trying to get things how I think one would
> want them.
> 
> Nevertheless I am happy to give your suggestion a try.

I understand and I apologize. I was the original reviewer and maybe it would have been better to just put things on hold while I was on vacation rather than hand it over to someone else. It will have to go through a final review by someone else anyway as I am not a peer.

I also wasn't trying to be snarky. I am totally happy with you just keeping it as is, if you think that's best. I won't be bothered if you choose not to change it.

Comment 79

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #78)
> (In reply to Kevin Jones from comment #77)
> > Forgive me, I am not trying to make waves here.  This has passed through 3
> > different reviewers, and sometimes it seems that each one has a little
> > different view about how things should be done and it does seem frustrating
> > at times when I have spent time trying to get things how I think one would
> > want them.
> > 
> > Nevertheless I am happy to give your suggestion a try.
> 
> I understand and I apologize. I was the original reviewer and maybe it would
> have been better to just put things on hold while I was on vacation rather
> than hand it over to someone else. It will have to go through a final review
> by someone else anyway as I am not a peer.
> 
> I also wasn't trying to be snarky. I am totally happy with you just keeping
> it as is, if you think that's best. I won't be bothered if you choose not to
> change it.

Thank you Bob for explaining it more clearly.  I appreciate that everyone is doing their best to keep things of good quality.

Comment 80

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #76)
> (In reply to Kevin Jones from comment #74)
> > > > +        let testValue2 = await browser.sessions.getTabValue(currentTabId, key);
> > > > +        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
> > > > +        runNextTest();
> > > > +      } catch(e) {
> > > > +        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);
> > > 
> > > It might be better to check that the error is what we expect, rather than
> > > just passing the test for any exception.
> > 
> > Unfortunately, the error reported from the try/catch block in the extension
> > is the ambiguous "An unexpected error occurred" :-/
> >
> 
> When an exception is encountered during execution of an API method and it is
> not handled in any way, we end up with this "An unexpected error occurred",
> so we really should handle any exceptions we can anticipate ourselves,
> passing along an appropriate message. For example, in the case of
> getTabValue, there are a couple of options:
> 
> 1. We can anticipate that sometimes the value returned from
> SessionStore.getTabValue will be empty, so we can check it and throw our own
> exception, e.g.:
> 
>           let value = SessionStore.getTabValue(tab, key);
>           if (value) {
>             return Promise.resolve(JSON.parse(value));
>           }
>           throw new ExtensionError(`No value was found for the key:
> ${key}.`);
> 
> 2. We also have the option of rethrowing an exception when we're not sure
> why one might occur, like so:
>  
>           try {
>             let value = JSON.parse(SessionStore.getTabValue(tab, key));
>             return Promise.resolve(value);
>           } catch(e) {
>             throw new ExtensionError(e.message);
>           }
> 
> But we generally try to avoid that as we don't want to expose the user to
> internal exception messages. 
> 
> I think if you go back to your API code and make some changes along the
> lines of option 1 above you'll find you can eliminate the 
> "An unexpected error occurred" messages.

Unfortunately, explicitly throwing error in browser.sessions.setTabState as you suggested, the error caught in the WE try/catch block is still "An unexpected error occurred".

What is interesting though, is that the proper message gets sent to the console:

21 INFO Console message: [JavaScript Error: "browser.sessions.setTabValue: Value is invalid" {file: "chrome://browser/content/ext-sessions.js" line: 140}]

Comment 81

[alexmehler]

im seconding this !   one of my addons gives tabs a different background color when you wanna mark them. 

i just save a flag with sessionstore to the tab 

and on session/tab restore the addon looks up the flag and colors the tab accordingly .

Comment 82

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #80)

> Unfortunately, explicitly throwing error in browser.sessions.setTabState as
> you suggested, the error caught in the WE try/catch block is still "An
> unexpected error occurred".
> 
> What is interesting though, is that the proper message gets sent to the
> console:
> 
> 21 INFO Console message: [JavaScript Error: "browser.sessions.setTabValue:
> Value is invalid" {file: "chrome://browser/content/ext-sessions.js" line:
> 140}]

It works for me. I only tested it with getTabValue, which I changed to look like:

        getTabValue: function(tabId, key) {
          enforceNoTemporaryAddon(extension.id);

          let tab = tabTracker.getTab(tabId);
          key = formatExtensionKey(extension.id, key);
          let value = SessionStore.getTabValue(tab, key);
          if (value) {
            return Promise.resolve(JSON.parse(value));
          }
          throw new ExtensionError(`No value was found for the key: ${key}.`);
        },

In the test, where you are doing:

      // This should now error as the key no longer exists.
      try {
        let testValue2 = await browser.sessions.getTabValue(currentTabId, key);
        browser.test.fail(`Key has been deleted and there should be no value for key '${key}', but value is '${testValue2}'`);
        runNextTest();
      } catch(e) {
        browser.test.succeed(`Key has been deleted and there should be no value for key '${key}'`);
        runNextTest();
      }

If I look at the value of `e` in the catch block it is "Error: No value was found for the key: extension:[e-mail]:tabkey2." (for example).

So I'm not sure what exactly isn't working for you.

Putting that aside for a moment, I have some news that you may not be thrilled about, although it's not that big a deal. I made some changes to ext-sessions.js for bug 1365562 which I expect to land today. It changes the file to use async/await so you'll need to rebase your changes against that new version of the file, and make some minor changes to your code. This might in fact make this whole error handling thing easier. Just take a look at the new code for ext-sessions.js (which you can see at [1] right now) and you should be able to see what changes you'll need to make.

After you've done that rebasing and made some alterations to your code, try the test again and perhaps that will fix the exception issues.

[1] https://reviewboard.mozilla.org/r/142370/diff/2#index_header

Comment 83

[u462496]

Attachment: API_sessions_window_tab_value_rebased2_P1_V1.diff

Rebased and updated.

Finally got to this Bob.  Your suggestions took care of the error reporting issue.

Comment 84

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8893454 [details] [diff] [review]
API_sessions_window_tab_value_rebased2_P1_V1.diff

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

Thanks for the update, Kevin, nice work. I actually forgot that this was still in progress! It all looks pretty good - just a bunch of little comments to be addressed.

::: browser/components/extensions/ext-sessions.js
@@ +56,5 @@
>    sessionObj.tab = extension.tabManager.convert(restored);
>    return sessionObj;
>  };
>  
> +function enforceNoTemporaryAddon(extensionId) {

Because all top-level functions in ext-*.js files are loaded into the same scope, we have adopted a practice of always declaring these via `const`, as you'll notice with `getRecentlyClosed` above. Can you please do this for `enforceNoTemporaryAddon`?

@@ +66,5 @@
> +    throw new ExtensionError(message);
> +  }
> +}
> +
> +function formatExtensionKey(extensionId, key) {

As above, please declare this via `const`.

@@ +169,5 @@
> +        removeTabValue: function(tabId, key) {
> +          enforceNoTemporaryAddon(extension.id);
> +
> +          let tab = tabTracker.getTab(tabId);
> +          key = formatExtensionKey(extension.id, key);

All three of the above functions (setTabValue, getTabValue and removeTabValue) do the same three steps at the beginning (call enforceNoTemporaryAddon(), get the tab via tabTracker, and get the key via formatExtensionKey), so it might be a good idea to extract these into their own function and then reuse it in each of these functions.

@@ +170,5 @@
> +          enforceNoTemporaryAddon(extension.id);
> +
> +          let tab = tabTracker.getTab(tabId);
> +          key = formatExtensionKey(extension.id, key);
> +          SessionStore.deleteTabValue(tab, key);

What happens if the key doesn't exist? Do we need to worry about possible exceptions?

@@ +198,5 @@
> +          throw new ExtensionError(`No value was found for the key: ${key}`);
> +        },
> +
> +        removeWindowValue: function(windowId, key) {
> +          enforceNoTemporaryAddon(extension.id);

As with the *TabValue functions, all of these *WindowValue functions have the same 3 setup steps, so you could reduce duplication my extracting them into a separate function. One function could be used for both the *TabValue functions and the *WindowValue functions by using an argument to indicate if it's a tab you care about or a window.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +8,5 @@
> +                 { key: "tabkey2", value: 25 },
> +                 { key: "tabkey3", value: { val: "Tab Value" } },
> +                 { key: "tabkey4", value: function() { return null; } }];
> +
> +    function runNextTest() {

It seems like this could be simplified by just looping through the `tests` array in the `background` function and dispensing with the whole `runNextTest` thing.

@@ +47,5 @@
> +      // Remove the tab key/value.
> +      browser.sessions.removeTabValue(currentTabId, key);
> +
> +      // This should now error as the key no longer exists.
> +      try {

Rather than a try/catch, you should be able to use assertRejects or assertThrows here.

@@ +115,5 @@
> +        let testValue2 = await browser.sessions.getTabValue(tabId_2, key);
> +
> +        browser.test.assertEq(value1, testValue1, `Value for key '${key}' should be '${value1}'.`);
> +        browser.test.assertEq(value2, testValue2, `Value for key '${key}' should be '${value2}'.`);
> +        browser.test.assertTrue((testValue1 != testValue2), `Value for key '${key}' in tabs 1 and 2 should not be the same.`);

Haven't you already proven this with the two lines above?

@@ +124,5 @@
> +        let tabId_3 = duptab.id;
> +
> +        let testValue3 = await browser.sessions.getTabValue(tabId_3, key);
> +
> +        browser.test.assertEq(value2, testValue3, `Value for key '${key}' should be '${value1}'.`);

I think there's a typo here. In the message I think you meant to say "should be '${value2}'".

@@ +133,5 @@
> +
> +        let sessions = await browser.sessions.getRecentlyClosed({ maxResults: 1 });
> +
> +        let sessionData = await browser.sessions.restore(sessions[0].tab.sessionId);
> +        let restoredTab = sessionData.tab;

You don't really need this interim restoredTab. Why not just `let restoredId = sessionData.tab.id;` ?

@@ +148,5 @@
> +
> +      }, { url: [{ hostContains: "example.com" }] });
> +    }
> +
> +    runTest();

You don't need to put the body of the test in a `runTest()` function and then call `runTest()`, just put the body directly inside the `background` function.

@@ +180,5 @@
> +                 { key: "winkey2", value: 25 },
> +                 { key: "winkey3", value: { val: "Window Value" } },
> +                 { key: "winkey4", value: function() { return null; } }];
> +
> +    function runNextTest() {

As above, it seems like this could be simplified by just looping through the `tests` array in the `background` function and dispensing with the whole `runNextTest` thing.

@@ +218,5 @@
> +      // Remove the window key/value.
> +      browser.sessions.removeWindowValue(currentWinId, key);
> +
> +      // This should now error as the key no longer exists.
> +      try {

As above, you should be able to use assertRejects or assertThrows here.

@@ +284,5 @@
> +      await browser.windows.remove(window2Id);
> +      browser.test.sendMessage("testComplete");
> +    }
> +
> +    test();

You don't need to put the body of the test in a `test()` function and then call `test()`, just put the body directly inside the `background` function.

Comment 85

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #84)
> Comment on attachment 8893454 [details] [diff] [review]
> API_sessions_window_tab_value_rebased2_P1_V1.diff
> 
> Review of attachment 8893454 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the speedy review Bob!

I just scanned your comments and they all seem to make sense.  I'll get to work on this I hope sometime today.

Comment 86

[u462496]

Attachment: API_sessions_window_tab_value_rebased2_P1_V2.diff

Hope this looks better.

(In reply to Bob Silverberg [:bsilverberg] from comment #84)
> Comment on attachment 8893454 [details] [diff] [review]
> API_sessions_window_tab_value_rebased2_P1_V1.diff
> 
> Review of attachment 8893454 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +170,5 @@
> > +          enforceNoTemporaryAddon(extension.id);
> > +
> > +          let tab = tabTracker.getTab(tabId);
> > +          key = formatExtensionKey(extension.id, key);
> > +          SessionStore.deleteTabValue(tab, key);
> 
> What happens if the key doesn't exist? Do we need to worry about possible
> exceptions?

No.  SessionStore.deleteTabValue used to throw if the key wasn't valid, but that is no longer true.  The intended result will remain the same, ie, no value for that key.  I've added tests to assert that fact.

Comment 87

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8893642 [details] [diff] [review]
API_sessions_window_tab_value_rebased2_P1_V2.diff

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

Looks good, Kevin. Thanks for addressing my comments so quickly. Just a few minor things left to address.

::: browser/components/extensions/ext-sessions.js
@@ +71,5 @@
> +  if (isWindow) {
> +    tabOrWindow = windowTracker.getWindow(id, context);
> +  } else {
> +    tabOrWindow = tabTracker.getTab(id);
> +  }

How about just `let tabOrWindow = isWindow ? windowTracker.getWindow(id, context) : tabTracker.getTab(id);` ?

@@ +150,5 @@
>          },
>  
> +        setTabValue: function(tabId, key, value) {
> +          let { tabOrWindow, encodedKey } =
> +            getTabOrWindowParams(extension.id, key, tabId, context);

Because you know you're getting a tab back, this doesn't need to pass `context`. Maybe make `context` an optional argument to `getTabOrWindowParams`? If you do that, you could even remove the `isWindow` argument as you'd know it's a window if you were passed a `context`.

@@ +154,5 @@
> +            getTabOrWindowParams(extension.id, key, tabId, context);
> +
> +          value = JSON.stringify(value);
> +
> +          SessionStore.setTabValue(tabOrWindow, encodedKey, value);

This seems a bit ugly to me, that you need to use `tabOrWindow` here, even though you know it's going to be a tab. I understand it's because you're returning an object with a `tabOrWindow` property from `getTabOrWindowParams`, but maybe that property could be renamed to something that works in both cases? Something like uiObject? I'm not sure of the best name, and this isn't a huge deal, but it just bugs me to use `tabOrWindow` in a function that is only about tabs.

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +255,5 @@
> +    let testValue2 = await browser.sessions.getWindowValue(window2Id, key);
> +
> +    browser.test.assertEq(value1, testValue1, `Value for key '${key}' should be '${value1}'.`);
> +    browser.test.assertEq(value2, testValue2, `Value for key '${key}' should be '${value2}'.`);
> +    browser.test.assertTrue((testValue1 != testValue2), `Value for key '${key}' in windows 1 and 2 should not be the same.`);

I guess I neglected to comment on this one last time, but as mentioned in my previous review, I think you've already proven this by the two asserts that precede it.

Comment 88

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #87)
> Comment on attachment 8893642 [details] [diff] [review]
> API_sessions_window_tab_value_rebased2_P1_V2.diff
> 
> Review of attachment 8893642 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks again for the speedy review !

> This seems a bit ugly to me, that you need to use `tabOrWindow` here, even
> though you know it's going to be a tab. I understand it's because you're
> returning an object with a `tabOrWindow` property from
> `getTabOrWindowParams`, but maybe that property could be renamed to
> something that works in both cases? Something like uiObject? I'm not sure of
> the best name, and this isn't a huge deal, but it just bugs me to use
> `tabOrWindow` in a function that is only about tabs.

I had thought about doing something like this (which can be modified to work with your comment regarding gleaning context instead of using isWindow):

  let tab;
  let window;
  if (isWindow) {
    window = windowTracker.getWindow(id, context);
  } else {
    tab = tabTracker.getTab(id);
  }

  return { encodedKey, tab, window };

        ....

        setTabValue: function(tabId, key, value) {
          let { tab, encodedKey } =
            getTabOrWindowParams(extension.id, key, tabId, context);

          value = JSON.stringify(value);

          SessionStore.setTabValue(tab, encodedKey, value);

        ....

        setWindowValue: function(windowId, key, value) {
          let { window, encodedKey } =
            getTabOrWindowParams(extension.id, key, windowId, context, true);

          value = JSON.stringify(value);

          SessionStore.setWindowValue(window, encodedKey, value);

What do you think?

Comment 89

[Bob Silverberg [:bsilverberg]]

(In reply to Kevin Jones from comment #88)
> 
> I had thought about doing something like this (which can be modified to work
> with your comment regarding gleaning context instead of using isWindow):
> 
>   let tab;
>   let window;
>   if (isWindow) {
>     window = windowTracker.getWindow(id, context);
>   } else {
>     tab = tabTracker.getTab(id);
>   }
> 
>   return { encodedKey, tab, window };
> 
>         ....
> 
>         setTabValue: function(tabId, key, value) {
>           let { tab, encodedKey } =
>             getTabOrWindowParams(extension.id, key, tabId, context);
> 
>           value = JSON.stringify(value);
> 
>           SessionStore.setTabValue(tab, encodedKey, value);
> 
>         ....
> 
>         setWindowValue: function(windowId, key, value) {
>           let { window, encodedKey } =
>             getTabOrWindowParams(extension.id, key, windowId, context, true);
> 
>           value = JSON.stringify(value);
> 
>           SessionStore.setWindowValue(window, encodedKey, value);
> 
> What do you think?

Honestly, I'm not sure. It solves my issue, but introduces a new one that both tab and window are returned even though one is essentially invalid. If you like it better, let's go with that, otherwise keep it the way it was. It will have to go through a final review anyway.

I'm about to leave for a couple of weeks' vacation, and I believe I'm good with the patch thus far, so I will give it my r+. Please address any of my comments from my last review that you feel should be addressed (they're mostly nits) and then request a final review from a WebExtensions peer.

Comment 90

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8893642 [details] [diff] [review]
API_sessions_window_tab_value_rebased2_P1_V2.diff

r+ with or without nits addressed. Nice work Kevin. :)

Comment 91

[u462496]

(In reply to Bob Silverberg [:bsilverberg] from comment #89)
> Honestly, I'm not sure. It solves my issue, but introduces a new one that
> both tab and window are returned even though one is essentially invalid. If
> you like it better, let's go with that, otherwise keep it the way it was. It
> will have to go through a final review anyway.
> 
> I'm about to leave for a couple of weeks' vacation, and I believe I'm good
> with the patch thus far, so I will give it my r+. Please address any of my
> comments from my last review that you feel should be addressed (they're
> mostly nits) and then request a final review from a WebExtensions peer.

Yeah, I know, it's pretty awkward either way.  If I can think of better sounding property to replace tabOrWindow, I'll probably go with that, like you suggested in the first place.

Thanks a lot Bob, and have a good time on your holiday :-)

Comment 92

[u462496]

Attachment: API_sessions_window_tab_value_rebased2_P2_V1.diff

Updated per Bob's recommendations.  Seeking WE peer review.

Comment 93

[Kris Maglione [:kmag]]

Comment on attachment 8896743 [details] [diff] [review]
API_sessions_window_tab_value_rebased2_P2_V1.diff

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

r=me with nits fixed.

Thanks, and sorry for the delay!

::: browser/components/extensions/ext-sessions.js
@@ +11,5 @@
>  } = ExtensionUtils;
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
>                                    "resource:///modules/sessionstore/SessionStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "AddonManagerPrivate",

Nit: Please keep sorted.

@@ +66,5 @@
> +    throw new ExtensionError(message);
> +  }
> +
> +  let encodedKey = `extension:${extensionId}:${key}`;
> +  let uiObject = context ? windowTracker.getWindow(id, context) : tabTracker.getTab(id);

It seems strange to decide whether to return a tab or a window depending on whether `context` is passed. Can we have separate `getTabParams` and `getWindowParams` functions, and have them return `{encodedKey, tab}` and `{encodedKey, window}` objects, and call a `getEncodedKey(extension, key)` function that does the ID checks and encoding?

@@ +68,5 @@
> +
> +  let encodedKey = `extension:${extensionId}:${key}`;
> +  let uiObject = context ? windowTracker.getWindow(id, context) : tabTracker.getTab(id);
> +
> +  return { encodedKey, uiObject };

Nit: ESLint forbids spaces inside curly braces in cases like this. Same for below.

@@ +143,5 @@
>            }
>            return createSession(session, extension, closedId);
>          },
>  
> +        setTabValue: function(tabId, key, value) {

Nit: `setTabValue(...) {`

Same for the others.

@@ +147,5 @@
> +        setTabValue: function(tabId, key, value) {
> +          let { uiObject, encodedKey } =
> +            getTabWindowParams(extension.id, key, tabId);
> +
> +          value = JSON.stringify(value);

Nit: Please do the `JSON.stringify` inline rather than assigning to a variable.

@@ +158,5 @@
> +            getTabWindowParams(extension.id, key, tabId);
> +
> +          let value = SessionStore.getTabValue(uiObject, encodedKey);
> +          if (value) {
> +            return Promise.resolve(JSON.parse(value));

Nit: Please mark function `async` and then just return the value rather than using `Promise.resolve`. Same for below.

@@ +161,5 @@
> +          if (value) {
> +            return Promise.resolve(JSON.parse(value));
> +          }
> +
> +          throw new ExtensionError(`No value was found for the key: ${key}`);

Can we just return undefined rather than rejecting here?

@@ +175,5 @@
> +        setWindowValue: function(windowId, key, value) {
> +          let { uiObject, encodedKey } =
> +            getTabWindowParams(extension.id, key, windowId, context);
> +
> +          value = JSON.stringify(value);

Nit: Please inline.

@@ +189,5 @@
> +          if (value) {
> +            return Promise.resolve(JSON.parse(value));
> +          }
> +
> +          throw new ExtensionError(`No value was found for the key: ${key}`);

And here?

::: browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js
@@ +3,5 @@
> +add_task(async function test_sessions_tab_value() {
> +  info("Testing set/get/deleteTabValue.");
> +
> +  async function background() {
> +    let tests = [{ key: "tabkey1", value: "Tab Value" },

Nit: No spaces inside curly brackets for object literals. Same for below. ESLint will enforce this.

@@ +50,5 @@
> +    }
> +
> +    // Attempt to remove a non-existent key, should not throw error.
> +    let tabs = await browser.tabs.query({currentWindow: true, active: true});
> +    browser.sessions.removeTabValue(tabs[0].id, "non-existent-key");

Nit: `await`

@@ +207,5 @@
> +    }
> +
> +    // Attempt to remove a non-existent key, should not throw error.
> +    let win = await browser.windows.getCurrent();
> +    browser.sessions.removeWindowValue(win.id, "non-existent-key");

Nit: `await`

Comment 94

[u462496]

Attachment: API_sessions_window_tab_value_rebased3_V1.diff

Updated per Kris's comments.

Comment 95

[u462496]

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

Comment 96

[u462496]

(In reply to Kris Maglione [:kmag] from comment #93)
> Comment on attachment 8896743 [details] [diff] [review]
> API_sessions_window_tab_value_rebased2_P2_V1.diff
> 
> Review of attachment 8896743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits fixed.
> 
> Thanks, and sorry for the delay!

Thanks a lot, Kris, and no need to apologize, I know you are very busy right now.

I've made all the changes you requested and if the try run looks good to everyone I'll ask for checkin.

Comment 97

[u462496]

I'm not seeing any errors from the try run that seem to be a result of this patch, so I am marking this checkin-needed.

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff78a3d649c
Extension API to store persistent values on window/tab using get/set/remove methods. r=kmag, r=bsilverberg

Comment 99

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0ff78a3d649c

Comment 100

[Dietrich Ayala (:dietrich)]

Would be good to have this documented on MDN, currently is hidden feature:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions

Comment 101

[Bob Silverberg [:bsilverberg]]

(In reply to Dietrich Ayala (:dietrich) from comment #100)
> Would be good to have this documented on MDN, currently is hidden feature:
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions

The bug is marked with dev-doc-needed, so it will get documented at some point. You are also welcome to add it yourself to MDN if you'd like.

Comment 102

[Will Bamberg [:wbamberg]]

Docs:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/setTabValue
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/getTabValue
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/removeTabValue
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/setWindowValue
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/getWindowValue
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/removeWindowValue

I've also written a simple example, which I'd be happy for you to take a look at: https://github.com/mdn/webextensions-examples/pull/300.

Comment 103

[YUKI "Piro" Hiroshi]

I realized that these features should be used carefully on some edge cases, especially restored tabs (by "Undo Close Tab", crash recovery, etc.) or duplicated tabs. When such a tab is tracked by tabs.onCreated, I sometimes fail to get stored values because the tab's session information is not restored yet. This I need to wait until stored data is actually become readable, like:
https://github.com/piroor/treestyletab/blob/11569dabfe97075a6f9b20669a41340af99eeab4/webextensions/common/tree/base.js#L124

I need new events like `sessions.onRestored` or something.

Comment 104

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #102)
> Docs:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> setTabValue
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> getTabValue
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> removeTabValue
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> setWindowValue
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> getWindowValue
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sessions/
> removeWindowValue
> 
> I've also written a simple example, which I'd be happy for you to take a
> look at: https://github.com/mdn/webextensions-examples/pull/300.

Hi Will, sorry for the delayed response.  The docs look great, just a couple of things I am wondering about:

There is no indication (unless I missed it) that values stored will only be accessible to the addon which stored them.  Is this already implied in the sessions API?

One nit, setTabValue and setWindowValue both can take either an object or a string for a value.  You make that clear in the description, though in the "Syntax" highlight block, only `string` is mentioned.  Is that in keeping with convention?

I installed the extension from github, and everything seems to work as it is supposed to.

Thanks for your work on this.

Comment 105

[Will Bamberg [:wbamberg]]

Thanks for the review, Kevin. I think your comments make plenty of sense and have tried to address them.

Comment 106

[YUKI "Piro" Hiroshi]

Sorry my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1322060#c103 was quite wrong. It based on my misunderstanding and getTabValue() works for duplicated and restored tabs as expected.

Comment 107

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 108

[randomreporter]

Tree Style Tab WebExtension has big problems with session restore even after this bug landed: order of tabs and tree structure are messed up after restoring saved session using Firefox built-in session manager. STR are quite simple - things start to go downhill if there is an active tab inside stack, stack inside stack, stack inside stack inside stack or even if you just move the stack. Here is the list of bugs that I filed:
https://github.com/piroor/treestyletab/issues/1476
https://github.com/piroor/treestyletab/issues/1488
https://github.com/piroor/treestyletab/issues/1489
https://github.com/piroor/treestyletab/issues/1492
https://github.com/piroor/treestyletab/issues/1493
https://github.com/piroor/treestyletab/issues/1500

Piro tried to fix these bugs to no avail.