578268 - (nanunanu) Turn off MOZ_MORKREADER in Firefox

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

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

We can turn off MOZ_MORKREADER now that we no longer support reading profile data from Firefox 2, right?

Comment 1

[Marco Bonardo [:mak]]

yes, but only in Firefox, since other Places users (seamonkey for example) are still using it. So it should be a configure option. I actually already tried asking for it (don't recall the bug # off-hand) but I was told we did not want new options, especially for these small things.

Comment 2

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

Attachment: Patch

I don't think we need a configure option for this.  I haven't actually tried to build a full browser with this change but it does stop morkreader from being compiled.

Comment 3

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

Tryserver approves.

Comment 4

[Marco Bonardo [:mak]]

Interesting, morkreader is used by nsMorkHistoryImporter.cpp, and this file is compiled in toolkit/places regardless that define, I would expect toolkit to not compile then since db/morkreader lib should be missing. Or could be just because  toolkit-makefiles.sh will make in db/morkreader/Makefile if storage is enabled. So that config would be bogus.

ImportHistory is a global history idl method, it should be splitted out to another interface or throw NS_ERROR_NOT_IMPLEMENTED, otherwise we would have an interface that changes based on whether we compile with or without morkreader and other stuff should be ifdefed.

Comment 5

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

Ah yes it works because toolkit is cheating a little.

Comment 6

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

Attachment: Try harder

I think this does it.  Throwing it at tryserver overnight.

Comment 7

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

Tryserver actually approves this time.  From codesighs:

-1015	nsMorkReader::Read(nsIFile*)
-1249	nsMorkReader::ParseTable(nsACString_internal const&, nsDataHashtable<nsMorkReader::IDKey, int> const&)

etc.  This will need test updates though.

Comment 8

[Marco Bonardo [:mak]]

I was thinking maybe we should just kill the idl entry at all. There are no implementers using it, apart a test, that could be rewritten in cpp or just disappear (no reason to continue test something we won't use nor change anymore). The only user of this interface is history itself, and so it can access to it internally without exposing it.

I see you changed configure.in, does that mean it will be disabled by default for everyone? Does this mean implementers will have to provide it in their own confvars.sh?
I'm cc-ing Robert so he's aware of what we do here. afaict Seamonkey is the only current implementer needing history.dat importing.

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> I'm cc-ing Robert so he's aware of what we do here. afaict Seamonkey is the
> only current implementer needing history.dat importing.

We badly need it on 1.9.1 branch, on mozilla-2.0 we'd like to have it but could do without it eventually.

IIRC, Joshua has requested review for a better mork reader to land, so I think he should be following what's happening here.

Comment 10

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

(In reply to comment #8)
> I see you changed configure.in, does that mean it will be disabled by default
> for everyone? Does this mean implementers will have to provide it in their own
> confvars.sh?
> I'm cc-ing Robert so he's aware of what we do here. afaict Seamonkey is the
> only current implementer needing history.dat importing.

Suite sets MOZ_MORK=1 in its confvars.  AIUI morkreader is just a scaled down readonly version of mork.  Tbird already sets MOZ_MORKREADER to nothing in its confvars.  So suite may need to pick that up, not sure.

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> Suite sets MOZ_MORK=1 in its confvars.  AIUI morkreader is just a scaled down
> readonly version of mork.  Tbird already sets MOZ_MORKREADER to nothing in its
> confvars.  So suite may need to pick that up, not sure.

Well, you don't seem to know the code (actually, I don't either, but I know this detail). history.dat importing doesn't actually work with the full mork code, it requires the mork reader, which by itself can't read real mork, but just the dumbed-down variant history had been using. Fun thing, right?

Comment 12

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

Ok, then suite will need to put MOZ_MORKREADER in its confvars.

Comment 13

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

Comment on attachment 457260 [details] [diff] [review]
Try harder

I'd like to move forward for this in Gecko 2.2/3/whatever it ends up being.  If SeaMonkey is still using morkreader (which I assume it is) I'll add MOZ_MORKREADER=1 to SeaMonkey's confvars.sh.  If we want to nuke the relevant IDL here I'd like to do that in a separate bug.

Comment 14

[Robert Kaiser]

AFAIK, the only thing using and needing morkreader in SeaMonkey is places, so no need to specifically enable it there. SeaMonkey also builds full mork for mailnews stuff anyhow.

Comment 15

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

Awesome.  I'll file a followup on purging morkreader's source entirely then.

Comment 16

[Joshua Cranmer [:jcranmer]]

We still want to use morkreader in the future for importing address book and msf files when we get around to that demorkification.

Comment 17

[Robert Kaiser]

(In reply to comment #16)
> We still want to use morkreader in the future for importing address book and
> msf files when we get around to that demorkification.

May make sense though to just put it into comm-central if it's to be a mailnews-only thing then.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 457260 [details] [diff] [review]
Try harder

Please file a separate Places bug to deprecate and then completely kill the importHistory method and support of history.dat.

Also, looking at tests, I've found this toolkit/components/places/tests/unit/test_migrateFrecency.js that seems to depend on old history.dat being imported, I'm surprised that it passes!
Feel free to delete that test, it's of low value today.
I think I've not found any other test doing a real import from that file.
 
>diff -r ce6054509d48 toolkit/components/places/src/nsNoMorkStubImporter.cpp

>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Foundation

nit: "the Mozilla Foundation."

Comment 19

[Marco Bonardo [:mak]]

ehr also toolkit/components/places/tests/unit/test_history_import.js depends on importHistory()
you can remove it and history_import_test.dat file as well
I guess yourtryserver run didn't include xpcshell tests?

Comment 20

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

It's possible, at this point I certainly don't remember :-)

I'll verify on try before pushing.

Comment 22

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

http://hg.mozilla.org/mozilla-central/rev/4e3b03de1fd3

Huzzah!