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

ASSIGNED
Assigned to

Status

()

Toolkit
WebExtensions: General
P3
normal
ASSIGNED
6 months ago
11 hours ago

People

(Reporter: Kevin Jones, Assigned: Kevin Jones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 13 obsolete attachments)

21.12 KB, patch
bsilverberg
: review-
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

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

Comment 2

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

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

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

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

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

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

Comment 4

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

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

MAX_SESSION_RESULTS
getRecentlyClosed
restore
onChanged

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

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

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

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

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

Comment 5

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

Comment 6

5 months ago
For example:

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

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

Currently, by accessing

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

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

Also, by storing my own data this way:

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

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

Comment 7

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

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

Updated

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

Comment 8

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

Updated

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

Comment 11

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

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

Comment 13

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

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

Comment 15

3 months ago
Created attachment 8843712 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V1.diff

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

Comment 16

3 months ago
Created attachment 8843713 [details]
WE_sessions_git.xpi

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

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

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

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

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

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

This isn't needed. See below.

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

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

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

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

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

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

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

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

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

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

Comment 18

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

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

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

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

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

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

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

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

Comment 20

3 months ago
Created attachment 8844605 [details] [diff] [review]
API_sessions_setgetWindowTabValue_V2.diff

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

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

Comment 22

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

Hello Bob,

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

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

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

Does that clear it up?
(Assignee)

Comment 24

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

Yes, that makes sense.
(Assignee)

Comment 25

3 months ago
Created attachment 8845431 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js

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

Comment 26

3 months ago
Created attachment 8845715 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js V2

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

Comment 27

3 months ago
Created attachment 8845716 [details]
browser_ext_sessions_setgetTabValue_setgetWindowValue.js V3

Ugh!! Corrected typo in test description.

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

Comment 28

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

Comment 29

3 months ago
Created attachment 8845902 [details]
browser_ext_sessions set/get/delete tab/window value

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

Comment 30

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

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

Comment 31

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

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

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

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

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

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

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

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

Comment 34

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

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

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

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

Comment 36

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

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

Comment 37

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

Comment 38

3 months ago
I have uploaded a new patch that does not contain the restrictions discussed in comment 34, comment 35.  Perhaps we can land the changes without the restrictions, and if a compelling reason ever presents itself in the future we can introduce those later.

Comment 39

2 months ago
I haven't read everything, but if the question is whether extensions should be able to see data stored by other extensions, I would argue for no, extensions should use runtime.sendMessage() to communciate with other extensions.
In that way, an extension can decide exactly what information it wants to share and which extensions it wants to share with, with the safe default being "nothing/nobody"
Flags: needinfo?(aswan)

Comment 40

2 months ago
Comment on attachment 8846505 [details] [diff] [review]
API_sessions_window_tab_value_V5.diff

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

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

So your test actually gets ran on automation.

----

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

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

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

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

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

same here.
(Assignee)

Comment 41

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

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

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

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

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

Comment 42

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

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

So maybe a property named "savedSessionData" ?

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

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

Comment 43

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

Comment 44

2 months ago
Created attachment 8847598 [details] [diff] [review]
API_sessions_window_tab_value_V6.diff

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

Comment 45

2 months ago
Comment on attachment 8847598 [details] [diff] [review]
API_sessions_window_tab_value_V6.diff

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

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

./mach eslint --setup

then

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


Someone from the WE team should review this.

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

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

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

You could probably use an async function here.

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

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

You could use a template string here:

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

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

Could be an async function too:

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

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

Updated

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

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

Good start, but needs a bit of work.

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

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

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

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

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

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

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

We should allow arbitrary JSON-compatible data here.

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

`currentWindow: true, active: true`

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

Nit: Please use a template string.

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

Template string.

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

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

Comment 47

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

Thanks, Kris.

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

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

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

Comment 48

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

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

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

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

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

Comment 50

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

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

Comment 51

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

Comment 52

2 months ago
Created attachment 8849561 [details] [diff] [review]
API_sessions_window_tab_value_V8.diff

Updated with recommendations.  I used strict privacy in this patch, as at this point I don't know what Tree Style Tabs intends in a WebExtension.
Attachment #8843713 - Attachment is obsolete: true
Attachment #8847598 - Attachment is obsolete: true
Attachment #8849561 - Flags: review?(kmaglione+bmo)
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.
Attachment #8849561 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 54

a month ago
(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}`);
}
(Assignee)

Updated

a month ago
Depends on: 1359806
(Assignee)

Comment 55

a month ago
Test for this bug depends on bug 1359806.
(Assignee)

Comment 56

29 days ago
(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.
(Assignee)

Comment 57

25 days ago
Created attachment 8863388 [details] [diff] [review]
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.
Attachment #8849561 - Attachment is obsolete: true
Attachment #8863388 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 58

25 days ago
Created attachment 8863391 [details] [diff] [review]
API_sessions_window_tab_value_rebased_P2_V2.diff

oops, the test didn't make it on the last one...
Attachment #8863388 - Attachment is obsolete: true
Attachment #8863388 - Flags: review?(kmaglione+bmo)
Attachment #8863391 - Flags: review?(kmaglione+bmo)
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.
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 ;)
(Assignee)

Comment 61

15 days ago
(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...
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.
(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?
Flags: needinfo?(mdeboer)
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?
Flags: needinfo?(kmaglione+bmo) → needinfo?(kevinhowjones)
(Assignee)

Updated

11 days ago
Flags: needinfo?(kevinhowjones)
Attachment #8863391 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

11 days ago
Attachment #8863391 - Flags: review?(bob.silverberg)
(Assignee)

Comment 65

10 days ago
(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 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?
Attachment #8863391 - Flags: review?(bob.silverberg) → review-
(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.
Flags: needinfo?(mdeboer)
Thanks Mike. I have opened bug 1365562 as a follow up.
(Assignee)

Comment 69

9 days ago
(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.

:-/
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 70

9 days ago
(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.
(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.
Flags: needinfo?(bob.silverberg)
(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?
(Assignee)

Comment 73

8 days ago
(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.
(Assignee)

Comment 74

8 days ago
(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.
Flags: needinfo?(bob.silverberg)
Duplicate of this bug: 1364019
(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.
Flags: needinfo?(bob.silverberg)
(Assignee)

Comment 77

3 days ago
(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.
(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.
(Assignee)

Comment 79

3 days ago
(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.
(Assignee)

Comment 80

2 days ago
(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

18 hours ago
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 .
(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:exampleextension@mozilla.org: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
You need to log in before you can comment on or make changes to this bug.