322369 - History importer

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Brett Wilson]

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This adds a generic MorkReader class which can enumerate the rows in a mork data store.  The NavHistoryImporter uses this to import from the user's existing history file.  I made that one have an XPCOM interface in case we'd like to offer import functionality from the UI at some point.

Comment 2

[Darin Fisher]

Do you really need to move ToCString from the obsolete section of nsTString to nsTSubstring?  I think we actually want to eliminate that method at some point instead.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> Do you really need to move ToCString from the obsolete section of nsTString to
> nsTSubstring?  I think we actually want to eliminate that method at some point
> instead.
> 

I did so because nsSubstring::ToFloat relies on it (and I thought it made sense to move ToFloat along with ToInteger).

Comment 4

[Darin Fisher]

Well, ToCString is redundant with BeginReading and numerous other string API functions.  It should be removed.  Perhaps it would be better to break this patch into a core patch (for string changes) and a places patch.  Probably good to have a separate bug for the string changes.

Comment 5

[Brian Ryner (not reading)]

There's a mistake in the current patch at any rate -- the implementation of the functions I added to nsTSubstring are #ifdef'd MOZ_STRING_WITH_OBSOLETE_API, but the declarations are not.  This can be fixed either way, obviously, but it seems rather backwards to add a new "obsolete" API.  I guess my preference would be to make these methods not "obsolete".

Comment 6

[Brian Ryner (not reading)]

(In reply to comment #5)
> seems rather backwards to add a new "obsolete" API.  I guess my preference
> would be to make these methods not "obsolete".
> 

(these methods == ToInteger, ToFloat)

Comment 7

[Brett Wilson]

Comment on attachment 208426 [details] [diff] [review]
patch

Maybe you should call it the Mork importer instead of NavHistory importer? It sounds like it import NavHistory. The other importers are called, for example, Opera importers.

>+nsNavHistoryImporter::EnumerateColumnsCB(const nsACString &aKey,
>+                                         nsCString aName, void *aData)
>+{
>+  TableReadClosure *data = NS_STATIC_CAST(TableReadClosure*, aData);
>+  for (PRUint32 i = 0; i < kColumnCount; ++i) {
>+    if (aName.Equals(gColumnNames[i])) {
>+      data->columnIDs[i].AssignLiteral("^");
>+      data->columnIDs[i].Append(aKey);

Could you explain what is happening here with the "^"?

>+SwapBytes(PRUnichar *buffer)

Have we tested the byte swapping code yet? That will be a pretty unusual case so we need to be sure to explicitly test it.

>Index: browser/components/places/src/nsNavHistoryImporter.h

Stopping here for now.

Comment 8

[Brett Wilson]

Comment on attachment 208426 [details] [diff] [review]
patch

>+// A FixedString implementation that can hold an 80-character line
>+class nsCLineString : public nsFixedCString

We're totally sure that its impossible to have a Mork file with > 80 chars per line. What if it's messed up? We should be sure to do the same thing (or better) as Mork in the failure cases.


>+// Unescape a Mork value.  Mork uses $xx escaping to encode non-ASCII
>+// characters.  Additionally, '$' and '\' are backslash-escaped.
>+// The result of the unescape is in returned into aResult.

<editorial_comment>Good lord</editorial_comment>


>+  if (!line.EqualsLiteral("// <!-- <mdb:mork:z v=\"1.4\"/> -->")) {
>+    return NS_ERROR_FAILURE; // unexpected file format

Is there any concern that we will need to import previous Mork formats from, for example, old Mozilla Suites?


>Index: db/morkreader/nsMorkReader.h
>===================================================================
>...the entire file
>+#endif

Can you say: #endif // nsMorkReader_h_


>Index: toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl
>===================================================================
>+/* noscript */
>+[uuid(e6396544-921d-4776-aa62-8bf2dc1ae058)]
>+interface nsIAutoCompleteBaseResult : nsIAutoCompleteResult
>+{
>+  void setSearchString(in AString searchString);
>+  void setErrorDescription(in AString errorDescription);
>+  void setDefaultIndex(in long defaultIndex);
>+  void setSearchResult(in unsigned long searchResult);
>+};

I assume you moved this from nsIAutoCompleteResultTypes so that file can be deleted when we remove Mork. However, I think the "Base" result is only used by the Mork MDB result. I wrote a "Simple" result in toolkit which is slightly improved and has an implementation for general use. I think we can just delete the Base and Mdb result types.

Still left to review: nsMorkReader.cpp starting from ::ParseMap

Comment 9

[Brian Ryner (not reading)]

(In reply to comment #7)
> Maybe you should call it the Mork importer instead of NavHistory importer? It
> sounds like it import NavHistory. The other importers are called, for example,
> Opera importers.

Ok.

> 
> >+nsNavHistoryImporter::EnumerateColumnsCB(const nsACString &aKey,
> >+                                         nsCString aName, void *aData)
> >+{
> >+  TableReadClosure *data = NS_STATIC_CAST(TableReadClosure*, aData);
> >+  for (PRUint32 i = 0; i < kColumnCount; ++i) {
> >+    if (aName.Equals(gColumnNames[i])) {
> >+      data->columnIDs[i].AssignLiteral("^");
> >+      data->columnIDs[i].Append(aKey);
> 
> Could you explain what is happening here with the "^"?

Sure (and maybe you can suggest a better way to deal with this).  When a column id appears in a cell value, it's prefixed with ^ to distinguish it from a literal column name.  This prefix is not used, however, where the column id -> column name map is defined.  So, this code is building up the id for each column so it can be quickly located in the StringMap for each row.

One alternative might be to insert the "^" as we build up the column id -> column name mapping, so that the two cases are consistent.

> 
> >+SwapBytes(PRUnichar *buffer)
> 
> Have we tested the byte swapping code yet? That will be a pretty unusual case
> so we need to be sure to explicitly test it.

I haven't tested it yet.  I'm planning to.

(In reply to comment #8)
> (From update of attachment 208426 [details] [diff] [review] [edit])
> >+// A FixedString implementation that can hold an 80-character line
> >+class nsCLineString : public nsFixedCString
> 
> We're totally sure that its impossible to have a Mork file with > 80 chars per
> line. What if it's messed up? We should be sure to do the same thing (or
> better) as Mork in the failure cases.

It's purely an optimization; the string will heap-allocate a buffer if the built-in storage is not big enough.  I just did this because nsAutoString's 64-character buffer will be too small in most cases.

> >+  if (!line.EqualsLiteral("// <!-- <mdb:mork:z v=\"1.4\"/> -->")) {
> >+    return NS_ERROR_FAILURE; // unexpected file format
> 
> Is there any concern that we will need to import previous Mork formats from,
> for example, old Mozilla Suites?

The version has been 1.4 since 4/21/1999, so I'm not really worried about this.  I'd say better safe than sorry since I don't have a previous version file to check the code against.

> >Index: db/morkreader/nsMorkReader.h
> >===================================================================
> >...the entire file
> >+#endif
> 
> Can you say: #endif // nsMorkReader_h_

Sure.

> >Index: toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl
> >===================================================================
> >+/* noscript */
> >+[uuid(e6396544-921d-4776-aa62-8bf2dc1ae058)]
> >+interface nsIAutoCompleteBaseResult : nsIAutoCompleteResult
> >+{
> >+  void setSearchString(in AString searchString);
> >+  void setErrorDescription(in AString errorDescription);
> >+  void setDefaultIndex(in long defaultIndex);
> >+  void setSearchResult(in unsigned long searchResult);
> >+};
> 
> I assume you moved this from nsIAutoCompleteResultTypes so that file can be
> deleted when we remove Mork. However, I think the "Base" result is only used by
> the Mork MDB result. I wrote a "Simple" result in toolkit which is slightly
> improved and has an implementation for general use. I think we can just delete
> the Base and Mdb result types.

Right.  I was attempting to get rid of the #include of mdb.h from anything that places relies on.  We can't do it quite yet, but when we do remove Mork I'll just get rid of nsIAutoCompleteResultTypes.idl.

Comment 10

[Brett Wilson]

> One alternative might be to insert the "^" as we build up the column id ->
> column name mapping, so that the two cases are consistent.

This brings up another question I had: What are column IDs and column names? The header file could use some additional explanation on this. And when you say that these appear in a cell, they are there to identify which colum this cell belongs to? (columID=some value) And can you distinguish these from atoms and keys? If not, can you try to unify the terminology?

Comment 11

[Brett Wilson]

Comment on attachment 208426 [details] [diff] [review]
patch

>+nsMorkReader::ParseMap(const nsCSubstring &aLine, StringMap *aMap)
>+{

Could you give an exaple of the format you are trying to parse like you do for ParseTable below? That would make this much easier to follow.


>+nsresult
>+nsMorkReader::ParseTable(const nsCSubstring &aLine)
...
>+      case ']':
>+        // We're done with the row
>+        currentRow = nsnull;
>+        break;

Shouldn't you delete currentRow?

...
>+    }
>+  } while (currentRow && NS_SUCCEEDED(ReadLine(line)));

Delete current row if it exists? You could have exited due to error.


>+void
>+nsMorkReader::NormalizeValue(nsCString &aValue)
>+{
>+  const nsCSubstring &str = Substring(aValue, 1, aValue.Length() - 1);
>+  if (aValue[0] == '^') {
>+    mValueMap.Get(str, &aValue);

If the "Get" fails, you may want to clear the value and/or return some error. You are requiring consumers to call this function, and they may have some garbage in their buffer before the call.

Comment 12

[Brian Ryner (not reading)]

(In reply to comment #11)
> >+nsresult
> >+nsMorkReader::ParseTable(const nsCSubstring &aLine)
> ...
> >+      case ']':
> >+        // We're done with the row
> >+        currentRow = nsnull;
> >+        break;
> 
> Shouldn't you delete currentRow?

currentRow is owned by mTable, so it should not be deleted until the table goes away.

Will address the other comments.

Comment 13

[Brian Ryner (not reading)]

Attachment: revised patch

This version fixes some problems with byte-swapping.  To make this work right, I had to treat the meta-row specially so that we can extra the byte order before we start processing any rows.  This also has Brett's comments addressed.

Comment 14

[Brett Wilson]

Comment on attachment 208792 [details] [diff] [review]
revised patch

You still have nsAutoCompleteBaseResult moved into nsIAutoCompleteResult. If you are removing nsIAutoCompleteMdbResult, you shouldn't need the base result at all. ie, why not just keep it in the old place and remove that file later?

Comment 15

[Brian Ryner (not reading)]

(In reply to comment #14)
> (From update of attachment 208792 [details] [diff] [review] [edit])
> You still have nsAutoCompleteBaseResult moved into nsIAutoCompleteResult. If
> you are removing nsIAutoCompleteMdbResult, you shouldn't need the base result
> at all. ie, why not just keep it in the old place and remove that file later?
> 

You're right, I'm not sure what I was thinking.  It works fine if I just leave the change out.

Comment 16

[Brian Ryner (not reading)]

checked in, darin agreed that sr comments could be addressed later.

Comment 17

[Darin Fisher]

Comment on attachment 208792 [details] [diff] [review]
revised patch

>Index: browser/components/places/public/nsINavHistoryService.idl

>+/**
>+ * nsIMorkHistoryImporter is a service which handles importing old
>+ * history files.
>+ */
>+[scriptable, uuid(be935aed-6929-4e44-be29-17ec89e5c8bd)]
>+interface nsIMorkHistoryImporter : nsISupports
>+{
>+  void importHistory(in nsIFile file, in nsINavHistoryService history);
>+};

nit: Document the parameters.


>Index: browser/components/places/src/nsMorkHistoryImporter.cpp

>+nsMorkHistoryImporter::EnumerateColumnsCB(const nsACString &aColumnID,
>+                                          nsCString aName, void *aData)

Usually better to pass a nsCString by reference (e.g., |const nsCString &|).


>+  const PRUnichar *title = NS_REINTERPRET_CAST(const PRUnichar*, titleBytes);

nit: I think this can be NS_STATIC_CAST since you are just converting
between pointers to two different integral types.


>+  mozIStorageConnection *conn = history->GetStorageConnection();
>+  mozStorageTransaction transaction(conn, PR_FALSE);

nit: should you null check |conn| before passing it on?


>Index: db/morkreader/Makefile.in

>+# The Original Code is Places code

nit: Perhaps "Mork Reader" would be better than "Places" here?


>Index: db/morkreader/nsMorkReader.cpp

>+ConvertChar(char *c)
...
>+    return PR_TRUE;
>+  } else if ('A' <= c1 && c1 <= 'F') {

nit: No need for else after return.


>Index: db/morkreader/nsMorkReader.h

>+  typedef nsDataHashtable<nsCStringHashKey,nsCString> StringMap;
>+
>+  // Enumerator callback type for processing column ids.
>+  // A column id is a short way to reference a particular column in the table.
>+  // These column ids can be used to look up cell values when enumerating rows.
>+  // columnID is the table-unique column id
>+  // name is the name of the column
>+  // userData is the opaque pointer passed to EnumerateColumns()
>+  // The callback can return PL_DHASH_NEXT to continue enumerating,
>+  // or PL_DHASH_STOP to stop.
>+  typedef PLDHashOperator
>+  (*PR_CALLBACK ColumnEnumerator)(const nsACString &columnID,
>+                                  nsCString name,
>+                                  void *userData);

So the nsCString parameter type is the result of the
nsDataHashtable?  Hmm... that's unfortunate...


>Index: toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl

>+interface nsIAutoCompleteBaseResult : nsIAutoCompleteResult
>+{
>+  void setSearchString(in AString searchString);
>+  void setErrorDescription(in AString errorDescription);
>+  void setDefaultIndex(in long defaultIndex);
>+  void setSearchResult(in unsigned long searchResult);
>+};

How about some documentation for this interface?


sr=darin with nits picked

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

You rule, Brian!

Comment 19

[Brian Ryner (not reading)]

comments addressed (except for nsIAutoCompleteBaseResult, which is going away soon)

Comment 20

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv