548939 - Use SessionStore in tab engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Firefox has sessionstore, so we should use it. It's already tracking everything that the tab engine ends up collecting, so let's not do that twice.

This will also involve writing a fake sessionstore service for Fennec (can reuse some of what is already being done, but we can make it match behavior of sessionstore better)

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

It might need a few tweaks, but it works.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Since it was asked, this does fix the issue where using the BarTab extension causes the "unloaded" tabs to go missing when syncing.

Comment 3

[Ed Lee :Mardak]

Comment on attachment 429882 [details] [diff] [review]
Patch v0.1

>@@ -245,16 +241,17 @@ TabTracker.prototype = {
>   onTab: function onTab(event) {
>-    event.originalTarget.lastUsed = Math.floor(Date.now() / 1000);
>+    Svc.Session.setTabValue(event.originalTarget, "weaveLastUsed",
>+                            Math.floor(Date.now() / 1000));
The originalTarget is different on Firefox vs Fennec. I fixed an issue with the original code because it assumed the DOM method .setAttribute was available on the tab. That's true on Firefox, but for Fennec, the tab is a plain JS object. That's why I went with just setting a JS property.

But I suppose we're implementing our own setTabValue anyway, and it works by adding a custom property. So we're doing pretty much the same thing except more session-store-y.

>+  // Session Restore
>+  "@mozilla.org/browser/sessionstore;1": {
>+    setTabValue: function Weave_fakeServices_sessionStore(tab, key, value) {
Just function setTabValue(..) ! :)

>+    getBrowserState: function Weave_fakeServices_sessionStore() {
>+      let state = { windows: [] };
>+      let wins = Svc.WinMediator.getEnumerator("navigator:browser");
>+      while (wins.hasMoreElements()) {
This loop /should/ be unnecessary on Fennec. There's only one browser. I only added the loop to catch multiple Firefox windows.

>+        // Get the tabs
>+        let tabs = window.Browser._tabs;
>+        // Extract various pieces of tab data
>+        tabs.forEach(function(tab) {
Could just window.Browser._tabs.forEach(function(tab)
Doesn't seem like we use "tabs" later.

>+          // Add the tab to the window
>+          windowState.tabs.push(tabState);
>+        });
>+        // Add the window to the state
>+        state.windows.push(windowState);
>+      }
>+      return JSON.stringify(state);
Actually.... we could do something along the lines of ;)

return JSON.stringify({
  windows: [{
    tabs: window.Browser._tabs.map(function(tab) {
      ...; return tabState;
    }
  }]
});

But if we're keeping code in sync with Firefox's sessionstore, it might make more sense to copy/paste what's there in-case we need to port bug fixes. Either way, what you had before or this code-compaction is fine for now.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> But I suppose we're implementing our own setTabValue anyway, and it works by
> adding a custom property. So we're doing pretty much the same thing except more
> session-store-y.

Exactly. It makes it a bit more clear that it's associated with the "session" and not just arbitrary. This way it also persists across sessions (at least on Firefox desktop)

> This loop /should/ be unnecessary on Fennec. There's only one browser. I only
> added the loop to catch multiple Firefox windows.

Alright. Double checked and you're right. No more getEnumerator for me.

> Could just window.Browser._tabs.forEach(function(tab)
> Doesn't seem like we use "tabs" later.

Truth

> return JSON.stringify({
>   windows: [{
>     tabs: window.Browser._tabs.map(function(tab) {
>       ...; return tabState;
>     }
>   }]
> });
> 
> But if we're keeping code in sync with Firefox's sessionstore, it might make
> more sense to copy/paste what's there in-case we need to port bug fixes. Either
> way, what you had before or this code-compaction is fine for now.

I'm going to keep (essentially) what I had. While what you had looks cooler & cleaner, I think it'll be better to have clear separation of data collection, especially when we add more to what we're collecting.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/labs/weave/rev/2c21f332427d with review comments addressed.
Created a fake SessionStore service for Fennec that imitates the parts of Firefox's SessionStore API that we need. Then used the now "consistent" SessionStore service in the Tabs engine.