613974 - "Load this bookmark in the sidebar" checkbox for bookmarks is not available/working in SeaMonkey [browserWin.openWebPanel is not a function]

Status: RESOLVED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[Manuel Reimer]

Part of the places bookmark system, as ported from Firefox, is a checkbox which allowes to open a bookmark into sidebar.

IMHO it's a pretty bad idea to even try to implement this into the current sidebar. The current way of adding sidebars from websites (store them into panels.rdf) should be dropped at all and all sidebars, added from websites, should be bookmarks (just as Firefox does). To be able to do this, sidebar has to be replaced with new code (maybe xsidebar) first.

Comment 1

[Robert Kaiser]

So, to be clear, bug 399310 hasn't enabled this to work? Why not? Isn't this way of opening a web page as a sidebar also part of the "Firefox Sidebar API"?

Comment 2

[Philip Chee]

There are other parts of the API that need to be ported. c.f. xSidebar.

Comment 3

[Manuel Reimer]

Bug 339310 was about adding the "broadcaster based" way of adding sidebars.

Addons add an overlay to the browser XUL and an entry for their sidebar to the "mainBroadcasterSet". In Firefox this causes the sidebar to be available via "View -> Sidebar". On SeaMonkey, it now causes the sidebar to be available via the current "Sidebar customize" stuff (RDF).

See also: https://developer.mozilla.org/en/Creating_a_Firefox_sidebar#overlay_xul

Places bookmarks are a totally difficult thing and are nearly impossible to add with the current sidebar code whithout causing even more confusion as already caused by the current sidebar design.

Comment 5

[Jez]

My vote is to remove this functionality altogether from Seamonkey as it doesn't work so nobody is using it, and it doesn't even seem like a good idea anyway.  Any objections?

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Jeremy Morton from comment #5)
> My vote is to remove this functionality altogether from Seamonkey as it
> doesn't work so nobody is using it, and it doesn't even seem like a good
> idea anyway.  Any objections?

I support this move. It never worked, so it should not have been there in the first place. If anyone wants to have this functionality, they should implement it - but that can easily be done after the removal, too. Changing summary to allow this bug to go either way.

Jeremy, do you want to take this? Feel free to go ahead.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: bug613974.patch

Not taking yet, but I think it's really time to get rid of the broken UI element that we've been dragging along for so long. The attached patch also cleans up a bit behind the scenes (tried not to touch APIs) but if it's preferred to only remove the XUL element and/or l10n, fine with me. Just please let's not leave it as is any longer, it's really horrible UX.

This is what I used to check for occurrences:
.../suite # grep -riE "loadInSidebar|load_in_sidebar|loadbookmarkinsidebar" *

Comment 8

[[e-mail]]

Comment on attachment 8442993 [details] [diff] [review]
bug613974.patch

Seems reasonable from code inspection.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

I've lately run into issues building SM on both Windows and Linux so I cannot verify myself whether my patch is safe. Philip (or anyone else reading this), can you apply the patch on this bug and check for obvious regressions? It should only remove the "Load this bookmark in the sidebar" checkbox; anything else (BM, Bookmarks Toolbar etc.) should still work as in a build without the patch.

Comment 10

[Philip Chee]

Comment on attachment 8442993 [details] [diff] [review]
bug613974.patch

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #9)
> I've lately run into issues building SM on both Windows and Linux so I
> cannot verify myself whether my patch is safe. Philip (or anyone else
> reading this), can you apply the patch on this bug and check for obvious
> regressions? It should only remove the "Load this bookmark in the sidebar"
> checkbox; anything else (BM, Bookmarks Toolbar etc.) should still work as in
> a build without the patch.

I took this for a spin and I didn't spot any obvious regressions. f+

Comment 11

[Edmund Wong (:ewong)]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/f6c40188e2d4