367386 - Complex queries only return visited places

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dan Mills [:thunder]]

Complex queries (queries which are not simple bookmark folder lookups) add "visit_count > 0" to the sql select statement (except when the query uses an annotation).

This may be a common case for history searches, but when searching for bookmarks it is incorrect.  Thus, it should be configurable.  Furthermore, one should be able to query for a particular range of visit_count, not just > 0.

Comment 1

[Dan Mills [:thunder]]

Attachment: Adds minVisits and maxVisits as uri (query) options

I decided to put these in nsNavHistoryQuery, not in nsNavHistoryQueryOptions.  This feels more correct to me (the QueryOptions object has more "meta" options, like how many items to return and the like), but it means that these settings are ignored for "simple" bookmark queries.

I'm inclined to think that that is a problem with the way we do bookmark queries, and not a reason to put these options in nsNavHistoryQueryOptions.

Also, this patch sneaks in a debug message (wrapped in an ifdef DEBUG).  It's not very noisy and very useful when debugging, so we might want to leave it this way.

Comments? :-)

Comment 2

[(not reading, please use [e-mail] instead)]

some review comments from the peanut gallery:

1)
 
-  nsCAutoString commonConditions("visit_count > 0 ");
+  nsCAutoString commonConditions("");

I don't think you need the ("");

2)

-  //printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get());
+#ifdef DEBUG
+  printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get());
+#endif

I like this and find it useful myself, but just how noisy it is?  I'd hate to drive other non-places developers crazy with our noise.

3)
 
   // only bookmarked
-  if (aQuery->OnlyBookmarked()) {
+  if (NS_SUCCEEDED(aQuery->GetOnlyBookmarked(&hasIt)) && hasIt) {
     if (! aClause->IsEmpty())
       *aClause += NS_LITERAL_CSTRING(" AND ");

why did you make this change?

4)
 
     // all non-annotation queries return only visited items
+    if (! aClause->IsEmpty() && aCommonConditions.Length() > 0)
+      *aClause += NS_LITERAL_CSTRING(" AND ");

instead of doing aCommonConditions.Length() > 0, can you do aCommonConditions.IsEmpty()?

5)

+    query->GetMinVisits(&visits);

what about the return value of GetMinVisits()?  It's (currently) always NS_OK, but I think we still should check it.

6)

+    query->GetMaxVisits(&visits);

7)

+      if (NS_SUCCEEDED(rv)) {
+        query->SetMinVisits(visits);
+      } else {
+        NS_WARNING("Bad number for minVisits in query");
+      }

style wise, we might be following the convention of not adding { } for blocks of code with only one line.

also, should we be doing:

rv = query->SetMinVisits(visits);

8)

+    // max visits
+    } else if ( kvp.key.EqualsLiteral(QUERYKEY_MAX_VISITS)) {
+      PRInt32 visits = kvp.value.ToInteger((PRInt32*)&rv);
+      if (NS_SUCCEEDED(rv)) {
+        query->SetMaxVisits(visits);
+      } else {
+        NS_WARNING("Bad number for maxVisits in query");
+      }

same as #7

9)

+/* attribute PRInt32 minVisits; */
+NS_IMETHODIMP nsNavHistoryQuery::GetMinVisits(PRInt32* _retval)
+{
+  *_retval = mMinVisits;
+  return NS_OK;
+}

is it convention to still do NS_ENSURE_ARG_POINTER(_retval)?

10)

+/* attribute PRint32 maxVisits; */
+NS_IMETHODIMP nsNavHistoryQuery::GetMaxVisits(PRInt32* _retval)
+{
+  *_retval = mMaxVisits;
+  return NS_OK;
+}

same as #9

+NS_IMETHODIMP nsNavHistoryQuery::SetMaxVisits(PRInt32 aVisits)
+{
+  mMaxVisits = aVisits;
+  return NS_OK;
+}

11)

+  PRInt32 MinVisits() { return mMinVisits; }
+  PRInt32 MaxVisits() { return mMaxVisits; }

you've added these, but are we using them?  I bet we could in your patch, and avoid having to check rv from calling GetMin/MaxVisits() (from our C++).

Comment 3

[(not reading, please use [e-mail] instead)]

oops, I see that dan has requested r= from dietrich already.

Comment 4

[Dan Mills [:thunder]]

Hey, no problem!  I was going to pass it to you afterward, anyway.

1) Ok.

2) I understand the concern.  It's been so useful to me that I might be a little biased :)  I don't think it's very noisy, but I dunno.  We could wrap it (and others like it) in a PLACES_DEBUG ifdef.

3) For consistency with the rest of the function.  Other tests for query options there use the NS_METHODIMP versions of the getters.  I'm not really sure what the tradeoffs are here, though.

4) Done!

5, 6) Hmm, that function doesn't check the return values of other checks, for example:

query->GetHasBeginTime(&hasIt);

That is all over.  Should I change them all to check the return values?  Are we concerned about garbage values?

7, 8) Nod, old habits die hard.  Fixed.

9, 10) I don't know.  How do we find out?

11) Oh, I forgot to mention that in my comment.  a) It does seem to me like those should be there, but I'm not sure I have them hooked up right, and b) I think I have to change the uuid in the idl file, is that correct?

Comment 5

[Dan Mills [:thunder]]

Attachment: Adds minVisits and maxVisits, updated patch

Notes:

Now uses the non-xpcom methods MinVisits / MaxVisits when we have a concrete instance of our class.  Fixed to check fo success otherwise.

Mano, you mentioned using AppendLiteral instead of Append, but the specific instance you pointed out is actually an nsACString, which doesn't have that method.  Let me know if I'm missing something/confused...

Seth, I was clearly confused about your point #11.  I've changed to using those two where possible, but they're not really necessary.  We can nuke them if you/others think they're not worth it.

The interface now has a new uuid.  My uuidgen tool creates uupercased strings, I presume that's not a problem, but let me know.

Unit tests coming up in a separate patch.

Comment 6

[Dan Mills [:thunder]]

Attachment: Unit tests for minVisits/maxVisits

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 252955 [details] [diff] [review]
Adds minVisits and maxVisits, updated patch

just a couple of nits. otherwise r=me

>Index: src/nsNavHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v
>retrieving revision 1.104
>diff -u -8 -r1.104 nsNavHistory.cpp
>--- src/nsNavHistory.cpp	5 Jan 2007 19:58:56 -0000	1.104
>+++ src/nsNavHistory.cpp	26 Jan 2007 22:33:55 -0000
>@@ -3114,32 +3132,35 @@
>     } else {
>       *aClause += NS_LITERAL_CSTRING("h.url = ") + paramString;
>     }
>     aClause->Append(' ');
>   }
> 
>   // annotation
>   aQuery->GetHasAnnotation(&hasIt);
>-  if (! aClause->IsEmpty())
>-    *aClause += NS_LITERAL_CSTRING(" AND ");
>   if (hasIt) {
>+    if (! aClause->IsEmpty())
>+      *aClause += NS_LITERAL_CSTRING(" AND ");
>+
>     nsCAutoString paramString;
>     parameterString(aStartParameter + *aParamCount, paramString);
>     (*aParamCount) ++;
> 
>     if (aQuery->AnnotationIsNot())
>       aClause->AppendLiteral("NOT ");
>     aClause->AppendLiteral("EXISTS (SELECT id FROM moz_annos anno JOIN moz_anno_attributes annoname ON anno.anno_attribute_id = annoname.id WHERE anno.place_id = h.id AND annoname.name = ");
>     aClause->Append(paramString);
>     aClause->AppendLiteral(") ");
>     // annotation-based queries don't get the common conditions, so you get
>     // all URLs with that annotation
>   } else {
>     // all non-annotation queries return only visited items
>+    if (!(aClause->IsEmpty() || aCommonConditions.IsEmpty()))

should remove this comment, as it's no longer true.


>@@ -3172,16 +3193,31 @@
>                                 aQuery->EndTime());
>     rv = statement->BindInt64Parameter(aStartParameter + *aParamCount, time);
>     NS_ENSURE_SUCCESS(rv, rv);
>     (*aParamCount) ++;
>   }
> 
>   // search terms FIXME
> 
>+  // min and max visit count
>+  PRInt32 visits = aQuery->MinVisits();
>+  if (visits >= 0) {
>+    rv= statement->BindInt32Parameter(aStartParameter + *aParamCount, visits);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    (*aParamCount) ++;
>+  }
>+
>+  visits = aQuery->MaxVisits();
>+  if (visits >= 0) {
>+    rv= statement->BindInt32Parameter(aStartParameter + *aParamCount, visits);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    (*aParamCount) ++;
>+  }
>+

nit: spaces after rv in the |rv=| above.


>Index: src/nsNavHistoryQuery.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v
>retrieving revision 1.23
>diff -u -8 -r1.23 nsNavHistoryQuery.cpp
>--- src/nsNavHistoryQuery.cpp	8 Dec 2006 19:56:30 -0000	1.23
>+++ src/nsNavHistoryQuery.cpp	26 Jan 2007 22:33:55 -0000
>@@ -133,16 +133,18 @@
> // Components of a query string.
> // Note that query strings are also generated in nsNavBookmarks::GetFolderURI
> // for performance reasons, so if you change these values, change that, too.
> #define QUERYKEY_BEGIN_TIME "beginTime"
> #define QUERYKEY_BEGIN_TIME_REFERENCE "beginTimeRef"
> #define QUERYKEY_END_TIME "endTime"
> #define QUERYKEY_END_TIME_REFERENCE "endTimeRef"
> #define QUERYKEY_SEARCH_TERMS "terms"
>+#define QUERYKEY_MIN_VISITS "minVisits"
>+#define QUERYKEY_MAX_VISITS "maxVisits"
> #define QUERYKEY_ONLY_BOOKMARKED "onlyBookmarked"
> #define QUERYKEY_DOMAIN_IS_HOST "domainIsHost"
> #define QUERYKEY_DOMAIN "domain"
> #define QUERYKEY_FOLDER "folder"
> #define QUERYKEY_NOTANNOTATION "!annotation"
> #define QUERYKEY_ANNOTATION "annotation"
> #define QUERYKEY_URI "uri"
> #define QUERYKEY_URIISPREFIX "uriIsPrefix"
>@@ -303,16 +305,30 @@
>                       url_XAlphas))
>         return NS_ERROR_OUT_OF_MEMORY;
> 
>       AppendAmpersandIfNonempty(aQueryString);
>       aQueryString += NS_LITERAL_CSTRING(QUERYKEY_SEARCH_TERMS "=");
>       aQueryString += escapedTerms;
>     }
> 
>+    // min and max visits
>+    PRInt32 visits;
>+    if (NS_SUCCEEDED(query->GetMinVisits(&visits)) && visits >= 0) {
>+      AppendAmpersandIfNonempty(aQueryString);
>+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MIN_VISITS "="));
>+      AppendInt32(aQueryString, visits);
>+    }
>+
>+    if (NS_SUCCEEDED(query->GetMaxVisits(&visits)) && visits >= 0) {
>+      AppendAmpersandIfNonempty(aQueryString);
>+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MAX_VISITS "="));
>+      AppendInt32(aQueryString, visits);
>+    }
>+
>     // only bookmarked
>     AppendBoolKeyValueIfTrue(aQueryString,
>                              NS_LITERAL_CSTRING(QUERYKEY_ONLY_BOOKMARKED),
>                              query, &nsINavHistoryQuery::GetOnlyBookmarked);
> 
>     // domain (+ is host), only call if hasDomain, which means non-IsVoid
>     // this means we may get an empty string for the domain in the result,
>     // which is valid
>@@ -539,16 +555,32 @@
> 
>     // search terms
>     } else if (kvp.key.EqualsLiteral(QUERYKEY_SEARCH_TERMS)) {
>       nsCString unescapedTerms = kvp.value;
>       NS_UnescapeURL(unescapedTerms); // modifies input
>       rv = query->SetSearchTerms(NS_ConvertUTF8toUTF16(unescapedTerms));
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>+    // min visits
>+    } else if ( kvp.key.EqualsLiteral(QUERYKEY_MIN_VISITS)) {
>+      PRInt32 visits = kvp.value.ToInteger((PRInt32*)&rv);
>+      if (NS_SUCCEEDED(rv))
>+        rv = query->SetMinVisits(visits);
>+      else
>+        NS_WARNING("Bad number for minVisits in query");
>+
>+    // max visits
>+    } else if ( kvp.key.EqualsLiteral(QUERYKEY_MAX_VISITS)) {
>+      PRInt32 visits = kvp.value.ToInteger((PRInt32*)&rv);
>+      if (NS_SUCCEEDED(rv))
>+        rv = query->SetMaxVisits(visits);
>+      else
>+        NS_WARNING("Bad number for maxVisits in query");
>+

nit: assigning the result value here doesn't actually serve a purpose if that rv isn't actually checked :)

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 252955 [details] [diff] [review]
Adds minVisits and maxVisits, updated patch

r=sspitzer, once you address dietrich's comments.

one question of my own:

is this existing comment still correct?

     // all non-annotation queries return only visited items
+    if (!(aClause->IsEmpty() || aCommonConditions.IsEmpty()))
+      *aClause += NS_LITERAL_CSTRING(" AND ");

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 252956 [details] [diff] [review]
Unit tests for minVisits/maxVisits

r=sspitzer, sorry for the delay here thunder.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 252955 [details] [diff] [review]
Adds minVisits and maxVisits, updated patch

what-dietrich-said, and:

>Index: src/nsNavHistoryQuery.cpp
>===================================================================
>+    // min and max visits
>+    PRInt32 visits;
>+    if (NS_SUCCEEDED(query->GetMinVisits(&visits)) && visits >= 0) {
>+      AppendAmpersandIfNonempty(aQueryString);
>+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MIN_VISITS "="));
>+      AppendInt32(aQueryString, visits);
>+    }
>+
>+    if (NS_SUCCEEDED(query->GetMaxVisits(&visits)) && visits >= 0) {

Ugh, I wish we could use nsNavHistoryQuery here and then call the non-xpcom versions of these methods.

>+      AppendAmpersandIfNonempty(aQueryString);
>+      aQueryString.Append(NS_LITERAL_CSTRING(QUERYKEY_MAX_VISITS "="));
>+      AppendInt32(aQueryString, visits);
>+    }

Please use separate variables (minVisits and maxVisits).

>+    // min visits
>+    } else if ( kvp.key.EqualsLiteral(QUERYKEY_MIN_VISITS)) {

nit: remove the extra space before kvp.

>+    } else if ( kvp.key.EqualsLiteral(QUERYKEY_MAX_VISITS)) {

ditto.

r=mano otherwise.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 252956 [details] [diff] [review]
Unit tests for minVisits/maxVisits

>Index: unit/test_history.js
>===================================================================

>-  // test for valid place ID
>   do_check_true(placeID > 0);

The comment still applies.

r=mano.

Comment 13

[Dan Mills [:thunder]]

Attachment: Add minVisits and maxVisits as uri (query) options

Fixes everything noted above (thanks guys!), except:

>>+    if (NS_SUCCEEDED(query->GetMaxVisits(&visits)) && visits >= 0) {
>
>Ugh, I wish we could use nsNavHistoryQuery here and then call the non-xpcom
>versions of these methods.

I agree, it'd be nice.  But outside the scope of this bug, I think.

>>-  // test for valid place ID
>>   do_check_true(placeID > 0);
>
>The comment still applies.

I moved the comment to the top of the (small) function:

// adds a test URI visit to the database, and checks for a valid place ID

Unless you want me to address those, this patch is done!

Comment 14

[(not reading, please use [e-mail] instead)]

dan, with the latest trunk, test_history.js fails:

I think this is the root of the failure:

*** test pending
[Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x805700
34 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: ../../../../_t
ests/xpcshell-simple/test_places/unit/test_history.js :: run_test :: line 98"  d
ata: no]
*** FAIL ***

are you seeing that as well?

Comment 15

[(not reading, please use [e-mail] instead)]

also, doesn't this bug block some work you are doing in the bookmarks side bar?  

I only ask because besides test_history.js, there is no good way for me to test / verify it until the bookmarks sidebar patch lands, right?

Comment 16

[Dan Mills [:thunder]]

No, this is working for me:

h-112:~/sources/mozilla-trunk/obj/firefox/browser/components/places/tests thunder$ make check
../../../../_tests/xpcshell-simple/test_places/unit/test_annotations.js: PASS
../../../../_tests/xpcshell-simple/test_places/unit/test_history.js: PASS
../../../../_tests/xpcshell-simple/test_places/bookmarks/test_bookmarks.js: PASS
../../../../_tests/xpcshell-simple/test_places/bookmarks/test_livemarks.js: PASS

Did I leave something out in my patch?  I'm re-checking now.

Comment 17

[(not reading, please use [e-mail] instead)]

I'm on windows xp, in case that matters.

Comment 18

[Dan Mills [:thunder]]

Could be.  I'm on Mac.
My trunk is also a little out of date, I'm updating now.
I don't think I forgot to include anything in my patch.

Comment 19

[Dan Mills [:thunder]]

Ok, this doesn't happen to me on the mac with an up to date tree.  Testing on windows xp as soon as I get my env setup there...

Comment 20

[(not reading, please use [e-mail] instead)]

dan and I looked into my problem on windows, and it was a build error on my part.  (specifically, the way I had clobbered and rebuilding mozilla/toolkit and mozilla/browser did not cause the .idl to get rebuilt)

after addressing, the tests ran fine.

now that mano has landed bug #362292, it is also possible to test this fix in a places-bookmarks build, as with out it, you will only see bookmarks that had been visited when you search.

re-testing and landing now...

Comment 21

[(not reading, please use [e-mail] instead)]

hey dan, after I updated to get your new sidebar, I still don't see bookmarks in my sidebar search unless I've visited them.

Comment 22

[Asaf Romano (gone)]

Sure you've compiled in toolkit/components (and not just in toolkit/)?

Comment 23

[(not reading, please use [e-mail] instead)]

> Sure you've compiled in toolkit/components (and not just in toolkit/)?

yes, I made that mistake originally (comment #14), but I think I'm ok now (comment #20).

debugging, I am executing dan's code, but I'm still not getting bookmarks that I haven't visited.

Comment 24

[Dan Mills [:thunder]]

Hm, are you sure?

I think the problem you're seeing is that all your bookmarks are marked "hidden".  These won't show up regardless of my patch.

Open up your places.sqlite and run 'select user_title,hidden from moz_places;' and  see if your bookmarks are all hidden.

I'll spin off a bug for that.

Comment 25

[Dan Mills [:thunder]]

Spun off: Bug 369887

Comment 26

[(not reading, please use [e-mail] instead)]

I'll use the very cool sql storage inspector addon (https://addons.mozilla.org/firefox/3072/) to see if that is what's happening.

if so, I'll take the discussion to bug #369887, and land your patch.

thanks dan!

Comment 27

[Dan Mills [:thunder]]

Hey Seth, neat extension, btw.

I went ahead and also tested whether changing those rows by hand (using the sqlite command-line app to run UPDATE queries) to unhide them would make them show up in searches, and it does.  So the problem is definitely (on my env at least) that everything is hidden.

Note that you need to restart minefield if it's running when you change the db, so that it loads it back into memory.

Comment 28

[(not reading, please use [e-mail] instead)]

sorry for the long delay here, dan.

I'm seeing what dan is seeing, w.r.t. hidden and visit count.

I'll go ahead and land dan's patch, and take the discussion into bug #369887, about issues with hidden.

Comment 29

[(not reading, please use [e-mail] instead)]

fixed landed on the trunk, finally!  sorry for the delay, dan.

Checking in browser/components/places/tests/unit/test_history.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_history.js,v  <--  te
st_history.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <-
-  nsINavHistoryService.idl
new revision: 1.51; previous revision: 1.50
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.105; previous revision: 1.104
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v  <--  nsN
avHistoryQuery.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.h,v  <--  nsNav
HistoryQuery.h
new revision: 1.11; previous revision: 1.10
done

Comment 31

[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