Last Comment Bug 669040 - Remove mork from mozilla-central
: Remove mork from mozilla-central
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla8
Assigned To: Matheus Kerschbaum
:
:
Mentors:
Depends on: nanunanu
Blocks: 675500
  Show dependency treegraph
 
Reported: 2011-07-02 21:29 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2011-08-08 05:40 PDT (History)
15 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader (15.34 KB, patch)
2011-07-25 22:52 PDT, Matheus Kerschbaum
mak77: review-
Details | Diff | Splinter Review
m-c part 2: Nuke mork and morkreader (1.42 MB, patch)
2011-07-25 22:55 PDT, Matheus Kerschbaum
mak77: review+
Details | Diff | Splinter Review
c-c part 1: Move mozilla/db/mork and mdb.h to comm-central/db/ (1.35 MB, patch)
2011-07-25 23:00 PDT, Matheus Kerschbaum
standard8: review+
emorley: checkin+
Details | Diff | Splinter Review
c-c part 2: Add new db/ directory to the build-system (5.94 KB, patch)
2011-07-25 23:01 PDT, Matheus Kerschbaum
standard8: review+
emorley: checkin+
Details | Diff | Splinter Review
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader (38.76 KB, patch)
2011-07-26 21:33 PDT, Matheus Kerschbaum
mak77: review+
Details | Diff | Splinter Review
m-c part 2: Nuke mork and morkreader (1.39 MB, patch)
2011-07-30 05:02 PDT, Matheus Kerschbaum
matjk7: review+
Details | Diff | Splinter Review
m-c part 3: Remove history.dat, migrateFrecency.dat and formhistory.dat (23.70 KB, patch)
2011-08-01 18:45 PDT, Matheus Kerschbaum
matjk7: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-02 21:29:32 PDT
We've killed the last bits of Mork from Firefox in Bug 578268.  I want to remove the code from mozilla-central next.  My understanding is that some comm-central apps still need Mork.

Thus, I propose to move the following to comm-central:

db/mdb
db/mork
db/morkreader

Any objections?
Comment 1 Joshua Cranmer [:jcranmer] 2011-07-02 21:43:47 PDT
If SeaMonkey has turned off mork importing, then we don't want morkreader either: it only reads a small subset of the file format and is insufficient to handle reading .mab or .msf files. Since we'd have to replace it with our own version later anyways, there's no point in maintaining a tool that we can't use in the future if it's not being used at the moment.
Comment 2 Justin Wood (:Callek) 2011-07-03 01:01:52 PDT
Bringing in Karsten as SeaMonkey mailnews guy, and Standard8 to this thought/discussion
Comment 3 neil@parkwaycc.co.uk 2011-07-04 01:34:31 PDT
(In reply to comment #1)
> If SeaMonkey has turned off mork importing
As yet, we're still using it to import history from 1.x/Mozilla/Netscape.
Comment 4 Mark Banner (:standard8, afk until Dec) 2011-07-04 02:01:23 PDT
I would be happy for mork & morkreader to move to comm-central (although it unfortunately means loosing a bit of history continuity but I don't think that's a significant issue with mork, and it'd be good to have it in c-c if we're the only ones using it).

Putting it under db/ seems reasonable.
Comment 5 David :Bienvenu 2011-07-04 08:13:11 PDT
funny, I was just building firefox a few days ago and surprised to see that it still used morkreader...

yeah, putting it under db is the right thing to do.
Comment 6 Joshua Cranmer [:jcranmer] 2011-07-04 22:57:03 PDT
I'm not sure if you're proposing move to c-c/db or c-c/mailnews/db, but my vote would be the latter.

While we're also moving stuff, I'd like to propose moving mdb/public/mdb.h into mork (it's not like anyone else uses it), and otherwise eliminating mdb (pointless makefile recursion); it might also be nice to collapse mork down into a single directory.

The only complication I see is that the history importer code is still in mozilla-central.

In short, my proposed move code [from comm-central root]:
mkdir mailnews/db/mork
mv mozilla/db/mork/*/*         mailnews/db/mork
mv mozilla/db/morkreader       mailnews/db
mv mozilla/db/mdb/public/mdb.h mailnews/db/mork
rm mozilla/db/Makefile.in mozilla/db/README.html
rm -r mozilla/db/mork mozilla/db/mdb

and all of the associated build changes that need to ride along.
Comment 7 Mark Banner (:standard8, afk until Dec) 2011-07-05 00:55:56 PDT
/db/... currently makes more sense as suite uses it for non-mailnews stuff.
Comment 8 Robert Kaiser 2011-07-05 06:11:13 PDT
(In reply to comment #3)
> (In reply to comment #1)
> > If SeaMonkey has turned off mork importing
> As yet, we're still using it to import history from 1.x/Mozilla/Netscape.

Are you sure that code is still in there? I thought that the places people have already removed it.
Comment 9 Joshua Cranmer [:jcranmer] 2011-07-14 08:50:11 PDT
(In reply to comment #8)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > If SeaMonkey has turned off mork importing
> > As yet, we're still using it to import history from 1.x/Mozilla/Netscape.
> 
> Are you sure that code is still in there? I thought that the places people
> have already removed it.

The code is still there but not built for FF and the test has been removed (see bug 578268). In all likelihood, it is probably a good idea to remove the code altogether soonish, since I don't trust places people to keep it maintained for very long.
Comment 10 Robert Kaiser 2011-07-18 04:06:34 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Are you sure that code is still in there? I thought that the places people
> > have already removed it.
> 
> The code is still there but not built for FF

I'm NOT talking about the mork(reader) code, but the code on the places side that does the actual importing. I'm not sure if that still exists.
Comment 11 Marco Bonardo [::mak] 2011-07-18 07:19:21 PDT
I'm fine with killing any remaining mork code in Places at this stage, I don't think there is need to bring it on, if i'm not wrong updates from old versions go through each major release, so all needed support to upgrades should already be in the wild. Is it not the same for Seamonkey?
Globally the usecases doesn't seem to pay back anymore enough to keep it.
Btw, Places always used only the morkreader (see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsMorkHistoryImporter.cpp).
Comment 12 Joshua Cranmer [:jcranmer] 2011-07-18 08:28:19 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Are you sure that code is still in there? I thought that the places people
> > > have already removed it.
> > 
> > The code is still there but not built for FF
> 
> I'm NOT talking about the mork(reader) code, but the code on the places side
> that does the actual importing. I'm not sure if that still exists.

That is the code i am talking about:
<http://hg.mozilla.org/mozilla-central/file/819a2ffc4f0e/toolkit/components/places/nsMorkHistoryImporter.cpp>
<http://hg.mozilla.org/mozilla-central/file/819a2ffc4f0e/toolkit/components/places/Makefile.in>
Comment 13 Robert Kaiser 2011-07-18 11:59:10 PDT
Ah, didn't know it actually was still around. SeaMonkey has released 2.0, 2.1, and 2.2 all on places history, though, so I think pulling the plug on that import is probably not that large a problem nowadays.
Comment 14 Joshua Cranmer [:jcranmer] 2011-07-18 12:08:32 PDT
So we seem to have reached an agreement on removing the mork history importer from places altogether. In that case, I wholeheartedly support removing the morkreader code altogether: it's otherwise unused, it's insufficient for future mork migration, and I already have a reimplementation of it from scratch.

Therefore, the new proposed migration is:
mkdir -p db/
mv mozilla/db/mork/*/*         db/mork
mv mozilla/db/mdb/public/mdb.h db/mork
rm mozilla/db/Makefile.in mozilla/db/README.html
rm -r mozilla/db/mork mozilla/db/mdb mozilla/db/morkreader
[whatever it takes to remove morkreader stubs in places]
[build-system changes]
Comment 15 Matheus Kerschbaum 2011-07-25 22:52:49 PDT
Created attachment 548373 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader
Comment 16 Matheus Kerschbaum 2011-07-25 22:55:23 PDT
Created attachment 548374 [details] [diff] [review]
m-c part 2: Nuke mork and morkreader

This patch removes db/mork, db/mdb, db/morkreader, the mork history importer and the *.dat tests.
Comment 17 Matheus Kerschbaum 2011-07-25 23:00:50 PDT
Created attachment 548375 [details] [diff] [review]
c-c part 1: Move mozilla/db/mork and mdb.h to comm-central/db/
Comment 18 Matheus Kerschbaum 2011-07-25 23:01:59 PDT
Created attachment 548376 [details] [diff] [review]
c-c part 2: Add new db/ directory to the build-system
Comment 19 Marco Bonardo [::mak] 2011-07-26 08:35:32 PDT
Comment on attachment 548373 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader

Review of attachment 548373 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsINavHistoryService.idl
@@ -1651,5 @@
> -  /**
> -   * Import the given Mork history file.
> -   *  @param file     The Mork history file to import
> -   */
> -  void importHistory(in nsIFile file);

if you change an interface you should bump its uuid

::: toolkit/components/places/nsNavHistory.cpp
@@ -500,5 @@
> -      mDatabaseStatus == DATABASE_STATUS_UPGRADED) {
> -    if (mDatabaseStatus == DATABASE_STATUS_CREATE) {
> -      // Check if we should import old history from history.dat
> -      nsCOMPtr<nsIFile> historyFile;
> -      rv = NS_GetSpecialDirectory(NS_APP_HISTORY_50_FILE,

NS_APP_HISTORY_50_FILE becomes useless now, should be removed in an additional part or a follow-up

@@ -511,5 @@
> -    // In case we've either imported or done a migration from a pre-frecency
> -    // build, we will calculate the first cutoff period's frecencies once the
> -    // rest of the places infrastructure has been initialized.
> -    if (obsSvc)
> -      (void)obsSvc->AddObserver(this, TOPIC_PLACES_INIT_COMPLETE, PR_FALSE);

you should also remove the observer work we were doing in Observe: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#5554

::: toolkit/components/places/tests/unit/test_history.js
@@ -246,5 @@
> -  histFile.append("history.dat");
> -  do_check_true(histFile.exists());
> -
> -  bh.removeAllPages();
> -  do_check_false(histFile.exists());

there are far more pieces of code touching history.dat, see mxr.mozilla.org/mozilla-central/search?string=history.dat

::: toolkit/components/satchel/test/unit/test_bug_329741.js
@@ -1,2 @@
> -/* ***** BEGIN LICENSE BLOCK *****
> - * Version: MPL 1.1/GPL 2.0/LGPL 2.1

you are removing  the formhistory test but not the code in nsFormHistory.js ?
Comment 20 Matheus Kerschbaum 2011-07-26 21:33:08 PDT
Created attachment 548687 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader
Comment 21 Marco Bonardo [::mak] 2011-07-27 14:57:51 PDT
Comment on attachment 548687 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader

Review of attachment 548687 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, I couldn't find missing pieces this time!
Clearly on this kind of patch I'd love to see a full tryserver run
Comment 22 Marco Bonardo [::mak] 2011-07-27 15:02:30 PDT
Comment on attachment 548374 [details] [diff] [review]
m-c part 2: Nuke mork and morkreader

Review of attachment 548374 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 23 Robert Kaiser 2011-07-27 15:30:13 PDT
Just make sure not to land the m-c parts before the c-c parts or Thunderbird and SeaMonkey builds will be broken... ;-)
Comment 24 Matheus Kerschbaum 2011-07-27 18:42:03 PDT
(In reply to comment #21)
> Clearly on this kind of patch I'd love to see a full tryserver run

http://tbpl.mozilla.org/?tree=Try&rev=0fe5d6a60a34
Comment 25 Joshua Cranmer [:jcranmer] 2011-07-29 16:05:30 PDT
I've pushed the comm-central patches:
<http://hg.mozilla.org/comm-central/pushloghtml?changeset=1dbfc673bc3f>.
Comment 26 Dão Gottwald [:dao] 2011-07-30 00:21:54 PDT
The second mozilla-central patch doesn't fails to apply.
Comment 27 Matheus Kerschbaum 2011-07-30 05:01:32 PDT
(In reply to comment #26)
> The second mozilla-central patch doesn't fails to apply.

Mercurial seems to have trouble removing the .dat files. I'll attach a patch without that change since there's no harm in keeping them around for now.
Comment 28 Matheus Kerschbaum 2011-07-30 05:02:44 PDT
Created attachment 549568 [details] [diff] [review]
m-c part 2: Nuke mork and morkreader
Comment 29 Marco Bonardo [::mak] 2011-08-01 05:22:18 PDT
(In reply to comment #27)
> Mercurial seems to have trouble removing the .dat files. I'll attach a patch
> without that change since there's no harm in keeping them around for now.

may you also attach a patch that just removed those .dat? maybe this is a issue specific to some Mercurial version or some specific OS, someone may be able to apply it.
Comment 30 Matheus Kerschbaum 2011-08-01 18:45:12 PDT
Created attachment 549991 [details] [diff] [review]
m-c part 3: Remove history.dat, migrateFrecency.dat and formhistory.dat
Comment 31 Mark Banner (:standard8, afk until Dec) 2011-08-02 02:50:34 PDT
Comment on attachment 548375 [details] [diff] [review]
c-c part 1: Move mozilla/db/mork and mdb.h to comm-central/db/

Checked in: http://hg.mozilla.org/comm-central/rev/52efa9789800
Comment 32 Dão Gottwald [:dao] 2011-08-06 08:28:31 PDT
I manually removed toolkit/components/places/tests/unit/history.dat, toolkit/components/places/tests/unit/migrateFrecency.dat and toolkit/components/satchel/test/unit/formhistory.dat.
Comment 33 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-08 05:40:48 PDT
http://hg.mozilla.org/mozilla-central/rev/f2a50910befc
http://hg.mozilla.org/mozilla-central/rev/8e73650ecc3e

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