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
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 374532 384043 386392
Blocks: 382187 383085 482718
  Show dependency treegraph
 
Reported: 2007-06-09 02:39 PDT by Robert Kaiser
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
no flags Details | Diff | Splinter Review
step 1: only build history/ for non-places builds (728 bytes, patch)
2007-06-21 14:07 PDT, Robert Kaiser
no flags Details | Diff | Splinter 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
no flags Details | Diff | Splinter 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
asaf: review+
Details | Diff | Splinter Review
step 2: finally kill toolkit history (168.92 KB, patch)
2008-08-25 10:39 PDT, Robert Kaiser
asaf: review+
Details | Diff | Splinter Review

Description Robert Kaiser 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 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 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 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 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 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 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 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 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.