Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON

RESOLVED FIXED in Firefox 52

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
getClosedTabData and getClosedWindowData in SessionStore.jsm each serialize their return data to JSON before returning it. As each only has one consumer [1] and [2], and those consumers just deserialize the data after receiving it, it makes sense to remove this serialization. These methods will also be used by the new sessions WebExtensions API (bug 1300399).

I discussed this with mikedeboer via email and he is in agreement with this change.

[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm#44
[2] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm#102
If my memory serves, this was kept JSON mainly for compatibility with some add-ons. I realize that it's less a priority these days, but perhaps a new name for the non-JSON version would be just as good?
(Assignee)

Comment 2

2 years ago
That's a good point, David, and doing that would also save me from having to fix up all the tests which expect JSON to be returned. Do you think it would be okay to allow for an optional argument to indicate the we do not want JSON returned, or do you think new functions with new names would be better?
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
I made the changes, introducing new methods for receiving an array rather than a JSON string from each method. I also updated a few of the tests just to prove that the new functions work. I'm not sure how much more test coverage is needed, if any, but I'm happy to add more if it is required.

Comment 5

2 years ago
mozreview-review
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84126

::: browser/components/sessionstore/SessionStore.jsm:249
(Diff revision 1)
>  
>    getClosedTabCount: function ss_getClosedTabCount(aWindow) {
>      return SessionStoreInternal.getClosedTabCount(aWindow);
>    },
>  
> -  getClosedTabData: function ss_getClosedTabDataAt(aWindow) {
> +  getClosedTabData: function ss_getClosedTabData(aWindow) {

I was actually thinking to expose this feature only to SessionStore.jsm consumers by adding an argument to `getClosedTabData` and not by adding another method:

```js
getClosedTabData: function ss_getClosedTabData(aWindow, aAsString = true) {
  // ... Proxies through to internal implementation ...
  return aAsString ? JSON.stringify(data._closedTabs) :
    [...data._closedTabs];
}
```

And not bother with updating the IDL and other related bloat, because addons will not use this. In fact, we don't want addons using this API/ optional return type at all.
Attachment #8800487 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84126

> I was actually thinking to expose this feature only to SessionStore.jsm consumers by adding an argument to `getClosedTabData` and not by adding another method:
> 
> ```js
> getClosedTabData: function ss_getClosedTabData(aWindow, aAsString = true) {
>   // ... Proxies through to internal implementation ...
>   return aAsString ? JSON.stringify(data._closedTabs) :
>     [...data._closedTabs];
> }
> ```
> 
> And not bother with updating the IDL and other related bloat, because addons will not use this. In fact, we don't want addons using this API/ optional return type at all.

Thanks Mike. I actually tried doing that first, but found that the function was not working when I tried returning an array rather than a string. It simply did not return at all. I assumed this was because the return value did not match what was in the IDL, but perhaps there was a different reason for that behaviour. Does it sound like this could have been the problem? If not, do you have any other ideas about what would have caused it to behave like that?

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84126

> Thanks Mike. I actually tried doing that first, but found that the function was not working when I tried returning an array rather than a string. It simply did not return at all. I assumed this was because the return value did not match what was in the IDL, but perhaps there was a different reason for that behaviour. Does it sound like this could have been the problem? If not, do you have any other ideas about what would have caused it to behave like that?

Yeah, that'd happen when you're calling the function on the XPCOM service, like RecentlyClosedTabsAndWindowsMenuUtils.jsm does. But if you want to change things up there too, you'll need to replace the `ss.*` usage with calls directly to `SessionStore.*` (after Cu.import-ing the module).
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84126

> Yeah, that'd happen when you're calling the function on the XPCOM service, like RecentlyClosedTabsAndWindowsMenuUtils.jsm does. But if you want to change things up there too, you'll need to replace the `ss.*` usage with calls directly to `SessionStore.*` (after Cu.import-ing the module).

Ok, I have updated the patch based on this and our conversation on IRC.

Comment 10

2 years ago
mozreview-review
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84322

Looking good! This is almost good to go, but if you don't mind, I'd like to have one more peek at it before we land it.
You can already run a try build with this patch to see if everything is still green.

Thanks!

::: browser/components/sessionstore/RecentlyClosedTabsAndWindowsMenuUtils.jsm:17
(Diff revision 2)
>  var Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

Can you make this a lazy module getter, just like 'PluralForms.jsm' below?

::: browser/components/sessionstore/SessionStore.jsm:249
(Diff revision 2)
>  
>    getClosedTabCount: function ss_getClosedTabCount(aWindow) {
>      return SessionStoreInternal.getClosedTabCount(aWindow);
>    },
>  
> -  getClosedTabData: function ss_getClosedTabDataAt(aWindow) {
> +  getClosedTabData: function ss_getClosedTabData(aWindow, asString=true) {

Yeah, this is old-style Hungarian notation for 'argument' (a) - so it should be `aAsString = true`. Please correct that throughout you patch. Also letting the default argument value breathe is good practice.

::: browser/components/sessionstore/SessionStore.jsm:2187
(Diff revision 2)
>      }
>  
>      return DyingWindowCache.get(aWindow)._closedTabs.length;
>    },
>  
> -  getClosedTabData: function ssi_getClosedTabDataAt(aWindow) {
> +  getClosedTabData: function ssi_getClosedTabData(aWindow, asString) {

Can you put the default argument value assignment here too?

::: browser/components/sessionstore/test/browser_394759_basic.js:41
(Diff revision 2)
>      setInputChecked(browser, {id: "chk", checked: true}).then(() => {
>        BrowserTestUtils.closeWindow(newWin).then(() => {
>          is(ss.getClosedWindowCount(), 1,
>             "The closed window was added to Recently Closed Windows");
> -        let data = JSON.parse(ss.getClosedWindowData())[0];
> +
> +        // verify that non JSON serialized data is the same as JSON serialized data

nit: capital 'V' and missing dot at the end.

::: browser/components/sessionstore/test/browser_394759_basic.js:42
(Diff revision 2)
>        BrowserTestUtils.closeWindow(newWin).then(() => {
>          is(ss.getClosedWindowCount(), 1,
>             "The closed window was added to Recently Closed Windows");
> -        let data = JSON.parse(ss.getClosedWindowData())[0];
> +
> +        // verify that non JSON serialized data is the same as JSON serialized data
> +        is(JSON.stringify(SessionStore.getClosedWindowData(false)), ss.getClosedWindowData(),

Or put `let data = SessionStore.getClosedWindowData(false);` on line 41 and do `JSON.stringify(data)` and reuse `data` again below.

::: browser/components/sessionstore/test/browser_461634.js:42
(Diff revision 2)
>    promiseWindowLoaded(newWin).then(() => {
>      gPrefService.setIntPref("browser.sessionstore.max_tabs_undo",
>                              test_state.windows[0]._closedTabs.length);
>      ss.setWindowState(newWin, JSON.stringify(test_state), true);
>  
> -    let closedTabs = JSON.parse(ss.getClosedTabData(newWin));
> +    // verify that non JSON serialized data is the same as JSON serialized data

nit: capital 'V' and missing dot.

::: browser/components/sessionstore/test/browser_461634.js:43
(Diff revision 2)
>      gPrefService.setIntPref("browser.sessionstore.max_tabs_undo",
>                              test_state.windows[0]._closedTabs.length);
>      ss.setWindowState(newWin, JSON.stringify(test_state), true);
>  
> -    let closedTabs = JSON.parse(ss.getClosedTabData(newWin));
> +    // verify that non JSON serialized data is the same as JSON serialized data
> +    is(JSON.stringify(SessionStore.getClosedTabData(newWin, false)), SessionStore.getClosedTabData(newWin),

Same here, you can move `let closedTabs = SessionStore.getClosedTabData(newWin, false);` on line 42, do `JSON.stringify(closedTabs)` and reuse it below.

::: browser/components/sessionstore/test/browser_461634.js:68
(Diff revision 2)
>      // Remove third tab, then first tab
>      ss.forgetClosedTab(newWin, 2);
>      ss.forgetClosedTab(newWin, null);
>  
> -    closedTabs = JSON.parse(ss.getClosedTabData(newWin));
> +    // verify that non JSON serialized data is the same as JSON serialized data
> +    is(JSON.stringify(SessionStore.getClosedTabData(newWin, false)), SessionStore.getClosedTabData(newWin),

Same here!
Attachment #8800487 - Flags: review?(mdeboer) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84322

> nit: capital 'V' and missing dot at the end.

I fixed this, but I actually intentionally did it that way as it is consistent with the rest of the comments in the test. I figured consistency was more important than punctuation. For this reason I have now changed all of the existing comments to be capitalized and dotted.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8800487 [details]
Bug 1309702 - Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON,

https://reviewboard.mozilla.org/r/85394/#review84514

Looking good! Thanks for the updates. Ship it!
Attachment #8800487 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 14

2 years ago
Try looks good, requesting check in.
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/60af9c33d9c1
Update getClosedTabData and getClosedWindowData in SessionStore.jsm to not return JSON, r=mikedeboer
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60af9c33d9c1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.