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

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

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?
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?
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 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-
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 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 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 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 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 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+
Try looks good, requesting check in.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/60af9c33d9c1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.