Closed
Bug 567583
Opened 14 years ago
Closed 14 years ago
Get tabs sync to work for Seamonkey
Categories
(Firefox :: Sync, enhancement)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
1.5
People
(Reporter: hariniachala, Assigned: hariniachala)
References
Details
Attachments
(2 files, 8 obsolete files)
1.65 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
404 bytes,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #446912 -
Flags: review?(mconnor)
Assignee | ||
Comment 2•14 years ago
|
||
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)
Comment 3•14 years ago
|
||
(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.
Assignee | ||
Comment 4•14 years ago
|
||
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 5•14 years ago
|
||
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. :-)
Updated•14 years ago
|
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•14 years ago
|
||
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-
Comment 7•14 years ago
|
||
(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.
Updated•14 years ago
|
Target Milestone: --- → Future
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #448266 -
Attachment is obsolete: true
Attachment #450604 -
Flags: review?(mconnor)
Assignee | ||
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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.)
Assignee | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
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-
Assignee | ||
Comment 15•14 years ago
|
||
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)
Comment 16•14 years ago
|
||
> - 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•14 years ago
|
||
(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).
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #456015 -
Attachment is obsolete: true
Attachment #456318 -
Flags: review?(mconnor)
Attachment #456015 -
Flags: review?(mconnor)
Comment 19•14 years ago
|
||
> +<?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?
Assignee | ||
Comment 20•14 years ago
|
||
Attachment #456318 -
Attachment is obsolete: true
Attachment #456395 -
Flags: review?(mconnor)
Attachment #456318 -
Flags: review?(mconnor)
Comment 21•14 years ago
|
||
(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
Assignee | ||
Comment 22•14 years ago
|
||
Attachment #456395 -
Attachment is obsolete: true
Attachment #456395 -
Flags: review?(mconnor)
Assignee | ||
Comment 23•14 years ago
|
||
Tabs sync will only work in seamonkey for 2.1a3 and after because of bug 558614
Attachment #464938 -
Flags: review?(mconnor)
Assignee | ||
Updated•14 years ago
|
Attachment #464293 -
Flags: review?(mconnor)
Comment 24•14 years ago
|
||
Harini, if current patch will not final, then increase, please, sm version compatibility up to 2.1a3pre
Comment 25•14 years ago
|
||
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 26•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 27•14 years ago
|
||
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
Updated•6 years ago
|
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.
Description
•