Last Comment Bug 567583 - Get tabs sync to work for Seamonkey
: Get tabs sync to work for Seamonkey
Status: RESOLVED FIXED
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: 1.5
Assigned To: hariniachala
:
Mentors:
Depends on:
Blocks: 576970 590633
  Show dependency treegraph
 
Reported: 2010-05-22 13:06 PDT by hariniachala
Modified: 2010-09-08 00:48 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
This patch gets firefox tabs sync code to work for seamonkey as well. (3.10 KB, patch)
2010-05-22 13:16 PDT, hariniachala
no flags Details | Diff | Splinter Review
This patch gets firefox tabs sync code to work for seamonkey as well. (3.92 KB, patch)
2010-05-22 21:08 PDT, hariniachala
no flags Details | Diff | Splinter Review
This patch gets firefox tabs sync code to work for seamonkey as well. (4.11 KB, patch)
2010-05-30 05:33 PDT, hariniachala
mconnor: review-
Details | Diff | Splinter Review
patch to get tabs sync to work for seamonkey (4.08 KB, patch)
2010-06-11 00:23 PDT, hariniachala
no flags Details | Diff | Splinter Review
patch to get tab sync engine to work for seamonkey (2.49 KB, patch)
2010-07-04 07:19 PDT, hariniachala
philip.chee: feedback-
Details | Diff | Splinter Review
patch to get tab sync engine to work for seamonkey -v2 (5.66 KB, patch)
2010-07-04 21:59 PDT, hariniachala
no flags Details | Diff | Splinter Review
patch to get tab sync engine to work for seamonkey -v3 (5.63 KB, patch)
2010-07-07 12:37 PDT, hariniachala
no flags Details | Diff | Splinter Review
patch to get tab sync engine to work for seamonkey -v3' (5.63 KB, patch)
2010-07-08 00:17 PDT, hariniachala
no flags Details | Diff | Splinter Review
Tabs sync for Seamonkey (1.65 KB, patch)
2010-08-09 22:06 PDT, hariniachala
mconnor: review+
Details | Diff | Splinter Review
Bump seamonkey max version to 2.1b1 (404 bytes, patch)
2010-08-11 13:54 PDT, hariniachala
no flags Details | Diff | Splinter Review

Description hariniachala 2010-05-22 13:06:36 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9) Gecko/20100317 SeaMonkey/2.0.4
Build Identifier: 

This bug is reported to add tabs sync functionality for Seamonkey.

Reproducible: Always
Comment 1 hariniachala 2010-05-22 13:16:41 PDT
Created attachment 446912 [details] [diff] [review]
This patch gets firefox tabs sync code to work for seamonkey as well.
Comment 2 hariniachala 2010-05-22 21:08:35 PDT
Created attachment 446938 [details] [diff] [review]
This patch gets firefox tabs sync code to work for seamonkey as well.

resubmitting patch with two separate constants defined for nsISessionStore component URIs in modules/util.js for Firefox and Seamonkey.
Comment 3 Jens Hatlak (:InvisibleSmiley) 2010-05-29 15:55:08 PDT
(In reply to comment #2)
> Created an attachment (id=446938) [details]
> This patch gets firefox tabs sync code to work for seamonkey as well.

Harini, mconnor seems to be busy. Meanwhile I'd like to point out some things that would come up during an initial review. Even though I'm not a Weave developer or reviewer, I have some experience as a SeaMonkey contributor. As such please take the following with a grain of salt; in the end only the comments of the reviewers and/or module owners are authoritative.

Mozilla reviewers are generally quite picky about the coding style. Take <https://developer.mozilla.org/En/Developer_Guide/Coding_Style> as a baseline (relevant here: anything related to JavaScript) and look at the code in the vicinity of your modifications and you should get a good impression of the requirements. E.g. in fx-tabs.js, your for loop needs to be indented properly and the braces are superfluous (single-statement body). Also no empty lines should be added unless necessary. The comment should be a bit more descriptive (like "SeaMonkey doesn't support gBrowser.loadTabs") and similar ones should be added for the fx-weave-overlay.js changes. In tabs.js, you have two spaces after "currentState =". 

Finally, be prepared to get negative reviews ("review not granted"). This usually doesn't mean your whole effort is rejected, but just that you need to answer and address the review comments (which the reviewer will add to the bug), come up with a new patch and request review again.
Comment 4 hariniachala 2010-05-30 05:33:00 PDT
Created attachment 448266 [details] [diff] [review]
This patch gets firefox tabs sync code to work for seamonkey as well.

Corrected code styling errors in previous patch. Thank you Jens Hatlak for the heads up.

mconnor promised to review this in June so will wait for that..
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-05-30 06:47:37 PDT
Comment on attachment 448266 [details] [diff] [review]
This patch gets firefox tabs sync code to work for seamonkey as well.

OK, a little more detailed, then. ;-)

>     }
>-    if (urls.length) {
>-      getTopWin().gBrowser.loadTabs(urls);
>+    
>+    //Seamonkey doesn't support gBrowser.loadTabs
>+ 	for(let j=0; j<urls.length; j++){
>+      getTopWin().gBrowser.addTab(urls[j]);
>+	}
>       this._tabsList.clearSelection();
>-    }

    if (urls.length) {
      // SeaMonkey doesn't support gBrowser.loadTabs
      let win = getTopWin();
      for (let j=0; j<urls.length; j++)
        win.gBrowser.addTab(urls[j]);
      this._tabsList.clearSelection();
    }

(Indentation, spacing and bracing style)
(getTopWin is probably not caching the result itself, mconnor might know. Defining a temporary variable outside the for loop and using it inside is the easiest way to play safe.)
(The clearSelection call would otherwise be executed in any case, which would be a logic change.)

>+<!-- For SeaMonkey -->
>+ <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>

  <!-- For SeaMonkey -->
  <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>

(Indentation)

>+        gBrowser.selectedTab = gBrowser.tabContainer.childNodes[i]; // gBrowser.selectTabAtIndex not supported by Seamonkey

        // SeaMonkey doesn't support gBrowser.selectTabAtIndex
        gBrowser.selectedTab = gBrowser.tabContainer.childNodes[i];

(Line length, consistent comments)

>+      gBrowser.selectedTab = gBrowser.addTab(this.WEAVE_TABS_URL, null, null, null, false); // gBrowser.loadOneTab not supported by Seamonkey 

      // SeaMonkey doesn't support gBrowser.loadOneTab
      gBrowser.selectedTab = gBrowser.addTab(this.WEAVE_TABS_URL, null, null, null, false);

(Line length, consistent comments)

>+        currentState =  JSON.parse(Svc.Session.getBrowserState());

        currentState = JSON.parse(Svc.Session.getBrowserState());

(Extra space)

>+        currentState =  JSON.parse(Svc.SessionSM.getBrowserState());

        currentState = JSON.parse(Svc.SessionSM.getBrowserState());

(Extra space)

I'll let mconnor take care of further reviews. This was just for warm-up. :-)
Comment 6 Mike Connor [:mconnor] 2010-05-31 14:13:00 PDT
Comment on attachment 448266 [details] [diff] [review]
This patch gets firefox tabs sync code to work for seamonkey as well.

first off, thanks for the patch, sorry for the delay here.

>diff -r 88dfff4a317e source/chrome/content/fx-tabs.js
>--- a/source/chrome/content/fx-tabs.js	Sat May 22 00:26:53 2010 -0400
>+++ b/source/chrome/content/fx-tabs.js	Sun May 30 17:30:20 2010 +0530
>@@ -75,12 +75,15 @@
>         this._tabsList.removeItemAt(index);
>       }
>     }
>-    if (urls.length) {
>-      getTopWin().gBrowser.loadTabs(urls);
>+    
>+    //Seamonkey doesn't support gBrowser.loadTabs
>+ 	for(let j=0; j<urls.length; j++){
>+      getTopWin().gBrowser.addTab(urls[j]);
>+	}

r- for this, if nothing else, because loadTabs does nice things like focusing the first tab of the set, and is more future-proof against tweaks to this type of behaviour in the app.

if SeaMonkey doesn't have a function, add a backup codepath, and either check explicitly for loadTabs being available, or switch on app ID.  I think I'd prefer the former, as SeaMonkey will hopefully add loadTabs in the future for better compat.

please also use spaces around operators:

for (let j = 0; j < urls.length; j++) {

Also, no tabs!

please take a good look at https://developer.mozilla.org/En/Developer_Guide/Coding_Style

>diff -r 88dfff4a317e source/chrome/content/fx-weave-overlay.js

>-      gBrowser.loadOneTab(this.WEAVE_TABS_URL, null, null, null, false);
>+      gBrowser.selectedTab = gBrowser.addTab(this.WEAVE_TABS_URL, null, null, null, false); // gBrowser.loadOneTab not supported by Seamonkey 

again, I'd prefer to have an explicit separate codepath for Seamonkey here.  We're likely to uplift the UI pieces into Firefox shortly, so it's preferable to have it explicit what is not needed for Firefox, and I'd like to continue to use the best APIs for Firefox.

>diff -r 88dfff4a317e source/modules/engines/tabs.js

>+    //get browser state for Firefox or Seamonkey
>+    switch (Svc.AppInfo.ID) {   	
>+      case FIREFOX_ID:
>+        currentState =  JSON.parse(Svc.Session.getBrowserState());
>+        break;
>+      case SEAMONKEY_ID:
>+        currentState =  JSON.parse(Svc.SessionSM.getBrowserState());
>+        break;
>+    }

don't do this here, see below.

this would also break Fennec, as written.

>diff -r 88dfff4a317e source/modules/util.js
>--- a/source/modules/util.js	Sat May 22 00:26:53 2010 -0400
>+++ b/source/modules/util.js	Sun May 30 17:30:20 2010 +0530
>@@ -894,7 +894,8 @@
>  ["Version", "@mozilla.org/xpcom/version-comparator;1", "nsIVersionComparator"],
>  ["WinMediator", "@mozilla.org/appshell/window-mediator;1", "nsIWindowMediator"],
>  ["WinWatcher", "@mozilla.org/embedcomp/window-watcher;1", "nsIWindowWatcher"],
>- ["Session", "@mozilla.org/browser/sessionstore;1", "nsISessionStore"],
>+ ["Session", "@mozilla.org/browser/sessionstore;1" , "nsISessionStore"],
>+ ["SessionSM", "@mozilla.org/suite/sessionstore;1" , "nsISessionStore"],
> ].forEach(function(lazy) Utils.lazySvc(Svc, lazy[0], lazy[1], lazy[2]));

rather than adding another service, just add:

get _sessionCID() {
  return Svc.AppInfo.ID == SEAMONKEY_ID ? "@mozilla.org/suite/sessionstore;1"
                                        : "@mozilla.org/browser/sessionstore;1";
}

then do:

   ["Session", this._sessionCID , "nsISessionStore"],
Comment 7 Robert Kaiser 2010-05-31 17:59:04 PDT
(In reply to comment #6)
> if SeaMonkey doesn't have a function, add a backup codepath, and either check
> explicitly for loadTabs being available, or switch on app ID.  I think I'd
> prefer the former, as SeaMonkey will hopefully add loadTabs in the future for
> better compat.

Explicitly checking for loadTabs/loadOneTab/whatever is IMHO the place to go as we're in the process of implementing said functions for SeaMonkey 2.1, so using the other functions should actually be SeaMonkey 2.0 compatibility only (if reviews on our side work out alright, which I presume).
I wanted to wait for mconnor reviewing before stating that, but I think the comments there should probably talk of "SeaMonkey 2.0" instead of SeaMonkey - but then, bug 558614 hasn't landed yet for 2.1, though we hope it will.
Comment 8 hariniachala 2010-06-11 00:23:21 PDT
Created attachment 450604 [details] [diff] [review]
patch to get tabs sync to work for seamonkey
Comment 9 hariniachala 2010-06-11 00:25:34 PDT
(In reply to comment #6)
> rather than adding another service, just add:
> 
> get _sessionCID() {
>   return Svc.AppInfo.ID == SEAMONKEY_ID ? "@mozilla.org/suite/sessionstore;1"
>                                         :
> "@mozilla.org/browser/sessionstore;1";
> }
> 
> then do:
> 
>    ["Session", this._sessionCID , "nsISessionStore"],

I could not implement this because I cannot use a variable for Svc.Session
component value in the array and set it depending on Svc.AppInfo.ID because
Svc.AppInfo and Svc.Session are both defined inside the same array.

Therefore as a workaround I changed my previous code so that it would not break
Fennec(please see new patch).

Kindly advise on this.
Comment 10 Philipp von Weitershausen [:philikon] 2010-06-11 16:43:22 PDT
This patch doesn't take the source code reorg (bug 569355) that was landed two days ago into account. Also, we're trying to get rid of switch (Svc.AppInfo.ID) statements in the code module, moving things to preference settings that the different apps (Firefox, Fennec, Seamonkey, etc.) can provide (see bug 570573).
Comment 11 Jens Hatlak (:InvisibleSmiley) 2010-06-17 11:08:16 PDT
With bug 558614 fixed, only gBrowser.selectTabAtIndex is left unimplemented in SM trunk (of the gBrowser functions with fall-back paths in the patch). The API changes probably won't be back-ported to the SM 2.0 branch, though. If there's a chance to have a Firefox Sync release with this bug fixed, branch compatibility would be really nice to have since the SM 2.1 release is still some months away. On the other hand, if the mozilla-central integration will happen soon, those extra code paths could be killed right now.
Comment 12 Jens Hatlak (:InvisibleSmiley) 2010-06-17 13:00:52 PDT
(In reply to comment #11)
> On the other hand, if the mozilla-central integration will happen soon

FYI: <http://blog.mozilla.com/meeting-notes/archives/312> (Sorry for the spam.)
Comment 13 hariniachala 2010-07-04 07:19:57 PDT
Created attachment 455947 [details] [diff] [review]
patch to get tab sync engine to work for seamonkey

This patch takes into account the latest source code reorganization and gets rid of the switch (Svc.AppInfoID) statement.

I have not included the tabbrowser api fixes in this patch. most of that code should work in sm 2.1 when it is released, which means if this patch is approved tab sync will work in sm 2.1. I'll include a separate patch with the alternate code paths for previous sm versions later.

Hope to get a review soon:)
Comment 14 Philip Chee 2010-07-04 08:17:46 PDT
Comment on attachment 455947 [details] [diff] [review]
patch to get tab sync engine to work for seamonkey

> diff -r 1796bf3fe141 ui/firefox/content/tabs.xul
> --- a/ui/firefox/content/tabs.xul	Thu Jul 01 16:44:32 2010 -0700
> +++ b/ui/firefox/content/tabs.xul	Sun Jul 04 19:15:13 2010 +0530
> @@ -13,6 +13,9 @@
>    <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
This doesn't exist in SeaMonkey.

>    <script type="application/javascript" src="chrome://browser/content/places/utils.js"/>
This doesn't exist in SeaMonkey.

>    <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  <!-- For SeaMonkey -->
> +  <script type="application/javascript" 
> +          src="chrome://communicator/content/utilityOverlay.js"/>
Firefox is going to be unhappy since this doesn't exist there.

I think it would be best to fork this file and put it in ui/seamonkey/content/tabs.xul
You'll have to update the relevant makefiles and chrome.manifest.in as well.

> +this.__defineGetter__("_sessionCID", function() {
> +  //sets session CID based on browser type	
Trailing whitespace.

> +  let appInfo = Components.classes["@mozilla.org/xre/app-info;1"].
Prevailing style in this file is to place the member operator "." at the start of the following line. Also See <https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Operators>

> +                getService(Components.interfaces.nsIXULAppInfo);  
Trailing whitespace.

> +  return appInfo.ID == SEAMONKEY_ID ? "@mozilla.org/suite/sessionstore;1"
> +                                    :"@mozilla.org/browser/sessionstore;1";
Space after the ":" please.
Comment 15 hariniachala 2010-07-04 21:59:48 PDT
Created attachment 456015 [details] [diff] [review]
patch to get tab sync engine to work for seamonkey -v2

Created /ui/seamonkey/content/tabs.xul. Using a switch statement in AboutWeaveTabs.js to call the correct tabs.xul file depending on AppID. 

An alternative to the switch statement would be to explicitly overlay (dynamic overlaying doesn't work here) /ui/firefox/content/tabs.xul with an overlay in /ui/seamonkey/content/tabs.xul that contains the script line,

  <script type="application/javascript" 
          src="chrome://communicator/content/utilityOverlay.js"/>

Corrected code styling errors.
Comment 16 Philip Chee 2010-07-05 01:32:34 PDT
> -    let ch = ios.newChannel("chrome://weave/content/firefox/tabs.xul", null, null);
> +    let ch = null;
> +    switch (Svc.AppInfo.ID) {
> +      case SEAMONKEY_ID:
> +        ch = ios.newChannel(""chrome://weave/content/seamonkey/tabs.xul", null, null);
> +        break;
> +      default:
> +        ch = ios.newChannel("chrome://weave/content/firefox/tabs.xul", null, null);
> +        break;
> +    }

No need. In the chrome.manifest.in do something like:

# Seamonkey only
.....
override chrome://weave/content/firefox/tabs.xul chrome://weave/content/seamonkey/tabs.xul application={92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}

> diff -r 1796bf3fe141 ui/seamonkey/content/tabs.xul
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/ui/seamonkey/content/tabs.xul	Mon Jul 05 09:50:23 2010 +0530

Use hg copy to preserve hg blame/history.

> +  <script type="application/javascript;version=1.8"
> +          src="chrome://weave/content/firefox/tabs.js"/>
> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  <script type="application/javascript" 
> +          src="chrome://communicator/content/utilityOverlay.js"/>

In the original there are dependencies on things in chrome://browser/content/utilityOverlay.js and chrome://browser/content/places/utils.js. Check if there are suitable replacements in chrome://communicator/content/utilityOverlay.js and chrome://communicator/content/history/utils.js.

> chrome://communicator/content/history/utils.js.

Note: Place bookmarks hasn't landed yet in SeaMonkey 2.1 (but will very soon) after which there will be a consolidation with SM Places history. We will then likely end up with something like chrome://content/communicator/places/utils.js. In the mean time I think the bookmarking menu items should be removed since there is currently no support for places bookmarks. KaiRo, your opinion?
Comment 17 Robert Kaiser 2010-07-05 06:30:52 PDT
(In reply to comment #16)
> In the mean time I think the
> bookmarking menu items should be removed since there is currently no support
> for places bookmarks. KaiRo, your opinion?

Not sure if complete removal is good, as we'd love to land places bookmarks ASAP, just review is rather slow, unfortunately (is that a common topic?) but we ultimately should have this work and places bookmarks both ship in 2.1 (and yes, Sync will be integrated on our side, I just filed bug 576970 for doing that).
Comment 18 hariniachala 2010-07-07 12:37:40 PDT
Created attachment 456318 [details] [diff] [review]
patch to get tab sync engine to work for seamonkey -v3
Comment 19 Philip Chee 2010-07-07 20:54:37 PDT
> +<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
Does not exist in Suite. Use ""chrome://communicator/skin/" instead.

> +  <richlistbox context="tabListContext" id="tabsList" seltype="multiple" align="center" flex="1"
id should be the first attribute.

> +      <hbox id="headers" align="center">

> +      <image src="chrome://weave/skin/firefox/sync-32x32.png" height="32" width="32"/>
> +      <label id="tabsListHeading"
> +             value="&tabs.otherComputers.label;"/>
> +      <spacer flex="1"/>
> +      <textbox type="search"
> +               emptytext="&tabs.searchText.label;"
> +               oncommand="RemoteTabViewer.filterTabs(event)"/>
Wrong indentation in the above block.

> +      </hbox>

As an aside, Mike, why don't most of the files in /ui/firefox/content/ have a licence header?
Comment 20 hariniachala 2010-07-08 00:17:47 PDT
Created attachment 456395 [details] [diff] [review]
patch to get tab sync engine to work for seamonkey -v3'
Comment 21 Jens Hatlak (:InvisibleSmiley) 2010-07-19 04:07:38 PDT
(In reply to comment #11)
> With bug 558614 fixed, only gBrowser.selectTabAtIndex is left unimplemented in
> SM trunk (of the gBrowser functions with fall-back paths in the patch).

xref bug 579845
Comment 22 hariniachala 2010-08-09 22:06:03 PDT
Created attachment 464293 [details] [diff] [review]
Tabs sync for Seamonkey
Comment 23 hariniachala 2010-08-11 13:54:08 PDT
Created attachment 464938 [details] [diff] [review]
Bump seamonkey max version to 2.1b1

Tabs sync will only work in seamonkey for 2.1a3 and after because of bug 558614
Comment 24 Igor Velkov 2010-08-11 21:32:02 PDT
Harini, if current patch will not final, then increase, please, sm version compatibility up to 2.1a3pre
Comment 25 Mike Connor [:mconnor] 2010-08-16 12:02:24 PDT
Comment on attachment 464293 [details] [diff] [review]
Tabs sync for Seamonkey

once again I'm sad that seamonkey can't just use the same CID, but this'll do.
Comment 26 Mike Connor [:mconnor] 2010-08-16 12:07:50 PDT
Comment on attachment 464938 [details] [diff] [review]
Bump seamonkey max version to 2.1b1

pretty sure this is covered in the bookmark sync bug already.
Comment 27 Robert Kaiser 2010-08-17 06:48:47 PDT
Pushed the main patch as http://hg.mozilla.org/services/fx-sync/rev/cc8bf4544b57

Note You need to log in before you can comment on or make changes to this bug.