Closed
Bug 578268
(nanunanu)
Opened 14 years ago
Closed 13 years ago
Turn off MOZ_MORKREADER in Firefox
Categories
(Firefox :: General, enhancement)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 7
People
(Reporter: khuey, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
4.52 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
We can turn off MOZ_MORKREADER now that we no longer support reading profile data from Firefox 2, right?
Comment 1•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
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)
Reporter | ||
Comment 3•14 years ago
|
||
Tryserver approves.
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
Ah yes it works because toolkit is cheating a little.
Updated•14 years ago
|
Attachment #457077 -
Flags: review?(mak77)
Reporter | ||
Updated•14 years ago
|
Attachment #457077 -
Attachment is obsolete: true
Reporter | ||
Comment 6•14 years ago
|
||
I think this does it. Throwing it at tryserver overnight.
Reporter | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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•14 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.
Reporter | ||
Comment 10•14 years ago
|
||
(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•14 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?
Reporter | ||
Comment 12•14 years ago
|
||
Ok, then suite will need to put MOZ_MORKREADER in its confvars.
Reporter | ||
Comment 13•13 years ago
|
||
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•13 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.
Reporter | ||
Comment 15•13 years ago
|
||
Awesome. I'll file a followup on purging morkreader's source entirely then.
Comment 16•13 years ago
|
||
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•13 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 18•13 years ago
|
||
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+
Comment 19•13 years ago
|
||
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?
Reporter | ||
Comment 20•13 years ago
|
||
It's possible, at this point I certainly don't remember :-) I'll verify on try before pushing.
Reporter | ||
Comment 22•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4e3b03de1fd3 Huzzah!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Updated•13 years ago
|
Alias: nanunanu
You need to log in
before you can comment on or make changes to this bug.
Description
•