669040 - Remove mork from mozilla-central

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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]]

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)]

Bringing in Karsten as SeaMonkey mailnews guy, and Standard8 to this thought/discussion

Comment 3

[[e-mail]]

(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)]

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]

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]]

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)]

/db/... currently makes more sense as suite uses it for non-mailnews stuff.

Comment 8

[Robert Kaiser]

(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]]

(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]

(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]]

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]]

(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]

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]]

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]

Attachment: m-c part 1: Remove toolkit and build-system dependency on mork and morkreader

Comment 16

[Matheus Kerschbaum]

Attachment: 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]

Attachment: c-c part 1: Move mozilla/db/mork and mdb.h to comm-central/db/

Comment 18

[Matheus Kerschbaum]

Attachment: c-c part 2: Add new db/ directory to the build-system

Comment 19

[Marco Bonardo [:mak]]

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]

Attachment: m-c part 1: Remove toolkit and build-system dependency on mork and morkreader

Comment 21

[Marco Bonardo [:mak]]

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]]

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]

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]

(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]]

I've pushed the comm-central patches:
<http://hg.mozilla.org/comm-central/pushloghtml?changeset=1dbfc673bc3f>.

Comment 26

[Dão Gottwald [:dao]]

The second mozilla-central patch doesn't fails to apply.

Comment 27

[Matheus Kerschbaum]

(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]

Attachment: m-c part 2: Nuke mork and morkreader

Comment 29

[Marco Bonardo [:mak]]

(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]

Attachment: m-c part 3: Remove history.dat, migrateFrecency.dat and formhistory.dat

Comment 31

[Mark Banner (:standard8)]

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]]

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 (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f2a50910befc
http://hg.mozilla.org/mozilla-central/rev/8e73650ecc3e