Closed Bug 692731 Opened 14 years ago Closed 11 years ago

Fork nsISidebar

Categories

(SeaMonkey :: Sidebar, defect)

Product:
SeaMonkey
Component:
Sidebar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.7

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file)

Proposed patch
4.79 KB, patch
mnyromyr
: review+
Details | Diff | Splinter Review
Firefox wants to modify nsISidebar so we're probably better off with a copy. As per bug 691647 we could switch from wstring to a better string type. We could also merge nsISidebar and nsISidebarExternal at the same time. The new interface would have to have a new name of course.
(In reply to neil@parkwaycc.co.uk from comment #0) > Firefox wants to modify nsISidebar Any details about what they want to change?
> The new interface would have to have a new name of course. Would we still have Components.classes["@mozilla.org/sidebar;1"] and Components.interfaces.nsISidebar ?
(In reply to Philip Chee from comment #2) > > The new interface would have to have a new name of course. > Would we still have Components.classes["@mozilla.org/sidebar;1"] This exists because we create it in SuiteCommon.manifest > Components.interfaces.nsISidebar ? This exists because it is created by toolkit. Bug 691647 will change the definition though, so we need to fork it to retain our methods.
nsIMonkeySidebar ? Ook?
(In reply to neil@parkwaycc.co.uk from comment #0) > Firefox wants to modify nsISidebar so we're probably better off with a copy. Can you elaborate on why a fork is a better idea here? [Rather than, say, simply extending it if needed]? Especially given the descriptions currently in Bug 691647?
Diverging from Gecko when it comes to web-exposed features doesn't seem like an ideal situation for SeaMonkey to be in. Presumably you want to keep it because there are users of it that you care about? If so, you should probably at least mention that in bug 691647.
Unfortunately, we don't know how strong usage really is. But given the (non-)amount of pages using addPanel or even providing old-style sidebars, I'm not quite sure if keeping the removed methods is really useful. sidebar.addPanel etc. were Mozilla-only for all these years and there's only a handful of pages left using it after the decline of Netscape. (Note that even the dmoz pages we link to are devoid of sidebars!) But if we want to keep the feature, we should derive from the base interface, not forking it. We need a restructuring of our sidebar more than ever.
I believe that it is possible to create/register a component that overrides the toolkit one but mostly forwards methods to the toolkit implementation except for whatever custom stuff we want to do. Advantage of doing it this way is that we only need to maintain the code for the SeaMonkey specific member methods, passing through everything else to the toolkit nsISidebar. I'm not sure about how to go about the IDL however. Phil
Blocks: 613971
IMHO we don't need a API function in website context, that only works for SeaMonkey. This may cause security problems in future and if Firefox doesn't support this feature, there will get even less websites, which still support it. Somehow this discussion remembers me on this: http://www.bpic.co.uk/articles/deadhorse.htm Lets dismount in this case!
No longer blocks: 613971
As Philip Chee posted in the newsgroup thread, there is an alternative API in firefox for this: http://mxr.mozilla.org/mozilla1.8.0/source/browser/base/content/browser.js#4933 I guess it is a better idea to forget about forking nsISidebar and creating a new bug about adding this one to a (still to be created) new SeaMonkey sidebar GUI? If Neil accepts this suggestion, I'll wontfix this one and create a new bug for creating the new API.
We're going to get rid of that code too, FWIW.
Commented here: https://bugzilla.mozilla.org/show_bug.cgi?id=691647#c11 IMHO "some" way should be kept to allow websites to offer sidebar additions. If nothing like that is left, then there is no more use for the sidebar for the webmaster. Noone will provide a long instruction to users where they tell the user many steps he has to follow to finally get a website, designed to open in the sidebar, to where it should be.
OK. As it seems like the Firefox developers want to kill sidebar at all, I think we have to decide on our own. As I think that SeaMonkey more or less is the open source counterpart to Opera, I think we should add support for the "Opera way" of adding sidebars. This would just mean code addition to our navigator.js and no need to fork code. I could create the bug and write up a first version of this new code, which would trigger the old sidebar crap^H^H^Hode. No need to maintain something that is different from any of the other available browsers.
Given the current summary of this bug, I feel this whole discussion is a bit out of place. IMO we should either adapt the summary or close the bug, depending on whether we actually want to keep the web content facing API or not. To answer that, let me ask: Is anyone (with some authority) actually in favor of keeping it? If so, why?
In bug 691647 comment #12 Gavin helpfully revealed that Firefox is planning on removing the sidebar entirely. This would explain why they don't need an API. Unless we remove the sidebar, I suggest we retain some sort of API for it, although I realise we may not necessarily want to retain our current API. Note that this does not bear any relation to our current back end code; any improvements there would be orthogonal to this bug.
(In reply to neil@parkwaycc.co.uk from comment #15) > In bug 691647 comment #12 Gavin helpfully revealed that Firefox is planning > on removing the sidebar entirely. This would explain why they don't need an > API. I revealed that in bug 691647 comment #0! :) But the API isn't the only way to trigger new sidebars, AFAIK you can still manually set bookmarks as "load in sidebar" otherwise.
(In reply to Gavin Sharp from comment #16) > (In reply to comment #15) > > In bug 691647 comment #12 Gavin helpfully revealed that Firefox is planning > > on removing the sidebar entirely. > I revealed that in bug 691647 comment #0! Indeed. Sorry for overlooking it.
(In reply to Jens Hatlak from comment #14) > Given the current summary of this bug, I feel this whole discussion is a bit > out of place. I don't think so. This bug is about forking something, which also means someone will have to keep it up to date. If there are good reason against a fork, I think it's a good idea to discuss them here. I'll try to port over the Opera API for sidebars, but this will be a separate bug. Will post bug number here as soon as I have it.
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #14) > Is anyone (with some authority) actually in favor of keeping it? If so, why? I, at least, am not. If anyone is: speak now! I mean, it's a nifty feature, but it didn't took off. Of course we could still keep it (including potential issues) until our World Domination is complete, but that somehow doesn't sound pretty useful in the short term. ^_^
(In reply to Jens Hatlak from comment #14) > Is anyone (with some authority) actually in favor of keeping it? If so, why? I'm in favour of keeping the sidebar ;-) And this is the only API that we have. (Well, apart from the inscrutable code that DOM Inspector users.) Of course, should someone ever implement a better API, then we won't need this one.
(In reply to neil@parkwaycc.co.uk from comment #20) > (In reply to Jens Hatlak from comment #14) > > Is anyone (with some authority) actually in favor of keeping it? If so, why? > I'm in favour of keeping the sidebar ;-) And this is the only API that we > have. "This is the only API that we have" is not really a reason for keeping it, imo. It adds maintenance burden, increases Sec Vuln Area of attack. And is a divergence from the generic API gecko exposes. Frankly, if "we have this now" is your only real reason for keeping it, I say we drop it.
Ahem. As the author of xSidebar I vote to keep the API. Of course nobody listens to me anyway.
Maybe we should first agree upon what "the API" actually contains? I feel that we all don't mean the same here … - Just nsISidebar, as reflected into the window object of a web page? - Certain objects/methods/stuff found in sidebarOverlay? - Side entrances like bookmarks?
I also think we have to keep the sidebar (but really have to rewrite it from scratch ;) ). As noted somewhere else, I think the counterpart to SeaMonkey is Opera (just as the counterpart to Firefox seems to be Google Chrome) and Opera also has a sidebar. But I don't think we should keep the old API. It *will* get obsolete! We not longer need it in websites as SeaMonkey will be the only who supports it and there are already much more pages supporting the "Opera way" of adding sidebars. If SeaMonkey will be the only who still uses it, then this means we can drop it! SeaMonkey is *very* small if compared to Firefox. Websits, which still offer a sidebar using "addPanel", will drop that support. They won't keep it just for SeaMonkey! And for the record: I've started to port the "Opera way" to SeaMonkey, but I still have a few problems with that. As soon as I have something that doesn't suck, I'll publish this in a new bug. And we no longer need the old API in privileged javascript code (addons, ...) as in future, I think, only two ways are needed to add sidebars: - Broadcaster based API (borrowed from Firefox and already implemented in SM) - Places based API (Add bookmark with "open in sidebar" flag) which, in future, can be triggerd using the "Opera way" using the <a href="" ref="sidebar> syntax. @Philip Chee: Can you give a good reason for keeping it? Firefox *will* drop it and if we replace our API with the one used in Opera, I think we have access to a much bigger list of sidebars.
(In reply to Manuel Reimer from comment #24) > But I don't think we should keep the old API. It *will* get obsolete! Indeed. But only when we're ready to obsolete it, which we're not quite, yet.
(In reply to neil@parkwaycc.co.uk from comment #25) > (In reply to Manuel Reimer from comment #24) > > But I don't think we should keep the old API. It *will* get obsolete! > Indeed. But only when we're ready to obsolete it, which we're not quite, yet. So, in the end, we have two problems here: - We desperately need to rewrite the sidebar code, for various reasons. This will take a non-zero amount of time, though marked as "ASAP". - Our currently supported way has been cut off *now* and the rest may be killed by the powers that be in the midterm. Forking the interface is the easiest way to maintain our featureset *now* until the replacement is ready, without doing strange to-be-disposed hacking on our current sidebar code. Forking the interface makes us independent of Firefox' feature deleteism until we're ready to replace it. Let's do the fork now and discuss the rewrite's corner stones in the newsgroup.
Attached patch Proposed patch — — Splinter Review
Doing it this way avoids breaking xSidebar, which calls getService(nsISidebar)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #569790 - Flags: review?
Attachment #569790 - Flags: review? → review?(mnyromyr)
Just for your Information: Bug 697799 contains a patch for a working implementation of the Opera API for sidebars.
Attachment #569790 - Flags: review?(mnyromyr) → review+
Comment on attachment 569790 [details] [diff] [review] Proposed patch Pushed changeset bd69b3204891 to comm-central.
Looks like I forgot to resolve this.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to neil@parkwaycc.co.uk from comment #30) > Looks like I forgot to resolve this. You also forgot to set the Milestone. I _think_ I found the right one for comm-central changeset bd69b3204891 but please check.
Target Milestone: --- → seamonkey2.7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: