Bookmarks tree widgets need to be converted to rdfliner

VERIFIED FIXED in mozilla0.9.7


Bookmarks & History
18 years ago
14 years ago


(Reporter: Cathleen, Assigned: Blake Ross)



Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [br][nav+perf])


(7 attachments)



18 years ago
waterson checked in new RDF outliner tree widget ( bug 71530 )
need to convert bookmark to use the new outlier.


18 years ago
Blocks: 73948
I'm currently working on getting a test/demo of outliner bookmarks up and 

I had a discussion with waterson this evening, and here are some of the issues 
we identified and some proposed solutions:

- The Outliner Builder is a generic class that is not tied to any particular
  RDF datasource. As such, it leaves stubbed certain nsIOutlinerView methods
  that are of use in some RDF outliner situations, like APIs relating to
  inline editing, cell cycling and so on. 

- The Outliner Builder is being modified to accept observers 
  (|nsIXULOutlinerBuilderObserver|) which are notified of various events that
  occur. Clients can then implement these observers to handle such events. 

  Provision has been made for the attachment of multiple observers, despite
  potentially tricky semantics with some APIs like IsEditable and SetCellText.
  A single observer claiming support for Inline Editing will be enough to
  produce a text field, and the changed value will then be handed to 
  any and all observers that claim to support it. That way, a name change
  can be propagated not only to the local bookmark store, but perhaps to 
  an online public bookmark repository if a third party plugin for such a
  system is installed. 

Other Issues:
- Inline editing will need to be implemented in the Outliner's XBL binding. 
- Drag and Drop feedback needs to be provided (pinkerton)
- Separators need to be provided (hyatt)

Created attachment 29693 [details] [diff] [review]
initial (rough) patch to get this going.
Created attachment 29694 [details] [diff] [review]
patch for xpfe/components/bookmarks, includes a demo XUL file
OK, with this initial patch, the outliner builder should now theoretically 
support n nsIXULOutlinerBuilderObservers. I've provided methods on the observer 
interface to the methods that clients of the outliner builder are likely to 
want to handle. Please let me know if there are others. 

To test: apply the patch in content/xul/templates and in 
xpfe/components/bookmarks/resources. Make chrome in 
xpfe/components/bookmarks/resources, load the browser and type: 
chrome://communicator/content/bookmarks/oTest.xul in the location field. A XUL 
document with a bookmarks list should display. Outliner bookmarks!

If you expand or collapse a row, you should see a message printed to the 
console. This is a method implemented as a js object defined in the XUL file 
which is attached to the outliner builder as an observer. As the demo is 
currently pretty simple, this is the only notification you can easily see. 
Nevertheless, it's the first step to supporting other things!

Comment 5

18 years ago
The first patch (to nsXULOutlinerBuilder.cpp, 04/04/01 05:31) looks wonderful.
Two nits:

  - In AddObserver(), use NS_NewISupportsArray() instead of thunking
    through the component manager. (We link against XPCOM, so no need
    to componentize here.) Also, make sure that you actually *got one
    back*, and return NS_ERROR_OUT_OF_MEMORY if you didn't.

  - In DocumentWillBeDestroyed(), just call Clear() on mObservers,
    instead of removing each element one-by-one.

Do that, and r/sr=waterson for the first patch, whichever helps.

cc'ing alecf: looks like ben is taking a first crack at doing bookmarks...
Created attachment 29782 [details] [diff] [review]
updated patch with requested changes
Created attachment 29783 [details] [diff] [review]
how about a patch that actually compiles.
Created attachment 29784 [details] [diff] [review]
another patch that checks for mObservers before trying to remove an observer from it.
+  // Called when an item is opened or closed. 
+  void onToggleOpenState (in long index);

You should use
   * Doc comments
here, so that they propagate to headers and can show up in doxygen some day.

+  // Called when selection in the outliner changes
+  void onSelectionChanged();

Is it worth aping (or even inheriting from) the existing selection listeners,
such as passing in the current selection state?  Seems like an easy optimization

Also, could these notifications not take an nsIXULOutlinerBuilder argument, so
that objects could observe multiple outliners?  It's not too onerous to whip up
some placeholders/multiplexers in JS, but the C++ people will suffer.

+  void onPerformAction(in wstring action);

(Our commands are Unicode?  Wow.)

+    /** 
+     * The builder observer.
+     */
+    nsCOMPtr<nsISupportsArray> mObservers;

Comment is singular, but variable type and naming indicates plurality.  One must
be wrong!

+nsXULOutlinerBuilder::AddObserver(nsIXULOutlinerBuilderObserver* aObserver)
+    nsresult rv = NS_OK;  
+    if (!mObservers) 
+        rv = NS_NewISupportsArray(getter_AddRefs(mObservers));

You don't check the result, though you bother to store it.  Also, there's no
point in initializing rv to NS_OK, when you can never read it without setting first.

+    if (mObservers && mObservers->IndexOf(aObserver) == -1) 
+        rv = mObservers->AppendElement(aObserver);

...then you check if the preceding call worked!  How about propagating rv from
NS_NewISupportsArray, and then losing the mObservers check there?

Also, the IndexOf check indicates that this


will result in foo not being registered.  That might be very surprising to one
of the code paths that registered foo!  At the least, document this behaviour.

Other than that, very nice.  You write pretty code; seems a shame to waste it in
content/xul. =)

Created attachment 29791 [details] [diff] [review]
once more, for the interested, addressing shaver's comments.
This patch checked has been checked in. Thanks, waterson & shaver! Now, to try 
and get some functionality up. 



18 years ago
Depends on: 75404


17 years ago
Depends on: 75572
Keywords: nsbeta1
Summary: bookmark needs to use new RDF outliner → Bookmarks tree widgets need to be converted to rdfliner
Target Milestone: --- → mozilla0.9.2


17 years ago
Keywords: perf
nav triage: we will not do the conversion to rdfliner in this release cycle. 
moving to future. 
Keywords: nsbeta1 → nsbeta1-
Target Milestone: mozilla0.9.2 → Future


17 years ago
Whiteboard: [br]

Comment 13

17 years ago
just a note as i'm disappointed to see this bug is moved to "future"... 
right now bookmarks are too slow. 
i hoped this wonderful "outliner" thing i saw in mail/news could benefit to the
are you sure this is not urgent ?

Comment 14

17 years ago
Ben created a mini branch for this and plans on starting work on it soon; 
retargetting to 0.9.2 so this shows up on his list.
Target Milestone: Future → mozilla0.9.2


17 years ago
Blocks: 52144
I consider this one VERY important.  I cannot effectively use Mozilla with my
very large bookmarks file.
nav triage: we're not going to be able to do this for mozilla0.9.2. In fact we 
do not want to do this for m0.9.2 because we do not wish to destabilize the 
existing bookmarks at this point. moving to mozilla1.0 at least. 
Priority: -- → P4
Target Milestone: mozilla0.9.2 → mozilla1.0


17 years ago
Whiteboard: [br] → [br][nav+perf]


17 years ago
Blocks: 91351
Moving this forward to mozilla0.9.5. Ben is working on this on a branch, we hope 
to land this in mozilla0.9.5. 
Target Milestone: mozilla1.0 → mozilla0.9.5


17 years ago
Blocks: 92860


17 years ago
Blocks: 52149


17 years ago
No longer blocks: 92860

Comment 18

17 years ago
I hope this is landed soon I am getting *numerous* complaints based on this.

Comment 19

17 years ago
I guess mozilla 0.9.5 isn't realistic for this one considering that bug 75572
(which this one depends on) is targeted at mozilla 1.0? Please feel free to
prove me wrong:-)

Comment 20

17 years ago
The "manage bookmarks" is my daily most problematic part of mozilla.

I was just able to produce (by drag-dropping bookmarks around) something in my
bookmarks which crashes mozilla while I open a bookmarks folder).

Drag-and-drop often stops working, etc... Things are also slow even on fastest

This needs to be done ASAP so it can be debugged well before 1.0.

Adding nsCatFood.
Keywords: nsCatFood
Should we push this to 0.9.6 and try to land it there?  Ben, are you actively
working on this?


17 years ago
Keywords: mozilla0.9.6

Comment 22

17 years ago
0.9.5 is out the door. bumping TM up by one.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Paul Chen has volunteered to take this ;) 
Assignee: ben → pchen
And now, because everyone gave everything to Paul, we're load balancing again. 
Over to blake...
Assignee: pchen → blakeross


17 years ago
Blocks: 65957


17 years ago
Blocks: 70347


17 years ago


17 years ago
Blocks: 83901


17 years ago
Blocks: 90829

Comment 25

17 years ago
bookmarksliner is landing in --> 0.9.7.
Target Milestone: mozilla0.9.6 → mozilla0.9.7


17 years ago
Blocks: 93693


17 years ago
Blocks: 93920


17 years ago
Blocks: 77496


17 years ago
Blocks: 52298


17 years ago
Blocks: 107591

Comment 26

17 years ago
Created attachment 60383 [details] [diff] [review]


17 years ago
No longer blocks: 107591

Comment 27

17 years ago
Okay, marking this fixed. There are still some issues. I know. Really. Most of 
them are not bugs in bookmarksliner code. Don't file bugs just yet.
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 28

17 years ago
This patch toasts the triple license in bookmarks.js.

Comment 29

17 years ago
Yeah, that's because Ben was working on this on a really old branch. I'll fix 
it tomorrow.
mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).


[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.