Last Comment Bug 383833 - eliminate toolkit's mork-based history implementation
: eliminate toolkit's mork-based history implementation
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
Depends on: 374532 384043 386392
Blocks: 382187 383085 482718
  Show dependency treegraph
 
Reported: 2007-06-09 02:39 PDT by Robert Kaiser (not working on stability any more)
Modified: 2009-03-11 09:08 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
build history/public for places, never build all of history (1.91 KB, patch)
2007-06-09 04:32 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
step 1: only build history/ for non-places builds (728 bytes, patch)
2007-06-21 14:07 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
step 1, v1.1: build history/ only for non-places builds, make places use its own nsIBrowserHistory.idl (1.50 KB, patch)
2007-06-22 08:28 PDT, Robert Kaiser (not working on stability any more)
no flags Details | Diff | Review
step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places (checked in) (2.29 KB, patch)
2007-06-23 06:30 PDT, Robert Kaiser (not working on stability any more)
asaf: review+
Details | Diff | Review
step 2: finally kill toolkit history (168.92 KB, patch)
2008-08-25 10:39 PDT, Robert Kaiser (not working on stability any more)
asaf: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2007-06-09 02:39:26 PDT
I just realized that toolkit's mork-based history implementation is currently unused, and we should not create new users for it, so best is to just remove it.

This allows us to not only remove a few files from cvs (toolkit/components/history/src/*) but also use MOZ_PLACES as a clean switch for migrating SeaMonkey and Camino from xpfe history to places history.
Comment 1 Robert Kaiser (not working on stability any more) 2007-06-09 04:32:37 PDT
Created attachment 267794 [details] [diff] [review]
build history/public for places, never build all of history

This removes everything but history/public from build, along with that, the following files can be cvs removed:

mozilla/toolkit/components/history/.cvsignore
mozilla/toolkit/components/history/Makefile.in
mozilla/toolkit/components/history/src/.cvsignore
mozilla/toolkit/components/history/src/Makefile.in
mozilla/toolkit/components/history/src/nsGlobalHistory.cpp
mozilla/toolkit/components/history/src/nsGlobalHistory.h
mozilla/toolkit/components/history/src/nsHistoryLoadListener.h
mozilla/toolkit/components/history/src/nsMdbPtr.h
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-09 14:23:20 PDT
Comment on attachment 267794 [details] [diff] [review]
build history/public for places, never build all of history

I think the correct fix is to cvs copy the interface and then cvs remove toolkit/components/history  altogether. That said, non-places Fx builds are still supported... it might be just the right time to change that though.
Comment 3 Robert Kaiser (not working on stability any more) 2007-06-11 11:34:35 PDT
Is it OK to go for cvs copying the interface right now, make it be built from places accordingly, and apply the following hunk to the components/Makefile.in?

 ifndef MOZ_SUITE
 # XXX Suite doesn't want these just yet
 ifdef MOZ_XUL
 DIRS += \
        autocomplete \
-       history \
        passwordmgr \
        satchel \
        $(NULL)
 endif # MOZ_XUL
+ifndef MOZ_PLACES
+DIRS +=        history
+endif # MOZ_PLACES
 endif # MOZ_SUITE

Comment 4 Robert Kaiser (not working on stability any more) 2007-06-21 14:07:52 PDT
Created attachment 269280 [details] [diff] [review]
step 1: only build history/ for non-places builds

This patch goes the first step by isolating history/ so that it is only built for non-places builds, as the interface places needed from there is now in places/public.

post-alpha6 we can go and remove this ifdef along with the history/ subdir, but until then, I was informed that we want to still support non-places builds.
Comment 5 Robert Kaiser (not working on stability any more) 2007-06-22 08:28:11 PDT
Created attachment 269385 [details] [diff] [review]
step 1, v1.1: build history/ only for non-places builds, make places use its own nsIBrowserHistory.idl

Umm, sorry, of course we also need to do something with the copied IDL file ;-)
Comment 6 Robert Kaiser (not working on stability any more) 2007-06-23 06:30:03 PDT
Created attachment 269512 [details] [diff] [review]
step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places (checked in)

...and sorry again, I found an additional change we need. This has been _really_ tested now, though, and it really compiles correctly.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-06-23 13:25:06 PDT
Comment on attachment 269512 [details] [diff] [review]
step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places (checked in)

r=mano
Comment 8 Robert Kaiser (not working on stability any more) 2007-06-25 04:20:31 PDT
Comment on attachment 269512 [details] [diff] [review]
step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places (checked in)

This patch is checked in now, leaving the bug open for later completely removing the old history impl.
Comment 9 (not reading, please use seth@sspitzer.org instead) 2007-06-27 00:22:42 PDT
before we remove the old implementation, I think we'd like to do some "old" vs "new" performance testing, right dietrich?
Comment 10 Dietrich Ayala (:dietrich) 2007-06-27 09:35:51 PDT
(In reply to comment #9)
> before we remove the old implementation, I think we'd like to do some "old" vs
> "new" performance testing, right dietrich?
> 

yes, we're doing some places vs non-places testing under alice's new test harness in bug 374532.
Comment 11 Igor Velkov 2008-04-03 16:19:27 PDT
why did bug 386392 and bug 374532 is blockers for this?
Comment 12 Joshua Cranmer [:jcranmer] 2008-06-07 13:36:31 PDT
Seeing as how the consensus for bug 374532 is that a) there is a 2% regression for empty profiles and b) this regression is acceptable, would we be able to move forward on removing this now?
Comment 13 Robert Kaiser (not working on stability any more) 2008-08-25 10:39:42 PDT
Created attachment 335390 [details] [diff] [review]
step 2: finally kill toolkit history

This is the final second step to actually kill off toolkit's mork-based history.
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-07 07:33:31 PDT
Comment on attachment 335390 [details] [diff] [review]
step 2: finally kill toolkit history

r=mano
Comment 15 Robert Kaiser (not working on stability any more) 2008-09-10 16:05:26 PDT
Pushed as http://hg.mozilla.org/mozilla-central/rev/428c66910e66
Comment 16 Florian Quèze [:florian] [:flo] 2008-09-17 12:03:12 PDT
I suspect you may also want to remove the line "tkhstory" (currently line 266) and maybe all the else branch after ifdef MOZ_PLACES in toolkit/library/libxul-config.mk

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