Closed
Bug 383833
Opened 17 years ago
Closed 15 years ago
eliminate toolkit's mork-based history implementation
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
People
(Reporter: kairo, Assigned: kairo)
References
Details
Attachments
(2 files, 3 obsolete files)
2.29 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
168.92 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•17 years ago
|
||
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
Attachment #267794 -
Flags: review?
![]() |
Assignee | |
Updated•17 years ago
|
Attachment #267794 -
Flags: review? → review?(mano)
Comment 2•17 years ago
|
||
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.
Attachment #267794 -
Flags: review?(mano)
![]() |
Assignee | |
Comment 3•17 years ago
|
||
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
![]() |
Assignee | |
Comment 4•17 years ago
|
||
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.
Attachment #267794 -
Attachment is obsolete: true
Attachment #269280 -
Flags: review?(mano)
![]() |
Assignee | |
Comment 5•17 years ago
|
||
Umm, sorry, of course we also need to do something with the copied IDL file ;-)
Attachment #269280 -
Attachment is obsolete: true
Attachment #269385 -
Flags: review?(mano)
Attachment #269280 -
Flags: review?(mano)
![]() |
Assignee | |
Comment 6•17 years ago
|
||
...and sorry again, I found an additional change we need. This has been _really_ tested now, though, and it really compiles correctly.
Attachment #269385 -
Attachment is obsolete: true
Attachment #269512 -
Flags: review?(mano)
Attachment #269385 -
Flags: review?(mano)
Comment 7•17 years ago
|
||
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
Attachment #269512 -
Flags: review?(mano) → review+
![]() |
Assignee | |
Comment 8•17 years ago
|
||
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.
Attachment #269512 -
Attachment description: step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places → step 1, v1.2: build history/ only for non-places builds, use nsIBrowserHistory.idl from places (checked in)
Comment 9•17 years ago
|
||
before we remove the old implementation, I think we'd like to do some "old" vs "new" performance testing, right dietrich?
Comment 10•17 years ago
|
||
(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.
Depends on: 374532
Comment 11•16 years ago
|
||
why did bug 386392 and bug 374532 is blockers for this?
Comment 12•16 years ago
|
||
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?
Component: History → Bookmarks & History
QA Contact: history → bookmarks
![]() |
Assignee | |
Comment 13•16 years ago
|
||
This is the final second step to actually kill off toolkit's mork-based history.
Attachment #335390 -
Flags: review?(mano)
Updated•16 years ago
|
Attachment #335390 -
Flags: review?(mano) → review+
Comment 14•16 years ago
|
||
Comment on attachment 335390 [details] [diff] [review] step 2: finally kill toolkit history r=mano
![]() |
Assignee | |
Comment 15•15 years ago
|
||
Pushed as http://hg.mozilla.org/mozilla-central/rev/428c66910e66
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 16•15 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•