Remove mork from mozilla-central

RESOLVED FIXED in mozilla8

Status

()

Core
General
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: Matheus Kerschbaum)

Tracking

unspecified
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(5 attachments, 2 obsolete attachments)

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?
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.
Bringing in Karsten as SeaMonkey mailnews guy, and Standard8 to this thought/discussion

Comment 3

6 years ago
(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.
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

6 years ago
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.
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.
/db/... currently makes more sense as suite uses it for non-mailnews stuff.

Comment 8

6 years ago
(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.
(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

6 years ago
(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.
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).
(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

6 years ago
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.
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]
(Assignee)

Comment 15

6 years ago
Created attachment 548373 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader
Attachment #548373 - Flags: review?(mak77)
Attachment #548373 - Flags: review?(khuey)
(Assignee)

Comment 16

6 years ago
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.
Attachment #548374 - Flags: review?(mak77)
(Assignee)

Comment 17

6 years ago
Created attachment 548375 [details] [diff] [review]
c-c part 1: Move mozilla/db/mork and mdb.h to comm-central/db/
Attachment #548375 - Flags: review?(mbanner)
(Assignee)

Comment 18

6 years ago
Created attachment 548376 [details] [diff] [review]
c-c part 2: Add new db/ directory to the build-system
Attachment #548376 - Flags: review?(mbanner)
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 ?
Attachment #548373 - Flags: review?(mak77) → review-
Attachment #548373 - Flags: review?(khuey)
(Assignee)

Comment 20

6 years ago
Created attachment 548687 [details] [diff] [review]
m-c part 1: Remove toolkit and build-system dependency on mork and morkreader
Attachment #548373 - Attachment is obsolete: true
Attachment #548687 - Flags: review?(mak77)

Updated

6 years ago
Assignee: nobody → matjk7
Status: NEW → ASSIGNED
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
Attachment #548687 - Flags: review?(mak77) → review+
Comment on attachment 548374 [details] [diff] [review]
m-c part 2: Nuke mork and morkreader

Review of attachment 548374 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #548374 - Flags: review?(mak77) → review+

Comment 23

6 years ago
Just make sure not to land the m-c parts before the c-c parts or Thunderbird and SeaMonkey builds will be broken... ;-)
(Assignee)

Comment 24

6 years ago
(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
Attachment #548376 - Flags: review?(mbanner) → review+
Attachment #548375 - Flags: review?(mbanner) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
I've pushed the comm-central patches:
<http://hg.mozilla.org/comm-central/pushloghtml?changeset=1dbfc673bc3f>.
The second mozilla-central patch doesn't fails to apply.
Keywords: checkin-needed
(Assignee)

Comment 27

6 years ago
(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.
Keywords: checkin-needed
(Assignee)

Comment 28

6 years ago
Created attachment 549568 [details] [diff] [review]
m-c part 2: Nuke mork and morkreader
Attachment #548374 - Attachment is obsolete: true
Attachment #549568 - Flags: review+
(Assignee)

Updated

6 years ago
Blocks: 675500
(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.
(Assignee)

Comment 30

6 years ago
Created attachment 549991 [details] [diff] [review]
m-c part 3: Remove history.dat, migrateFrecency.dat and formhistory.dat
Attachment #549991 - Flags: review+

Updated

6 years ago
Attachment #548375 - Flags: checkin+

Updated

6 years ago
Attachment #548376 - Flags: checkin+
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
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.
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/f2a50910befc
http://hg.mozilla.org/mozilla-central/rev/8e73650ecc3e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.