Bug 578268 (nanunanu)

Turn off MOZ_MORKREADER in Firefox

RESOLVED FIXED in Firefox 7

Status

()

Firefox
General
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: khuey, Unassigned)

Tracking

unspecified
Firefox 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We can turn off MOZ_MORKREADER now that we no longer support reading profile data from Firefox 2, right?
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.
Created attachment 457077 [details] [diff] [review]
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.
Attachment #457077 - Flags: review?(mak77)
Tryserver approves.
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.
Ah yes it works because toolkit is cheating a little.

Updated

7 years ago
Attachment #457077 - Flags: review?(mak77)
Attachment #457077 - Attachment is obsolete: true
Created attachment 457260 [details] [diff] [review]
Try harder

I think this does it.  Throwing it at tryserver overnight.
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.
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

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

7 years ago
(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?
Ok, then suite will need to put MOZ_MORKREADER in its confvars.
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.
Attachment #457260 - Flags: review?(mak77)

Comment 14

6 years ago
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.
Awesome.  I'll file a followup on purging morkreader's source entirely then.
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

6 years ago
(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 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."
Attachment #457260 - Flags: review?(mak77) → review+
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?
It's possible, at this point I certainly don't remember :-)

I'll verify on try before pushing.
Duplicate of this bug: 650316
http://hg.mozilla.org/mozilla-central/rev/4e3b03de1fd3

Huzzah!
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Alias: nanunanu
Blocks: 669040
You need to log in before you can comment on or make changes to this bug.