579361 - Enhance SessionStore service in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

including:
* Add basic support for undo closed tabs
* Add basic support so Sync does not need to fake a session for Fennec

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch:
* Adds basic interface and impl for supporting undo closed tabs to SessionStore component
* Adds getBrowserState and tab value API to SessionStore so Sync can use it
* Switches components to "@mozilla.org/browser" instead of "@mozilla/mobile" (Sync really needed this so SessionStore could actually be used)
* Adds a simple command and key for undoing the last closed tab (ctrl+shift+t, just like linux desktop)
* Added BrowserUI.undoClosedTab(index) like Firefox has
* browser.webNavigation.sessionHistory no longer throws, it just returns null. And I added a setter to stop a JS strict warning
* Added receiveMessage support to SessionStore - it was broken by e10s
* Fixed reading last open tabs in aboutHome.html since the SessionStore output JSON changed, It's more like the Firefox output now.
* InputHandler was killing the keyevents in chrome. None of the shortcut keys worked so I couldn't test ctrl+shift+t. Removing the event.stopPropagation and event.preventDefault seems to have worked and doesn't break web content form filling.

that's all!

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Oops. Added missing SessionStore.idl

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 457892 [details] [diff] [review]
patch 2

diff -r db21b5d9b799 chrome/content/InputHandler.js
--- a/chrome/content/InputHandler.js	Thu Jul 15 16:46:34 2010 +0200
+++ b/chrome/content/InputHandler.js	Fri Jul 16 18:57:38 2010 +0200
@@ -1163,11 +1163,8 @@
   handleEvent: function handleEvent(evInfo) {
     if (evInfo.event.type == "keydown" || evInfo.event.type == "keyup" || evInfo.event.type == "keypress") {
       let keyer = this._browserViewContainer.customKeySender;
-      if (keyer) {
+      if (keyer)
         keyer.dispatchKeyEvent(evInfo.event);
-        evInfo.event.preventDefault();
-        evInfo.event.stopPropagation();
-      }
     }
   },

Shortcuts seems to work for me (I'm using Ctrl+L) all the time, in the worst case I wonder if we could use getPreventDefault() somewhere.

 
diff -r db21b5d9b799 chrome/content/aboutHome.xhtml
--- a/chrome/content/aboutHome.xhtml	Thu Jul 15 16:46:34 2010 +0200
+++ b/chrome/content/aboutHome.xhtml	Fri Jul 16 18:57:38 2010 +0200
@@ -162,15 +162,19 @@
       
       let tabs = data.windows[0].tabs;
       for (let i=0; i<tabs.length; i++) {
-        let url = tabs[i].url;
+        let tabData = tabs[i];
+	if ("entries" in tabData)
+	  tabData = tabData.entries[0];
+

indentation error

diff -r db21b5d9b799 components/MobileComponents.manifest
--- a/components/MobileComponents.manifest	Thu Jul 15 16:46:34 2010 +0200
+++ b/components/MobileComponents.manifest	Fri Jul 16 18:57:38 2010 +0200
@@ -187,6 +192,15 @@
     }
   },
 
+  receiveMessage: function ss_receiveMessage(aMessage) {
+    let window = aMessage.target.ownerDocument.defaultView;
+    switch (aMessage.name) {
+      case "pageshow":
+        this.onTabLoad(window, aMessage.target, aMessage);
+        break;
+    }
+  },
+

I think page show could happen for nested iframes, too (bindings/browser.js#240).
I'm not sure that is what you want?

   onTabSelect: function ss_onTabSelect(aWindow) {
+    if (this._loadState != STATE_RUNNING)
+      return;
+
+    let index = 0;
+    let browser = aWindow.Browser;
+    let tabs = browser.tabs;
+    for (let i = 0; i < tabs.length; i++) {
+      if (tabs[i].browser == browser.selectedBrowser)
+        index = i;
+    }

You probably want to break once you have your index.



Works great on my computer.

Maybe we could add an UI for that (in a followup).
Also Firefox seems to restore the tab at the previous same position we should probably do that in a near future.

And this definitively need tests. :)

r+ with nits and questions addressed.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> diff -r db21b5d9b799 chrome/content/InputHandler.js
>        let keyer = this._browserViewContainer.customKeySender;
> -      if (keyer) {
> +      if (keyer)
>          keyer.dispatchKeyEvent(evInfo.event);
> -        evInfo.event.preventDefault();
> -        evInfo.event.stopPropagation();

> Shortcuts seems to work for me (I'm using Ctrl+L) all the time, in the worst
> case I wonder if we could use getPreventDefault() somewhere.

I couldn't get anything to work without these changes.

> diff -r db21b5d9b799 chrome/content/aboutHome.xhtml

> -        let url = tabs[i].url;
> +        let tabData = tabs[i];
> +    if ("entries" in tabData)
> +      tabData = tabData.entries[0];
> 
> indentation error

Fixed

> +  receiveMessage: function ss_receiveMessage(aMessage) {
> +    let window = aMessage.target.ownerDocument.defaultView;
> +    switch (aMessage.name) {
> +      case "pageshow":
> +        this.onTabLoad(window, aMessage.target, aMessage);
> 
> I think page show could happen for nested iframes, too
> (bindings/browser.js#240).
> I'm not sure that is what you want?

Won't matter really. We just use it as a trigger to update the data and save it at the next interval.

>    onTabSelect: function ss_onTabSelect(aWindow) {

> +    for (let i = 0; i < tabs.length; i++) {
> +      if (tabs[i].browser == browser.selectedBrowser)
> +        index = i;
> +    }
> 
> You probably want to break once you have your index.

Fixed

> Maybe we could add an UI for that (in a followup).
> Also Firefox seems to restore the tab at the previous same position we should
> probably do that in a near future.

I'll file another bug

> And this definitively need tests. :)

Coming up

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Added some tests. I had to disable all other tests so I could make sure these tests work in e10s, and they do. Our tests need to be fixed!

Anyway, pushed:
http://hg.mozilla.org/mobile-browser/rev/8c56aff4fd8e