I think that profile migration to use SQLite files would be better if we ditched db/mork and used db/morkreader instead to make reading the files sane and maintainable in the case of problems. Also, yarns are a great big pain to use. Anyways, I am currently attaching my documentation on mork. This was compiled by sitting down with a (virtual) notebook and taking notes based on how morkParser.cpp and morkBuilder.cpp (with some other friends) worked, along with a survey of a .mab and a .msf file. I would especially like it if David Bienvenu could look over it for any errors I made.

I'd lose the personal editorial comments here: ======================================================================== (I can think of more readable and more compact ways to express the same information...). jwz once remarked in a perl script reading the mork format that "David McCusker is a barking lunatic;" I disagree with that statement based on reading the mork code and his newsgroup posts. He is quite sane, just not a good person to write database code. The code, originally sitting in a mixture of db/mork and db/mdb, is an excellent example of hard-to-understand code. Sure, functions were documented, but an argument might say "this is the output yarn" without explaining when you have to set the yarn up yourself or when the code does that for you. By the way, *do not* mention yarns to me ever again. Variable names looked like bMorkParser_inMeta, and exception handling was often done through an environment variable constantly being passed around like a hot potato, with failures being handled through constant if (ev->Good()) blook. ============================================================= AFAICT, meta-rows are never used in mork. I'm not sure why you say that - the msg db code uses meta rows (see mailnews/db/msgdb/src/nsMsgThread.cpp), as you yourself point out later :-) "Cells are the core of mork: they are key-value pairs of columns and the particular row's values. That said, they are often the core of unreadability due to the fact that they are often used as (^hex^hex), even when doing so is memory inefficient." Do you mean wasteful of disk space? They're stored as ints in memory, not strings. "Also confusing are the semantics of the values of a mork cell. It's simple: they're a string, a char* (or char[] if you prefer)" confusing and simple don't usually go together. Perhaps counter intuitive, surprising, or unexpected :-) I haven't written a mork parser (I find mork's parser does fine parsing mork files :-) ) so I'm sure you know more about the actual bytes in the file format, but this does seem generally accurate and helpful.

Comment on attachment 311066 [details] Mork documentation minusing for the editorial comments, and the meta-row comment. Re charsets, afaik, the philosophy has always been that the caller was responsible for managing charsets. I think the charsets in the code were just a convenience for the caller to help keep track of the charset, but not meant to be used internally by Mork, since I know for a fact that Mork didn't want to be in the charset conversion business.

First portion of patch. Creaky, doesn't work, unfinished. Well, I finished it once, but that code was destroyed in an inexplicable work of hg madness. Nice to have copy #3 sitting around. Not quite my first patch. There is one problem though: I caused a segfault in string foo.

This patch is better and less hacky. My test mab file doesn't test some of the features in there yet, and I don't implement some features (and probably never will). Details of hacks: * Comments. These are hacked (to keep 1-byte lookahead) to only be expected in a dictionary. * Groups. I expect that all groups (transactions) execute normally. * Metarows (of rows). I've not found an example nor clear rationale for this. * Cutting cells in a row (e.g. [1 -...]). At best, it could be ignored; but I haven't seen it anywhere. * The !hex part of a row reference. Its usefulness is dubious, and I'm not 100% sure what is going on, since it isn't used. * Cutting a row in a table using the -[ syntax. I haven't seen it use (since [- does the same thing). I'm not even 100% sure this syntax is legal. In anticipation to some of your questions, yes, I will be changing the error messages. Those were one of my outlets for mork, STL, and XPCOM frustation. No test mab file is provided (copy it from your own profile to test); I have to create an invalid one first. And yes, I do know that a perfectly-working file will throw an EOF error because I have no feof test.

It works, except that GetTableIDs crashes, which causes tests to not work...

And, after some arduous rewrites, I have a completed version of the patch. Since the patch touches three modules, I am requesting one reviewer for each of the modules, and bienvenu for his general subject knowledge in the area.

Comment on attachment 331961 [details] [diff] [review] Final patch, v. 1 Curse bugzilla..

And curse it some more...

Comment on attachment 331961 [details] [diff] [review] Final patch, v. 1 I only reviewed the build-config, not the actual code. You have removed at least one license header accidentally. There is inconsistent indenting in db/morkreader/test/Makfeile.in. All variable-setting should be indented with two spaces and no tabs: tabs should only go in rules. I think that the check:: rule should produce standard output, but I'm not sure what that output is nowadays: ted would know. In any case, the pipes will hide error codes which you probably don't want to hide, so you may want to use temporary files instead of pipes.

Please format test output like so: TEST-PASS | test filename | extra info TEST-UNEXPECTED-FAIL | test filename | extra info You can see an example C++ test here: http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/TestCrashReporterAPI.cpp#205

I updated the patch to not remove the license headers (oops!), and should have fixed the tabs issue. The morkreader test facility was modified a rather lot so that it now produces modern test output, and documented its usage in the README, which is also expanded. Other minor changes include tweaks to documentation and changing an assertion to a warning because it's kind of expected.

Comment on attachment 334139 [details] [diff] [review] Final patch, v. 2 >diff --git a/toolkit/components/places/src/nsMorkHistoryImporter.cpp b/toolkit/components/places/src/nsMorkHistoryImporter.cpp >--- a/toolkit/components/places/src/nsMorkHistoryImporter.cpp >+++ b/toolkit/components/places/src/nsMorkHistoryImporter.cpp >@@ -44,41 +44,18 @@ > #include "nsTArray.h" > > // Columns for entry (non-meta) history rows >-enum { >- kURLColumn, >- kNameColumn, >- kVisitCountColumn, >- kHiddenColumn, >- kTypedColumn, >- kLastVisitColumn, >- kColumnCount // keep me last >-}; nit: remove the comment >@@ -188,48 +186,20 @@ > } > > // Read in the mork file >+ //nsnsMorkReader reader; nit: remove r=me on the history importer changes

Comment on attachment 334139 [details] [diff] [review] Final patch, v. 2 an impressive amount of work, sorry it's taken me so long to review it. Algorithmically, it seems OK, though you're the expert now :-) +nsMorkReader::nsMorkReader() +: buf(0), since this is a member var, it should be mBuf, or m_buf; also, a more descriptive member name might be helpful - is it the cached result of a lookahead? there are other member vars that should be named with m*, like parseError - I won't enumerate them all :-) + // toWrite isn't printable. So we'll escape it's value in the output. "its" remove the commented out line: // Read in the mork file + //nsnsMorkReader reader; +static const char header[] = "// <!-- <mdb:mork:z v=\"1.4\"/> -->"; +PRBool nsMorkReader::ParseHeader() +{ + char line[sizeof(header)]; + line[sizeof(header) - 1] = 0; I guess we'll never change the header string in Mork :-) + morkMid tid = ParseMid(); + if (!m_tables.Get(tid, &m_currentTable)) { + m_currentTable = new morkTable; + m_currentTable->metaRow = nsnull; + m_currentTable->toid = tid; + m_tables.Put(tid, m_currentTable); + } from what I can tell, the prevailing braces style (bless you :-) ) is not the above. can you make this explicit that you're returning an int/morkMid here: +nsMorkReader::morkMid nsMorkReader::ParseRowReference() { - PRBool res; - nsresult rv = mStream->ReadLine(aLine, &res); - NS_ENSURE_SUCCESS(rv, rv); - if (!res) { - return NS_ERROR_NOT_AVAILABLE; + morkMid referent = ParseMid(); + + if (peek() == '!') + { + HandleParseError("I'm not exactly clear what should be done in this case"); + referent.scope = nsDependentCString("ERROR\nBAD\nString"); + return referent; } + if ('a' <= c && c <= 'z') + return PR_TRUE; + if ('A' <= c && c <= 'Z') + return PR_TRUE; + return c == '_' || c == '?' || c == ':'; for readability, it's nice to be consistent in the order of constants vs vars in these comparsions, e.g., if (c >= 'a' && c <= 'z') magic number worthy of a comment? : +PLDHashNumber nsMorkReader::morkMid::HashKey(const nsMorkReader::morkMid *key) +{ + return key->id * 37 + HashString(key->scope); +} returns on their own line, generally: +{ + NS_PRECONDITION(m_currentTable, "No table selected!"); + if (!m_currentTable) return; + NS_WARN_IF_FALSE(m_currentTable->metaRow, "No metarow of table!"); + if (!m_currentTable->metaRow) return;

Comment on attachment 334139 [details] [diff] [review] Final patch, v. 2 an impressive amount of work, sorry it's taken me so long to review it. Algorithmically, it seems OK, though you're the expert now :-) +nsMorkReader::nsMorkReader() +: buf(0), since this is a member var, it should be mBuf, or m_buf; also, a more descriptive member name might be helpful - is it the cached result of a lookahead? there are other member vars that should be named with m*, like parseError - I won't enumerate them all :-) + // toWrite isn't printable. So we'll escape it's value in the output. "its" remove the commented out line: // Read in the mork file + //nsnsMorkReader reader; +static const char header[] = "// <!-- <mdb:mork:z v=\"1.4\"/> -->"; +PRBool nsMorkReader::ParseHeader() +{ + char line[sizeof(header)]; + line[sizeof(header) - 1] = 0; I guess we'll never change the header string in Mork :-) + morkMid tid = ParseMid(); + if (!m_tables.Get(tid, &m_currentTable)) { + m_currentTable = new morkTable; + m_currentTable->metaRow = nsnull; + m_currentTable->toid = tid; + m_tables.Put(tid, m_currentTable); + } from what I can tell, the prevailing braces style (bless you :-) ) is not the above. can you make this explicit that you're returning an int/morkMid here: +nsMorkReader::morkMid nsMorkReader::ParseRowReference() { - PRBool res; - nsresult rv = mStream->ReadLine(aLine, &res); - NS_ENSURE_SUCCESS(rv, rv); - if (!res) { - return NS_ERROR_NOT_AVAILABLE; + morkMid referent = ParseMid(); + + if (peek() == '!') + { + HandleParseError("I'm not exactly clear what should be done in this case"); + referent.scope = nsDependentCString("ERROR\nBAD\nString"); + return referent; } + if ('a' <= c && c <= 'z') + return PR_TRUE; + if ('A' <= c && c <= 'Z') + return PR_TRUE; + return c == '_' || c == '?' || c == ':'; for readability, it's nice to be consistent in the order of constants vs vars in these comparsions, e.g., if (c >= 'a' && c <= 'z') magic number worthy of a comment? : +PLDHashNumber nsMorkReader::morkMid::HashKey(const nsMorkReader::morkMid *key) +{ + return key->id * 37 + HashString(key->scope); +} returns on their own line, generally: +{ + NS_PRECONDITION(m_currentTable, "No table selected!"); + if (!m_currentTable) return; + NS_WARN_IF_FALSE(m_currentTable->metaRow, "No metarow of table!"); + if (!m_currentTable->metaRow) return;

Comment on attachment 334139 [details] [diff] [review] Final patch, v. 2 minusing based on review comments - new patch is coming, I believe

Updated for bienvenu's comments as well as dietrich's comments. Carrying forward bsmedberg and dietrich's reviews. I changed the review from mconnor to mano for the toolkit/components/satchel stuff per reviewer's request.

CC'ing mano, un-CC'ing mconnor.

Comment on attachment 338017 [details] [diff] [review] Final patch, v. 3 some of this code might conceivably be more concise by using toupper: +PRBool nsMorkReader::IsNameStart(char c) +{ + if (c >= 'a' && c <= 'z') + return PR_TRUE; + if (c >= 'A' && c <= 'Z') + return PR_TRUE; + return c == '_' || c == '?' || c == ':'; +} +PRUint32 nsMorkReader::ToHex(PRUint32 c) +{ + if (c >= 'a' && c <= 'f') + c = c - 'a' + 10; + else if (c >= 'A' && c <= 'F') + c = c - 'A' + 10; + else if (c >= '0' && c <= '9') + c = c - '0'; e.g., char uppperC = toupper(c); return (upperC >= 'A' && upperC <= 'Z') || c == '_' || c == '?' || c == ':'; similarly for ToHex. but that's totally up to you...

Comment on attachment 338017 [details] [diff] [review] Final patch, v. 3 r=mano on the changes to satchel.

Lemme explain the last changes, since bz committed too early: I noticed that I managed to leave out a new test file for satchel that I had written, so re-requesting review from mano on that. I also noticed that I had two bugs: one being that the quoting didn't work right (with a test case added to the list) and the other being that TestMorkReader only printed out the information for the last table instead of all of them (both discovered from trying to debug information in a very lengthy *.msf) file. The fprintf that the interdiff shows I removed wasn't supposed to have been removed (I was zealous in removing intermittent debugging code for the problems in the last paragraph). I'm also (explicitly) requesting sr from Neil this time, as more eyes on this code wouldn't hurt; carrying forward the r+ from bienvenu, dietrich, and bsmedberg.

Comment on attachment 340952 [details] [diff] [review] Final patch, v. 4 Neil know this code better than me, really.

Fixing old bitrot and adding more tests (mostly about parse failures).

Comment on attachment 448421 [details] [diff] [review] Updated, with more tests You use both kEOF and 0 as special characters; presumably neither of these are expected in actual Mork files? >+// Parsing utility methods You almost always call next() after peek(), and often you don't use the return value, so it looks like you're providing the wrong API. Instead, I think you could do this all with just three methods: 1. A method that reads a new character into the peek buffer (also call this after verifying the header to initialise the peek buffer) void MorkReader::next() { PRUint32 length; nsresult rv = m_stream->Read(&m_lookahead, 1, &length); if (NS_FAILED(rv) || length == 0) m_lookahead = kEOF; // I hope this value doesn't appear in actual files } 2. A method that peeks the next byte void MorkReader::peekValue() { return m_lookahead; } 3. A method that peeks the next non-whitespace void MorkReader::peek() { while (isspace(m_lookahead)) // hopefully this excludes kEOF next(); return m_lookahead; } >+ m_stream = stream; >+ if (!ParseHeader()) >+ { >+ HandleParseError("Unexpected header"); >+ return NS_ERROR_FILE_CORRUPTED; Don't you need to close the stream? >+ // Be nice and print out the rest of the line This fails if the parse error was caused by EOF. if (peekValue() == 0x0d) // I don't like 0xd/0xa either next(); if (peekValue() == 0x0a) next(); while (peekValue() != kEOF && peekValue() != 0x0d && peekValue() != 0x0a) { restOfLine += peekValue(); next(); } >+ if (peek() == '/') ... >+ if (peek() == '<') Could do with being a switch? (Plus some others too.) >+ referent.scope = nsDependentCString("ERROR\nBAD\nString"); Not sure what type scope is, but would AssignLiteral be better? >+ // We could be holding a whitespace in our buffer... clear it out! >+ char buffer = peekValueByte(); >+ if (buffer == ' ' || buffer == 0xa || buffer == 0xd) >+ next(); Isn't this the same as calling peek()?

Comment on attachment 448421 [details] [diff] [review] Updated, with more tests Not sure why I'm on this review, but I don't have time to look at this code.

Comment on attachment 448421 [details] [diff] [review] Updated, with more tests Requesting review from ted for the test and build portions of this patch.

(In reply to comment #24) > (From update of attachment 448421 [details] [diff] [review]) > You use both kEOF and 0 as special characters; presumably neither of these are > expected in actual Mork files? Mork was designed to be essentially a text file, so 0 and kEOF don't show up in practice. > You almost always call next() after peek(), and often you don't use the return > value, so it looks like you're providing the wrong API. Instead, I think you > could do this all with just three methods: I am going to need to think about the API for a bit more. > >+ m_stream = stream; > >+ if (!ParseHeader()) > >+ { > >+ HandleParseError("Unexpected header"); > >+ return NS_ERROR_FILE_CORRUPTED; > Don't you need to close the stream? My bad.

Comment on attachment 448421 [details] [diff] [review] Updated, with more tests >diff --git a/db/morkreader/test/Makefile.in b/db/morkreader/test/Makefile.in >new file mode 100644 >--- /dev/null >+++ b/db/morkreader/test/Makefile.in >+DEPTH = ../../.. >+topsrcdir = @top_srcdir@ >+srcdir = @srcdir@ >+VPATH = @srcdir@ >+ >+include $(DEPTH)/config/autoconf.mk >+ >+CPPSRCS = TestMorkReader.cpp \ >+ TestParseError.cpp Please don't put tabs in Makefiles unless you need them (in rule commands). Prefer two-space indent after line continuations. Generally the formatting works better if you do CPPSRCS = \ foo.cpp \ bar.cpp \ $(NULL) >+check:: >+ rm -rf data >+ cp -R $(srcdir)/data data >+ @for file in $(MORK_FILES); do \ >+ echo "Testing $$file"; \ >+ $(RUN_TEST_PROGRAM) $(DIST)/bin/TestMorkReader $$file \ >+ data/$${file}.expected; \ In TestMorkReader's RunTest, you locate the test file in $cwd/data. Any reason not to just pass data/$$file here instead? (Seems more sane.) >diff --git a/db/morkreader/test/TestMorkReader.cpp b/db/morkreader/test/TestMorkReader.cpp >new file mode 100644 >--- /dev/null >+++ b/db/morkreader/test/TestMorkReader.cpp >@@ -0,0 +1,187 @@ >+#include "MorkReader.h" >+#include "nsILocalFile.h" >+#include "nsComponentManagerUtils.h" >+#include "nsIServiceManager.h" >+#include "nsIProperties.h" >+#include "nsDirectoryServiceDefs.h" >+#include "nsServiceManagerUtils.h" >+#include "nspr.h" >+#include <stdarg.h> >+ >+using mozilla::database::MorkReader; >+ >+// The file of expected results >+FILE *matchFile = nsnull; >+// The filename of what we're reading >+char *fileName; >+ >+void Output(const char *format, ...) This function is really hard to understand. So you're using the callbacks, and then checking against the expected data in the .expected file? Would it be simpler just to printf all the output here, and then diff the results in the Makefile or something like that? I guess if what you have works it's hard to complain, and it is Mork, so sanity is not exactly the #1 concern. Otherwise this looks fine.

(In reply to comment #28) > In TestMorkReader's RunTest, you locate the test file in $cwd/data. Any reason > not to just pass data/$$file here instead? (Seems more sane.) That was because, when I rewrote the test code to get $cwd working correctly, I couldn't figure out how to do a relative directory access... Now I figured it out and reverted this change. > This function is really hard to understand. So you're using the callbacks, and > then checking against the expected data in the .expected file? Would it be > simpler just to printf all the output here, and then diff the results in the > Makefile or something like that? I guess if what you have works it's hard to > complain, and it is Mork, so sanity is not exactly the #1 concern. I don't entirely recall why I did it this way, but I think it was related to getting diff to do what I wanted it to do. Needless to say, bash scripting is not my strong point. :-)

This should apply to tip again. I've addressed ted's issues, so carrying over r+ from him. I think I've also addressed all of Neil's issues (we discussed the parser over IRC).

Comment on attachment 472416 [details] [diff] [review] Updated to tip >+ // Backpointers to the history we're operating on > nsNavHistory *history; nit: Backpointer the Places changes look ok, r=me. asking for additional review from Marco, since migration code is muy sensitivo.

Comment on attachment 472416 [details] [diff] [review] Updated to tip Did you already run all Places tests (we have a couple that will import history.dat iirc)? >diff --git a/toolkit/components/places/src/nsMorkHistoryImporter.cpp b/toolkit/components/places/src/nsMorkHistoryImporter.cpp > // Enumerator callback to add a table row to history >-static PLDHashOperator >-AddToHistoryCB(const nsCSubstring &aRowID, >- const nsTArray<nsCString> *aValues, >+static void >+AddToHistoryCB(const mozilla::database::MorkReader::morkMid &roid, roid? I prefer the old aRowID name, unless roid is some sort of special name? >+ nsCString *valueString, emptyString; >+ PRBool hasValue; I guess hasValue var could be scoped inside the macro, it is used only there also should probably use AutoStrings >+#define GET_VALUE(name) \ >+ PR_BEGIN_MACRO \ >+ hasValue = cells.Get(nsDependentCString(name), &valueString); \ >+ if (!hasValue) \ >+ valueString = &emptyString; \ This could use EmptyCString()? >+ >+ // Do not import hidden records Trailing space, btw if possible in new code use proper phrases ending with commas. >+ GET_VALUE("Hidden"); >+ if (valueString->EqualsLiteral("1")) >+ return; > > nsCOMPtr<nsIURI> uri; >+ GET_VALUE("URL"); this sux a bit because in various parts of this method valueString has always different meanings, and it's confusing. Could you do somethink like NS_NAMED_LITERAL_CSTRING(n,s), so a GET_NAMED_VALUE("URL", varname); that defines an autostring for you? So you would have GET_NAMED_VALUE("Hidden", hiddenValue); GET_NAMES_VALUE("URL", urlValue); >+static void >+MetarowEnumCB(const nsACString &header, const nsACString &value, void *userData) params code style please, "a" prefix for input "_" prefix for output. >@@ -195,48 +191,19 @@ > } > > // Read in the mork file >- nsMorkReader reader; >- nsresult rv = reader.Init(); >+ mozilla::database::MorkReader reader; >+ nsresult rv = reader.ParseFile(aFile); > NS_ENSURE_SUCCESS(rv, rv); > >- rv = reader.Read(aFile); >- NS_ENSURE_SUCCESS(rv, rv); >+ const mozilla::database::MorkReader::morkMid tableId(1, >+ nsDependentCString("ns:history:db:row:scope:history:all")); reasons to not use NS_LITERAL_CSTRING? r=me with above fixed/answered.

(In reply to comment #32) > Did you already run all Places tests (we have a couple that will import > history.dat iirc)? I tested this with make xpcshell-tests in toolkit/components/places/tests on Linux. > roid? I prefer the old aRowID name, unless roid is some sort of special name? `roid' is essentially Mork's internal name. I've changed it back to aRowID, though. > >+#define GET_VALUE(name) \ > >+ PR_BEGIN_MACRO \ > >+ hasValue = cells.Get(nsDependentCString(name), &valueString); \ > >+ if (!hasValue) \ > >+ valueString = &emptyString; \ > > This could use EmptyCString()? EmptyCString() is const, and valueString is not const (due to the assignment later to another value). I think I can get around it, though, if I rewrite the macro handling here, which I'll already need to do for your next comment.

(In reply to comment #32) > Trailing space, btw if possible in new code use proper phrases ending with > commas. uh I figured out I meant "ending with periods" :)

David review ping ! Neil Sr ping.

Neil Sr ping.

Neil Sr ping.

Comment on attachment 472416 [details] [diff] [review] Updated to tip As the places code has removed the mork importer and no other code uses it, bug 669040 removes the morkreader outright. I've got the source of this code saved as a tarball locally, so when mailnews needs the import code again, I can pull it back up.

This is unneeded for the near future.