324170 - Migrate nsFormHistory to use mozstorage

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Brian Ryner (not reading)]

One of our goals for FF2 is to compensate for the size of sqlite by removing mork.  With global history replaced by the Places implementation, Form History is the last consumer of Mork. It can be converted to mozstorage fairly easily, and use nsMorkReader to import the old form history.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

This forks nsFormHistory to create a new storage-based implementation.  When we're ready to make the switch to Places, we can remove the old implementation.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

*** Bug 317882 has been marked as a duplicate of this bug. ***

Comment 3

[Benjamin Smedberg]

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

r=me for the build stuff, I'm not competent to review the actual formcomplete code. I'd really like to get bug 322965 landed before this so that we don't multiply the ifdef-hell of toolkit/components/build

Comment 4

[Brett Wilson]

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

>--- toolkit/components/build/Makefile.in	18 Nov 2005 07:48:54 -0000	1.27
>+++ toolkit/components/build/Makefile.in	20 Jan 2006 09:02:12 -0000
>@@ -90,6 +89,17 @@ REQUIRES = 	\
> 		alerts \
> 		$(NULL)
> 
>+ifdef MOZ_STORAGE
>+REQUIRES += storage
>+ifdef MOZ_MORKREADER
>+REQUIRES += morkreader
>+endif
>+else # MOZ_STORAGE
>+ifdef MOZ_MORK
>+REQUIRES += mork
>+endif
>+endif # MOZ_STORAGE

This requires storage to get the mork reader. It seems like somebody may want mork reader but not storage. Is there any reason that you can't bring 
  ifdef MOZ_MORKREADER
  REQUIRES += morkreader
  endif
out of the MOZ_STORAGE conditional altogether? It looks like your EXTRA_DSO_LDOPTS does this below, so they should be consistent.

> #ifndef MOZ_THUNDERBIRD
>-  nsFormHistory::ReleaseInstance();
>   nsPasswordManager::Shutdown();
> #endif

I don't understand this change, but if you're sure it's right I guess it's fine.

Later I will check the source code.

Comment 5

[Brian Ryner (not reading)]

(In reply to comment #4)
> > #ifndef MOZ_THUNDERBIRD
> >-  nsFormHistory::ReleaseInstance();
> >   nsPasswordManager::Shutdown();
> > #endif
> 
> I don't understand this change, but if you're sure it's right I guess it's
> fine.

Basically, this code was doing some wacky things that can be done much easier by just relying on the service manager to keep the single instance around.

Comment 6

[Brett Wilson]

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

>+NS_IMETHODIMP
>+nsFormHistory::GetEntryCount(PRUint32 *aEntryCount)
>+{
>+  mozStorageStatementScoper scope(mDBRowCount);
>+  PRBool hasMore;
>+  if (NS_SUCCEEDED(mDBRowCount->ExecuteStep(&hasMore)) && hasMore) {
>+    *aEntryCount = mDBRowCount->AsInt32(0);
>+  } else {
>+    *aEntryCount = 0;
>+  }
>+  return NS_OK;
>+}

It doesn't look like this function is used by anything. Can we get rid of it? If we only need to see if there are any entries, we should write a function to check that. sqlite count() involves iterating over all the results, so is very inefficient.

>+nsFormHistory::AddEntry(const nsAString &aName, const nsAString &aValue)
>+{
...
>+    nsCOMPtr<mozIStorageStatement> dbInsertStatement;
>+    nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING("INSERT INTO moz_formhistory (fieldname, value) VALUES (?1, ?2)"),
>+                                           getter_AddRefs(dbInsertStatement));
>+    NS_ENSURE_SUCCESS(rv, rv);

I'd suggest precompiling this statement. It will get called multiple times for each form submission.


>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE UNIQUE INDEX moz_formhistory_index ON moz_formhistory (fieldname, value)"));
>+    NS_ENSURE_SUCCESS(rv, rv);

This will let you efficiently search for (fieldname, value) pairs, but not (fieldname) items. Don't we want this for generating autocomplete results?


>+  if (aPrevResult) {
>+    result = aPrevResult;
>+
>+    PRUint32 matchCount;
>+    result->GetMatchCount(&matchCount);
>+
>+    for (PRInt32 i = matchCount - 1; i >= 0; --i) {
>+      nsAutoString match;
>+      result->GetValueAt(i, match);
>+      if (!StringBeginsWith(match, aInputValue,
>+                            nsCaseInsensitiveStringComparator())) {
>+        result->RemoveValueAt(i, PR_FALSE);
>+      }
>+    }

I think this will not work if you press backspace. The history autocomplete checks the new and the old one to see if the old one is a prefix of the new one. If not, it does a fresh autocomplete query.


>+      // filters out irrelevant results
>+      if(StringBeginsWith(entryString, aInputValue,
>+                          nsCaseInsensitiveStringComparator())) {

Should we use locale-aware case-insensetive string compares here? Europeans might want it to ignore accents for example.


>+nsFormHistoryImporter::AddToFormHistoryCB(const nsACString &aRowID,
>+                                          const nsMorkReader::StringMap *aMap,
>+                                          void *aData)
...
>+      // XXX this should handle byte swapping, but there's no endianness marker

Does this mean we can't tell share form history between different endian computers?


>+NS_IMETHODIMP
>+nsFormHistoryImporter::ImportFormHistory(nsIFile *aFile,
>+                                         nsIFormHistory *aFormHistory)
...
>+  // Add the rows to form history
>+  nsFormHistory *formHistory = NS_STATIC_CAST(nsFormHistory*, aFormHistory);

Might one of those static IID accessor things be better for this?

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> (From update of attachment 209132 [details] [diff] [review] [edit])
> >+NS_IMETHODIMP
> >+nsFormHistory::GetEntryCount(PRUint32 *aEntryCount)
> >+{
> >+  mozStorageStatementScoper scope(mDBRowCount);
> >+  PRBool hasMore;
> >+  if (NS_SUCCEEDED(mDBRowCount->ExecuteStep(&hasMore)) && hasMore) {
> >+    *aEntryCount = mDBRowCount->AsInt32(0);
> >+  } else {
> >+    *aEntryCount = 0;
> >+  }
> >+  return NS_OK;
> >+}
> 
> It doesn't look like this function is used by anything. Can we get rid of it?

It's used from javacsript (sanitize.js)

> If we only need to see if there are any entries, we should write a function to
> check that. sqlite count() involves iterating over all the results, so is very
> inefficient.

That's all it needs to check. I'll change it around.

> >+nsFormHistory::AddEntry(const nsAString &aName, const nsAString &aValue)
> >+{
> ...
> >+    nsCOMPtr<mozIStorageStatement> dbInsertStatement;
> >+    nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING("INSERT INTO moz_formhistory (fieldname, value) VALUES (?1, ?2)"),
> >+                                           getter_AddRefs(dbInsertStatement));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> I'd suggest precompiling this statement. It will get called multiple times for
> each form submission.

OK.

> >+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE UNIQUE INDEX moz_formhistory_index ON moz_formhistory (fieldname, value)"));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> This will let you efficiently search for (fieldname, value) pairs, but not
> (fieldname) items. Don't we want this for generating autocomplete results?

Oops, you're right.

> 
> 
> >+  if (aPrevResult) {
> >+    result = aPrevResult;
> >+
> >+    PRUint32 matchCount;
> >+    result->GetMatchCount(&matchCount);
> >+
> >+    for (PRInt32 i = matchCount - 1; i >= 0; --i) {
> >+      nsAutoString match;
> >+      result->GetValueAt(i, match);
> >+      if (!StringBeginsWith(match, aInputValue,
> >+                            nsCaseInsensitiveStringComparator())) {
> >+        result->RemoveValueAt(i, PR_FALSE);
> >+      }
> >+    }
> 
> I think this will not work if you press backspace. The history autocomplete
> checks the new and the old one to see if the old one is a prefix of the new
> one. If not, it does a fresh autocomplete query.

Hm, so this should work exactly like the old implementation... which isn't to say that it's necessarily correct.  I'll do some tests with backspace.

> >+      // filters out irrelevant results
> >+      if(StringBeginsWith(entryString, aInputValue,
> >+                          nsCaseInsensitiveStringComparator())) {
> 
> Should we use locale-aware case-insensetive string compares here? Europeans
> might want it to ignore accents for example.
> 

No idea.

> Does this mean we can't tell share form history between different endian
> computers?

Yes. Note that that's already the current situation.

> 
> 
> >+NS_IMETHODIMP
> >+nsFormHistoryImporter::ImportFormHistory(nsIFile *aFile,
> >+                                         nsIFormHistory *aFormHistory)
> ...
> >+  // Add the rows to form history
> >+  nsFormHistory *formHistory = NS_STATIC_CAST(nsFormHistory*, aFormHistory);
> 
> Might one of those static IID accessor things be better for this?
> 

probably yes.

Comment 8

[Benjamin Smedberg]

> This requires storage to get the mork reader. It seems like somebody may want
> mork reader but not storage. Is there any reason that you can't bring 

REQUIRES is merely an additional directory in dist/include/*, it doesn't actually REQUIRE anything.

Comment 9

[Brian Ryner (not reading)]

(In reply to comment #6)
> I think this will not work if you press backspace. The history autocomplete
> checks the new and the old one to see if the old one is a prefix of the new
> one. If not, it does a fresh autocomplete query.

The AutoCompleteController actually manages this detail already, see http://lxr.mozilla.org/mozilla1.8/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#216

The only thing that needs to happen in AutoCompleteSearch, I think, is to remove non-matching rows when an existing result is given.... which should only be when the search is being made _more_ specific.

I just now discovered that nsAutoCompleteSimpleResult doesn't implement RemoveValueAt, so I needed to add an implementation there.  Unfortunately, it can't currently implement the aRemoveFromDb parameter, which is necessary to make shift+delete work to remove form history entries.

Comment 10

[Brian Ryner (not reading)]

Attachment: patch

This patch has all of the above review comments addressed, as well as a change in the way that FormHistory creates autocomplete results.  I added a new type of nsIAutoCompleteResult specifically for form history that knows how to handle RemoveValueAt() with aRemoveFromDB=true.  This makes shift+delete work as expected.

Also, I'm aware of the ugliness in nsFormHistory::QueryInterface.  I couldn't see a way to make this work using the provided macros.  If anyone does see one, please let me know.  Note that assigning an |nsFormHistory*| to |foundInterface| is what causes the compile error, since it's an ambiguous conversion.

Comment 11

[Benjamin Smedberg]

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

>Index: db/morkreader/Makefile.in

> FORCE_STATIC_LIB = 1
>+EXPORT_LIBRARY = 1

This scares me. Is the goal here to have a static lib which is linked to multiple components in a dynamic build but included once in a final static build? Could we make this a dynamic lib in dynamic builds (remove the FORCE_STATIC_LIB declaration) and keep your EXPORT_LIBRARY? This might require some use of EXPAND_MOZLIBNAME... hrm. I don't want to hold you up on this, but I think that a dynamiclib would be the good and right long-term solution (unless there are plans to turn the morkreader into a component hidden behind interfaces or pseudo-interfaces, which might be even better).

Comment 12

[Brian Ryner (not reading)]

(In reply to comment #11)
> This scares me. Is the goal here to have a static lib which is linked to
> multiple components in a dynamic build but included once in a final static
> build? Could we make this a dynamic lib in dynamic builds (remove the

That's what I was going for, yes.

> FORCE_STATIC_LIB declaration) and keep your EXPORT_LIBRARY? This might require
> some use of EXPAND_MOZLIBNAME... hrm. I don't want to hold you up on this, but
> I think that a dynamiclib would be the good and right long-term solution

Could do that; the nsMorkReader class would need to have appropriate NS_EXPORT / NS_IMPORT declarations.  My initial feeling was that linking it statically would make it perform better, but the number of function calls into the library is actually very small (the majority of the function calls are callbacks via the row enumeration). So making this a DLL is doable.  I think it can be done in a separate patch though.

Comment 13

[Brett Wilson]

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

It sucks that we can't derive from a simpleresult, but oh well.

Comment 14

[Stefan Sitter [:ssitter]]

The check in for this bug busted all the sunbird tinderboxes builds:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird

c:/builds/tinderbox/Sb-Trunk/WINNT_5.2_Depend/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp(422) : error C2061: syntax error : identifier 'nsFormHistoryImporter'
c:/builds/tinderbox/Sb-Trunk/WINNT_5.2_Depend/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp(423) : error C2143: syntax error : missing ';' before '{'

Comment 15

[Stefan Sitter [:ssitter]]

(In reply to comment #14)
> The check in for this bug busted all the sunbird tinderboxes builds:
> http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird

<mvl> bsmedberg: the error looks like an error in satchel code
<bz> yeah
<bz> 126 #ifdef MOZ_MORKREADER
<bz> 127 class nsFormHistoryImporter : public nsIFormHistoryImporter
<bz> And then it's used outside ifdefs
<bz> 422       nsCOMPtr<nsIFormHistoryImporter> importer = new nsFormHistoryImporter();
* bz is not sure how this was envisioned as working
<bz> The only way satchel can possibly compile as it is is if MOZ_MORKREADER is defined
<dbaron> why is MOZ_MORKREADER even defined for a non-places Firefox build?
<bz> Since either the define should work or it should be removed
<bsmedberg> dbaron: this is sunbird, no?
<dbaron> yeah, but why doesn't Firefox have the same bustage?
<bz> The problem is that it's NOT defined
<bz> in sunbird
<bz> if it's defined (as in Firefox, apparently), then things build
<bsmedberg> because firefox doesn't --enable-storage
<bsmedberg> and sunbird does
<bsmedberg> and this entire file is only build if storage is enabled

Comment 16

[Stefan Sitter [:ssitter]]

Hello,

    Linux galactica Dep Sb-Release
    MacOSX Darwin 7.9.0 hilo Dep Sb-Release
    WINNT 5.2 solaria Dep Sb-Release

are now burning for about 24 hours because of your check in. The error is:

c:/dev/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp: 
    In member function `nsresult nsFormHistory::OpenDatabase()':
c:/dev/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp:422: 
    error: `nsFormHistoryImporter' has not been declared

Please fix it or provide information how to solve this.

Comment 17

[Darin Fisher]

The problem here is that --enable-storage without --enable-places is now broken.  I think that the correct fix is probably to make --enable-storage define MOZ_MORKREADER.  This change would need to be made to configure.in.

Comment 18

[Darin Fisher]

Another choice would be to make satchel revert to using mork if MOZ_MORKREADER is not defined ;-)

Comment 19

[Darin Fisher]

Attachment: possible bustage fix

This patch attempts to fix the problem by enabling use of storage in satchel if MOZ_STORAGE and MOZ_MORKREADER are both defined.  Otherwise, it assumes that mork is defined and uses the old code.

Comment 20

[Brian Ryner (not reading)]

Attachment: fix nsStorageFormHistory for gcc

Stop trying to make nsCOMPtr do something it wasn't meant to (point to classes that multiply inherit nsISupports).

Comment 21

[Darin Fisher]

Attachment: simpler bustage fix [fixed-on-trunk]

bryner suggested this fix instead as he intended it to be possible to run satchel with storage but without migration from the old mork format.

Comment 22

[Wolfgang Rosenauer [:wolfiR]]

Sunbird build from 1.8 branch is still broken because of the change to toolkit/components/build/Makefile.in
Trying to build nsToolkitCompsModule results in:

In file included from ./../autocomplete/src/nsAutoCompleteMdbResult.h:42,
                 from nsToolkitCompsModule.cpp:51:
../../../dist/include/autocomplete/nsIAutoCompleteResultTypes.h:23:17: error: mdb.h: No such file or directory

Comment 23

[Wolfgang Rosenauer [:wolfiR]]

Attachment: sunbird fix

This fixes sunbird but I have no clue if it would break other combinations.

Comment 24

[Brett Wilson]

Can you ask bryner to look at this? I'm not sure what he had in mind for the build system.