Last Comment Bug 578268 - (nanunanu) Turn off MOZ_MORKREADER in Firefox
(nanunanu)
: Turn off MOZ_MORKREADER in Firefox
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Firefox 7
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 650316 (view as bug list)
Depends on:
Blocks: 669040
  Show dependency treegraph
 
Reported: 2010-07-12 20:46 PDT by Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
Modified: 2011-07-02 21:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (428 bytes, patch)
2010-07-13 09:47 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
no flags Details | Diff | Review
Try harder (4.52 KB, patch)
2010-07-13 22:17 PDT, Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13)
mak77: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-12 20:46:50 PDT
We can turn off MOZ_MORKREADER now that we no longer support reading profile data from Firefox 2, right?
Comment 1 Marco Bonardo [::mak] 2010-07-13 05:52:48 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-13 09:47:22 PDT
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.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-13 13:14:52 PDT
Tryserver approves.
Comment 4 Marco Bonardo [::mak] 2010-07-13 16:17:44 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-13 16:22:20 PDT
Ah yes it works because toolkit is cheating a little.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-13 22:17:31 PDT
Created attachment 457260 [details] [diff] [review]
Try harder

I think this does it.  Throwing it at tryserver overnight.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-14 00:44:32 PDT
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] 2010-07-14 03:49:16 PDT
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 (not working on stability any more) 2010-07-14 04:21:18 PDT
(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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-20 13:51:06 PDT
(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 (not working on stability any more) 2010-07-20 18:35:09 PDT
(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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2010-07-20 18:36:03 PDT
Ok, then suite will need to put MOZ_MORKREADER in its confvars.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-03-25 08:22:41 PDT
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 (not working on stability any more) 2011-03-25 08:44:46 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-03-25 13:18:30 PDT
Awesome.  I'll file a followup on purging morkreader's source entirely then.
Comment 16 Joshua Cranmer [:jcranmer] 2011-03-25 19:19:29 PDT
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 (not working on stability any more) 2011-03-26 05:52:34 PDT
(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] 2011-04-02 11:56:04 PDT
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] 2011-04-02 11:59:18 PDT
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 [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-04-02 17:05:18 PDT
It's possible, at this point I certainly don't remember :-)

I'll verify on try before pushing.
Comment 21 Matt Brubeck (:mbrubeck) 2011-04-15 14:14:13 PDT
*** Bug 650316 has been marked as a duplicate of this bug. ***
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-07-02 12:23:26 PDT
http://hg.mozilla.org/mozilla-central/rev/4e3b03de1fd3

Huzzah!

Note You need to log in before you can comment on or make changes to this bug.