321219 - Separate places backend into toolkit

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Benjamin Smedberg]

per email, since we're getting rid of the mork history backend from the toolkit, we should replace it: the plan is to split the places backend into toolkit/ and keep the FE in browser/: this meansthat the toolkit will include all of the globalhistory/annotations/favicons backend inherent in places.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

I'm getting more and more suspicious of putting anything into toolkit that isn't part of xre. What's the reason for doing this?

Comment 2

[Benjamin Smedberg]

Ben, I don't know what your definition of "XRE" is, but the goal of this was to include the places backend in XULRunner so that other apps would have a replacement default implementation of a history backend.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

I wonder how hard would it be to throw together a braindead simple implementation of the frozen history interface. I think that'd be preferable for other apps, rather than forcing them to carry this weight. It also makes the code location etc and module-ness of this component for fx somewhat cleaner. 

Comment 4

[Benjamin Smedberg]

I don't think that would be preferable; I understand that there are some XULRunner consumers who don't care about a history impl at all, but in terms of the power provided by places (even as a mostly-nonfrozen API) I think it would be a good addition. And there is a pretty clear delineation of the FF frontend of places and the backend providers.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Let me rephrase - I don't want the application specific functionality provided by nsNavHistory* to be located in toolkit. I think this is the wrong breakdown. I don't want to get into a speculation as to what other apps may or may not want - all we promise is nsIGlobalHistory*... the content of toolkit/ should converge on a smaller set of APIs and functionality that we promise to support. I don't think we can make that claim of this new history system. 

Annotations, Favicon service etc are another story. 

Comment 6

[Benjamin Smedberg]

On the contrary, the toolkit/ should not be only those things that we "promise" to support, but those things that are useful to other applications.

If we're not going to move all of places into toolkit/, then I'm going to need to file bugs to move the feed-soup-parsing code into toolkit; and we still want to solve the "give toolkit-based apps the ability to sharing a common history store", which lands us back in providing a common implementation.

Basically, I'm saying that the places backend should not be considered app-specific functionality.

Sure, like all new functionality places is going to go through API churn and we're not pretending that its stable yet... that doesn't mean that it's not valuable or that it's somehow magically specific to Firefox.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

We should have this figured out by b1. 

Comment 8

[Benjamin Smedberg]

Attachment: Files to move

Because this is branch and trunk we can't do cvscopies, but this is the list of files that will be moved.

Comment 9

[Benjamin Smedberg]

Attachment: Patch to apply

Comment 10

[Benjamin Smedberg]

Attachment: Patch to apply, rev. 1.1

Needed morkreader

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

I'm still not sold on this. I need to think about it a little bit. Will get back to you. 

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Things that need to be agreed upon before we can check in a patch like this:

- contributors to mozilla/browser can make changes to this code whenever they need to, regardless of the schedule of any other project
- we are free to change any non-frozen api in this code whenever and however we like, including in "non-API-change user-interface feature driven" releases

As a general principle, we as contributors to mozilla/browser are allowed to invoke these rules whenever someone might suggest the contrary. ;-)

Comment 13

[Benjamin Smedberg]

I think we are agreed that Firefox is the designing force behind the places implementation and the places owner/peers should have ownership over its development. If you're looking for a special carte-blanche for this code, could you explain what exactly you mean?

What are "non-API-change user-interface feature driven" releases? If you're talking about something like FF4 off the 1.9 branch, these interfaces wouldn't be under different rules than any others, as far as I can tell (i.e. they wouldn't be allowed to change, just as no interfaces are allowed to change in ff2). This is a requirement driven by the needs of extension authors, not general-purpose re-use.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 216760 [details] [diff] [review]
Patch to apply, rev. 1.1

r=[e-mail]

Apologies for the extreme delay.

Comment 15

[Benjamin Smedberg]

Comment on attachment 216745 [details]
Files to move

CVS copies have been performed using the new client-side tool.

Comment 16

[Benjamin Smedberg]

Fixed on trunk. Yay!

Comment 17

[Boris Zbarsky [:bzbarsky]]

metrics no longer compiles with these changes...

Comment 18

[Benjamin Smedberg]

bz, you mean --enable-extension=default,metrics? What is the error?

Comment 19

[Boris Zbarsky [:bzbarsky]]

yes, --enable-extensions=default,metrics.

../../../../mozilla/extensions/metrics/src/nsProfileCollector.cpp:453: error: `
   NS_NAVBOOKMARKSSERVICE_CONTRACTID' undeclared (first use this function)

and similar for other contracts.  I suspect it just needs to require toolkitcomps and include nsToolkitCompsCID.h.

Comment 20

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv