Last Comment Bug 421235 - FUEL: Add support for new bookmark roots
: FUEL: Add support for new bookmark roots
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: Firefox 3 beta5
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-05 22:47 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2008-03-16 07:46 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds nsIBookmarkRoots and tests (7.47 KB, patch)
2008-03-05 22:57 PST, Mark Finkle (:mfinkle) (use needinfo?)
gavin.sharp: review+
Details | Diff | Splinter Review
v2 - same as first patch with review changes (8.56 KB, patch)
2008-03-07 17:49 PST, Mark Finkle (:mfinkle) (use needinfo?)
mark.finkle: review+
dsicore: approval1.9+
Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2008-03-05 22:47:29 PST
When originally landed, FUEL supported a single bookmark root (menu) because that's the only root that existed. Not long after that, several new bookmark roots were added: tags, toolbar and unfiled.

We should add support to FUEL to get access to the new roots.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-05 22:57:17 PST
Created attachment 307651 [details] [diff] [review]
Adds nsIBookmarkRoots and tests

This patch adds a nsIBookmarkRoots interface to hold the various bookmark roots. Each root getter simply returns a nsIBookmarkFolder for the specific root.

Tests are added to perform some basic "is the root there" tests. Since each root returns a nsIBookmarkFolder the main testing code is only executed once on the "menu" bookmark root.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-07 13:45:37 PST
Comment on attachment 307651 [details] [diff] [review]
Adds nsIBookmarkRoots and tests

>Index: browser/fuel/public/fuelIApplication.idl

>+function BookmarkRoots() {
>+  _menu = null;
>+  _toolbar = null;
>+  _tags = null;
>+  _unfiled = null;

r=me with these removed and these:

>+    if (this._menu == null)

turned into just |if (!this._menu)| and

http://lxr.mozilla.org/seamonkey/source/browser/base/content/test/browser_getshortcutoruri.js

fixed to use something other than just Application.bookmarks

and with the browser test suite run and other tests fixed as needed!
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-07 17:49:31 PST
Created attachment 308090 [details] [diff] [review]
v2 - same as first patch with review changes

Here is the patch to land with changes per gavin's review. Pulling r+ forward.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-07 17:55:08 PST
Comment on attachment 308090 [details] [diff] [review]
v2 - same as first patch with review changes

This patch addresses some important missing functionality with the FUEL bookmark wrappers. There is a chance it will break some Fx3.0b extensions, but the changes are needed to allow access to all the bookmark roots. There is no clean way to not break any current consumers.

I have a bug filed to scan the AMO source for possible breakage and I'll be blogging about the change when it lands.

There are currently no internal Firefox consumers that are affected.
Comment 5 Damon Sicore (:damons) 2008-03-10 16:15:19 PDT
Comment on attachment 308090 [details] [diff] [review]
v2 - same as first patch with review changes

a1.9+=damons
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-16 07:46:43 PDT
Checking in browser/base/content/test/browser_getshortcutoruri.js;
/cvsroot/mozilla/browser/base/content/test/browser_getshortcutoruri.js,v  <--  browser_getshortcutoruri.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/fuel/public/fuelIApplication.idl;
/cvsroot/mozilla/browser/fuel/public/fuelIApplication.idl,v  <--  fuelIApplication.idl
new revision: 1.12; previous revision: 1.11
done
Checking in browser/fuel/src/fuelApplication.js;
/cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v  <--  fuelApplication.js
new revision: 1.27; previous revision: 1.26
done
Checking in browser/fuel/test/browser_Bookmarks.js;
/cvsroot/mozilla/browser/fuel/test/browser_Bookmarks.js,v  <--  browser_Bookmarks.js
new revision: 1.3; previous revision: 1.2
done

Note You need to log in before you can comment on or make changes to this bug.