Closed Bug 567583 Opened 14 years ago Closed 14 years ago

Get tabs sync to work for Seamonkey

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: hariniachala, Assigned: hariniachala)

References

Details

Attachments

(2 files, 8 obsolete files)

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
Attachment #446912 - Flags: review?(mconnor)
resubmitting patch with two separate constants defined for nsISessionStore component URIs in modules/util.js for Firefox and Seamonkey.
Attachment #446912 - Attachment is obsolete: true
Attachment #446938 - Flags: review?(mconnor)
Attachment #446912 - Flags: review?(mconnor)
(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.
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..
Attachment #446938 - Attachment is obsolete: true
Attachment #448266 - Flags: review?(mconnor)
Attachment #446938 - Flags: review?(mconnor)
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. :-)
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
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"],
Attachment #448266 - Flags: review?(mconnor) → review-
(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.
Target Milestone: --- → Future
Attachment #448266 - Attachment is obsolete: true
Attachment #450604 - Flags: review?(mconnor)
(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.
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).
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.
(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.)
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:)
Attachment #450604 - Attachment is obsolete: true
Attachment #455947 - Flags: review?(mconnor)
Attachment #450604 - Flags: review?(mconnor)
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.
Attachment #455947 - Flags: feedback-
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.
Attachment #455947 - Attachment is obsolete: true
Attachment #456015 - Flags: review?(mconnor)
Attachment #455947 - Flags: review?(mconnor)
> -    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?
(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).
Attachment #456015 - Attachment is obsolete: true
Attachment #456318 - Flags: review?(mconnor)
Attachment #456015 - Flags: review?(mconnor)
> +<?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?
Attachment #456318 - Attachment is obsolete: true
Attachment #456395 - Flags: review?(mconnor)
Attachment #456318 - Flags: review?(mconnor)
(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
Attachment #456395 - Attachment is obsolete: true
Attachment #456395 - Flags: review?(mconnor)
Tabs sync will only work in seamonkey for 2.1a3 and after because of bug 558614
Attachment #464938 - Flags: review?(mconnor)
Attachment #464293 - Flags: review?(mconnor)
Harini, if current patch will not final, then increase, please, sm version compatibility up to 2.1a3pre
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.
Attachment #464293 - Flags: review?(mconnor) → review+
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.
Attachment #464938 - Flags: review?(mconnor)
Keywords: checkin-needed
Pushed the main patch as http://hg.mozilla.org/services/fx-sync/rev/cc8bf4544b57
Assignee: nobody → hariniachala
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Future → 1.5
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.