323492 - NavHistoryResult/Node refactor

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

This bug tracks Brett's large patch for results.

Comment 1

[Brett Wilson]

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

Comment 2

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

marking P1 blocker since it blocks brett's free time and other bugs. 

Comment 3

[Brett Wilson]

Attachment: Gigantor patch

This patch implemets the resutl refactor with incremental live updating.

Not done:
- Some frontend stuff (don't bother looking at .js or .xul files in this patch)
- Deleting URIs
- Keyword searching

Comment 4

[Brett Wilson]

Attachment: Just the new nsNavHistoryResult.cpp

This is just the new nsNavHiastoryResult.cpp file from the above patch. The diff makes it hard to see, and most of the code is new or moved anyway.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 209035 [details] [diff] [review]
Gigantor patch

>--- public/nsINavHistoryService.idl	18 Jan 2006 01:21:35 -0000	1.27
>+++ public/nsINavHistoryService.idl	20 Jan 2006 00:24:24 -0000
>+/**
>+ * This is a result node that includes a URL. Used for bookmark and URL
>+ * results, as well as a base class for visit information.
>+ */
>+[scriptable, uuid(b62daf70-936a-41f8-a57e-6d2a6598bca6)]
>+interface nsINavHistoryUrlResultNode : nsINavHistoryResultNode
>+{

The best term to use here is probably "URI"... and it seems like it should be all  caps in the interface name.

>+/**
>+ * When you reqest RESULT_TYPE_VISIT from query options, you will get this

typo: request

>+/**
>+ *
>+ */
>+/*[scriptable, uuid(42f6a740-11ad-4fd6-a135-d273335de669)]
>+interface nsINavHistoryCustomResultNode : nsINavHistoryContainerResultNode

Please just add a quick FIXME/TODO about why this interface is commented out.

>@@ -230,46 +377,48 @@ interface nsINavHistoryResultViewObserve
>    */
>   void onPerformActionOnCell(in wstring action, in long row, in nsITreeColumn column);
> };
> 
> 
> /**
>  * The result of a history/bookmark query.
>  *
>- * This class is also a result "node". Use those interfaces to access the
>- * toplevel children of this result.
>+ * This class is also a container result "node". Use those interfaces to access
>+ * the toplevel children of this result. Is it expanded by default.

Not really true now.

> [scriptable, uuid(25b45a94-3323-4c7b-910a-315f2c59bfb4)]
>-interface nsINavHistoryResult : nsINavHistoryResultNode
>+interface nsINavHistoryResult : nsISupports
> {
>   /**
>    * Call these functions to expand or collapse all elements in the tree.
>    */
>-  void expandAll();
>-  void collapseAll();
>+  //void expandAll();
>+  //void collapseAll();

What are your plans for these?

>@@ -288,16 +437,18 @@ interface nsINavHistoryResult : nsINavHi
>    */
>   void addObserver(in nsINavHistoryResultViewObserver observer,
>                    in boolean ownsWeak);
> 
>   /**
>    * Remove a tree builder observer.
>    */
>   void removeObserver(in nsINavHistoryResultViewObserver observer);
>+
>+  readonly attribute nsINavHistoryContainerResultNode root;

Add a comment for this.

>Index: src/nsNavBookmarks.cpp

Up to here.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 209035 [details] [diff] [review]
Gigantor patch

>--- src/nsNavBookmarks.cpp	5 Jan 2006 19:37:04 -0000	1.37
>+++ src/nsNavBookmarks.cpp	20 Jan 2006 00:24:24 -0000
>@@ -1063,16 +1063,53 @@ nsNavBookmarks::GetFolderType(PRInt64 aF
>+nsresult
>+nsNavBookmarks::ResultNodeForFolder(PRInt64 aID,
>+                                    nsNavHistoryQueryOptions *aOptions,
>+                                    nsNavHistoryResultNode **aNode)
>+{
...
>+  if (folderType.IsEmpty()) {
>+    *aNode = new nsNavHistoryFolderResultNode(title, 0, 0, EmptyCString(),
>+                                              aOptions, aID);

Check for out-of-memory.

>--- src/nsNavHistory.cpp	18 Jan 2006 01:01:41 -0000	1.52
>+++ src/nsNavHistory.cpp	20 Jan 2006 00:24:24 -0000
>+// nsNavHistory::CanLiveUpdateQuery
>+//
>+//    Returns true if this set of queries/options can be live-updated. That is,
>+//    we can look at a node and compare the stuff in it so that we can tell

This could be worded more clearly.  Compare it to what?

>+PRUint32
>+nsNavHistory::GetUpdateRequirements(nsCOMArray<nsINavHistoryQuery>* aQueries,
>+                                    nsNavHistoryQueryOptions* aOptions,
>+                                    PRBool* aHasKeywords)
>+{
>+  NS_ASSERTION(aQueries->Count() > 0, "Must have at least one query");
>+
>+  // first check if there are keywords
>+  *aHasKeywords = PR_FALSE;
>+  for (PRInt32 i = 0; i < aQueries->Count(); i ++) {
>+    (*aQueries)[i]->GetHasSearchTerms(aHasKeywords);
>+    if (*aHasKeywords)
>+      break;

Is there a difference between a "keyword" and a search term?  If not, I'd prefer if we could consistently use the latter, since keyword is a very overloaded word.

>+  }
>+
>+  // Whenever there is a maximum number of results, we must requery. This
>+  // is because we can't generally know if any given addition/change causes
>+  // the item to be in the top N items in the database.
>+  if (aOptions->MaxResults() > 0)
>+    return QUERYUPDATE_COMPLEX;
>+
>+  PRBool nonTimeBasedItems = PR_FALSE;
>+  for (PRInt32 i = 0; i < aQueries->Count(); i ++) {
>+    nsresult rv;
>+    nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface((*aQueries)[i], &rv);
>+    if (NS_FAILED(rv))
>+      return QUERYUPDATE_COMPLEX;

Hm, how can this happen?  This seems like it should at least issue a warning. (bonus points if the incoming array can have type nsNavHistoryQuery* instead of nsINavHistoryQuery*).

>+// nsNavHistory::EvaluateQueryForNode
>+//
>+//    This runs the node through the given queries to see if satisfies the
>+//    query conditions. Not every query parameters are handled by this code,
>+//    but we handle the most common ones so that performance is better.

Comment on what the return value indicates.

>+PRBool
>+nsNavHistory::EvaluateQueryForNode(nsCOMArray<nsINavHistoryQuery>* aQueries,
>+                                   nsNavHistoryQueryOptions* aOptions,
>+                                   nsNavHistoryUrlResultNode* aNode)
>+{
>+  // lazily created from the node's string when we need to match URIs
>+  nsCOMPtr<nsIURI> nodeUri;
>+
>+  nsresult rv;
>+  for (PRInt32 i = 0; i < aQueries->Count(); i ++) {
>+    PRBool hasIt;
>+    nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface((*aQueries)[i], &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);

This isn't what you want (the function returns a PRBool, not an nsresult).  Perhaps NS_ENSURE_SUCCESS(rv, PR_FALSE);

>+        // easy case: the URI is an exact match
>+        PRBool equals;
>+        rv = query->Uri()->Equals(nodeUri, &equals);
>+        NS_ENSURE_SUCCESS(rv, rv);

Same here.

>+        nsCAutoString queryUriString;
>+        query->Uri()->GetAsciiSpec(queryUriString);

It's not really clear to me why you want the spec as ASCII and not UTF8.

>+// nsNavHistory::DomainNameFromHostName
>+//
>+//    This does the www.mozilla.org -> mozilla.org and
>+//    foo.theregister.co.uk -> theregister.co.uk conversion
>+
>+void // static
>+nsNavHistory::DomainNameFromHostName(const nsCString& aHostName,
>+                                     nsACString& aDomainName)
>+{
>+  if (IsNumericHostName(aHostName)) {
>+    // easy case
>+    aDomainName = aHostName;
>+  } else {
>+    // compute the toplevel domain name
>+    PRInt32 tldLength = GetTLDCharCount(aHostName);
>+    if (tldLength < (int)aHostName.Length()) {

s/int/PRInt32/ (and constructor-style cast is generally preferred)

> // nsNavHistory::ExecuteQueries

Up to here.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 209035 [details] [diff] [review]
Gigantor patch

> // nsNavHistory::GroupByHost
> //
>-//    Here, we just hash every host name to get the unique ones. 256 is a guess
>-//    as to the upper bound on the number of unique hosts. The hash table will
>-//    grow when the number of elements is greater than 0.75 * the current size
>-//    of the table.
>+//    OPTIMIZATION: This parses the URI of each one that is coming in. This

of each node?

>-nsNavHistory::TitleForDomain(const nsString& domain, nsAString& aTitle)
>+nsNavHistory::TitleForDomain(const nsCString& domain, nsACString& aTitle)
> {
...
>-    aTitle = value;
>+    aTitle = NS_ConvertUTF16toUTF8(value);

Slightly more efficient:

CopyUTF16toUTF8(value, aTitle);


>--- src/nsNavHistory.h	18 Jan 2006 01:01:41 -0000	1.43
>+++ src/nsNavHistory.h	20 Jan 2006 00:24:24 -0000
>+  PRTime NormalizeTime(PRUint32 aRelative, PRTime aOffset);

This can be |static|, can't it?

>Index: src/nsNavHistoryQuery.cpp

up to here.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 209036 [details]
Just the new nsNavHistoryResult.cpp

nsNavHistoryResult.cpp:

>nsNavHistoryContainerResultNode::nsNavHistoryContainerResultNode(
>    const nsACString& aTitle, const nsACString& aIconURI,
>    PRUint32 aContainerType) :
>  nsNavHistoryResultNode(aTitle, 0, 0, aIconURI),
>  mResult(nsnull),
>  mContainerType(aContainerType),
>  mExpanded(PR_FALSE),
>  mChildrenReadOnly(PR_TRUE)
>{
>  // The container type must be a host or day. This constructor is used when
>  // we are grouping and will put a list of nodes to put in here. Therefore,

This comment confused me... what will put a list of nodes where?

>// nsNavHistoryContainerResultNode::IsChildrenVisible

This method name is a little off... maybe "AreChildrenVisible", or "HasVisibleChildren"?


>PRUint32
>nsNavHistoryContainerResultNode::FindInsertionPoint(
>    nsNavHistoryResultNode* aNode, SortComparator aComparator)
>{
...
>    else
>      beginRange = center + 1; // right site

s/site/side/

>// nsNavHistoryContainerResultNode::DoesChildNeedResorting

Add a comment for this function?

>PRInt32 PR_CALLBACK nsNavHistoryContainerResultNode::SortComparison_TitleLess(
>    nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure)
>{
...
>      value = aUrl->mUrl.Compare(PromiseFlatCString(bUrl->mUrl).get());

You only need to use PromiseFlatCString when dealing with nsACString, not with just nsCString.

>// nsNavHistoryContainerResultNode::SortComparison_URL*
>//
>//    Certain types of parent nodes are treated specially because URLs are not
>//    meaningful.

nit: maybe "because their URLs are not meaningful"... URLs in general are meaningful

>// nsNavHistoryContainerResultNode::FindChildUrl
>//
>//    Searches this folder for a node with the given URI. Returns true if found.
>//    Does not addref the node!

Is there ever a case where this function would return a null node, but also return true? It doesn't look like it... and in that case I'd suggest removing the addref ambiguity by making the node be the return value (instead of a redundant boolean).

>// nsNavHistoryContainerResultNode::FindChildFolder
>//
>//    Searches this folder for the given subfolder. Returns true if found.
>//    DOES NOT ADDREF.

Same here.

>//  nsNavHistoryContainerResultNode::FindChildContainerByName

Ditto.

>// nsNavHistoryContainerResultNode::FindChild

Just use mChildren.IndexOf(aNode)

>// nsNavHistoryContainerResultNode::MergeResults
>//
>//    This takes a fully grouped list of nodes and merges them into the

This also assumes that if the node list contains a container, that the container is already sorted, correct?

>nsresult
>nsNavHistoryContainerResultNode::RemoveChildAt(PRInt32 aIndex,
>                                               PRBool aIsTemporary)
>{
...
>    // If the removee was an open container, we may be removing more than just
>    // one row. In this case, lets just refresh our whole level. When there

s/lets/let's/

>    // are multple levels of containers, this usually means the list isn't very
>    // long and updating the visible list is not too heavyweight. We can
>    // optimize this to removing the correct rows if this is not the case.

s/removing/remove/

>// nsNavHistoryContainerResultNode::GetChild

I'm up to here.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 209036 [details]
Just the new nsNavHistoryResult.cpp

>// nsNavHistoryContainerResultNode::GetChild
>
>NS_IMETHODIMP
>nsNavHistoryContainerResultNode::GetChild(PRUint32 aIndex,
>                                          nsINavHistoryResultNode** _retval)
>{
>  if (! mExpanded)
>    return NS_ERROR_NOT_AVAILABLE;
>  if (aIndex >= NS_STATIC_CAST(PRUint32, mChildren.Count()))

Use a constructor-style cast here.

>nsNavHistoryQueryResultNode::nsNavHistoryQueryResultNode(
>    const nsACString& aTitle, PRUint32 aAccessCount, PRTime aTime,
>    const nsACString& aIconURI, nsINavHistoryQuery** aQueries,
>    PRUint32 aQueryCount, nsNavHistoryQueryOptions* aOptions) :
>  nsNavHistoryContainerResultNode(aTitle, aAccessCount, aTime, aIconURI,
>                                  nsINavHistoryResultNode::RESULT_TYPE_QUERY,
>                                  PR_TRUE),
>  mQueryURI(aIconURI),
>  mContentsValid(PR_FALSE),
>  mBatchInProgress(PR_FALSE)
>{
>  NS_ASSERTION(aQueryCount > 0, "Must have at least one query");
>  for (PRUint32 i = 0; i < aQueryCount; i ++)
>    mQueries.AppendObject(aQueries[i]);
>  aOptions->Clone(getter_AddRefs(mOptions));
>
>
>  nsNavHistory* history = nsNavHistory::GetHistoryService();
>  if (history)

Just assert if history is null -- it must have already been created to enter this code path, right?  This comes up a few other places as well.

>// nsNavHistoryQueryResultNode::RecursiveFindUrls
>//
>//    This function searches for matches for the given URL.
>//
>//    If aOnlyOne is set, it will terminate as soon as it finds a single match.
>//    (The return value of TRUE in this case indicates that a match was found.

Close your () somewhere.

>// nsNavHistoryQueryResultNode::OnVisit
>//
>//    Here we need to update all copies of the URL we have with the new visit
>//    count, and potentially add a new entry in our query. This is the most
>//    common update uperation and it is important that it be as efficient as

operation

>// nsNavHistoryQueryResultNode::OnTitleChanged
>//
>//    Find every node that matches this URL and rename it. We try to do
>//    incremental updates here, even when we are closed, because changing titles
>//    is easier than requerying if we are invalid.
>//
>//    This actually gets called a lot. Typically, we will get an AddURI message
>//    when the user visits the page, and then the title will be set asynchronously
>//    when the title element of the page is parsed.
>
>static void setTitleCallback(nsNavHistoryUrlResultNode* aNode,

This might need PR_CALLBACK in the signature on certain platforms (though I think OS2 VACPP may have been the last of those, and it's no longer supported).

>NS_IMETHODIMP
>nsNavHistoryQueryResultNode::OnTitleChanged(nsIURI* aURI,
>                                            const nsAString& aPageTitle,
>                                            const nsAString& aUserTitle,
>                                            PRBool aIsUserTitleChanged)
>{
...
>  // See if our queries have any keyword matching. In this case, we can't just
>  // replace the title on the items, but must redo the query. This could be
>  // handled more efficiently, but it is hard because keyword matching might
>  // match anything, including the title and other things.
>  if (mHasKeywords) {
>    return Refresh();

I think I mentioned this earlier, but "search terms" would be clearer than "keywords" given that we may be dealing with bookmarks.

>NS_IMETHODIMP
>nsNavHistoryQueryResultNode::OnPageChanged(nsIURI *aURI, PRUint32 aWhat,
>                         const nsAString &aValue)
>{
...
>      printf("Updating favicon for %s\n", newFavicon.get());

remove the printf before checking in, unless you really think it needs to be there for awhile.

>// nsNavHistoryResult::FillChildren
>//
>//    You can call this when the children are valid to re-query. The result
>//    will be slightly more efficient than removing the children and then
>//    calling FillChildren because we will only have to update the visible
>//    list once.

I'm a bit confused about how this comment fits with the code.  In particular, the way I read the comment, you can call this method on a node with valid children and they will be updated.  But, the code asserts that the child count is 0.  I'm also not sure I understand how you can know the children are valid before you requery.

>nsresult
>nsNavHistoryResult::PropertyBagFor(nsISupports* aObject,
>                                   nsIWritablePropertyBag** aBag)
>{
>  *aBag = nsnull;
>  if (mPropertyBags.Get(aObject, aBag) && *aBag)
>    return NS_OK;
>
>  nsresult rv = NS_NewHashPropertyBag(aBag);
>  NS_ENSURE_SUCCESS(rv, rv);
>  if (! mPropertyBags.Put(aObject, *aBag)) {
>    NS_RELEASE(*aBag);
>    *aBag = nsnull;
>    return NS_ERROR_OUT_OF_MEMORY;
>  }
>  return NS_OK;

I think this leaks the property bag.  NS_NewHashPropertyBag addrefs before return, and the hash table adds an additional reference to the object.

>// nsNavHistoryResult::GetColumnType
>
>nsNavHistoryResult::ColumnType
>nsNavHistoryResult::GetColumnType(nsITreeColumn* col)
>{
>  const PRUnichar* idConst;
>  col->GetIdConst(&idConst);
>  switch(idConst[0]) {
>    case 't':
>      return Column_Title;
>    case 'u':
>      return Column_URL;
>    case 'd':
>      return Column_Date;
>    case 'v':
>      return Column_VisitCount;

This feels like something that might generate a compiler warning (comparing a PRUnichar to a char).  Maybe you can have the case statements be like  case PRUnichar('c'):  ?

>// nsNavHistoryResult::SortTypeToColumnType
>//
>//    Places whether its ascending or descending into the given

it's

>nsNavHistoryResult::ColumnType
>nsNavHistoryResult::SortTypeToColumnType(PRUint32 aSortType,
>                                         PRBool* aDescending)
>{

...
>    default:
>      return Column_Unknown;

Maybe this should initialize aDescending to something, just for sanity.

>void
>nsNavHistoryResult::SetTreeSortingIndicator()
>{
...
>    element->SetAttribute(sortDirectionKey, NS_LITERAL_STRING(""));

EmptyString()

>void
>nsNavHistoryResult::InvalidateTree()
>{
>  if (! mTree)
>    return;
>  mTree->InvalidateRange(0, mVisibleElements.Length());

You can just call mTree->Invalidate()

>NS_IMETHODIMP
>nsNavHistoryResult::SortAll(PRUint32 aSortingMode)
>{
...
>    mTree->InvalidateRange(0, mVisibleElements.Length());

Also here.

>NS_IMETHODIMP
>nsNavHistoryResult::NodeForTreeIndex(PRUint32 index,
>                                     nsINavHistoryResultNode** aResult)
>{
>  if (index >= NS_STATIC_CAST(PRUint32, mVisibleElements.Length()))

ctor-style cast (same in some of the other methods).

>// nsNavHistoryResult::RowAdded
>//
>//    Called by nodes when to perform dynamic updates of the visible list.

s/when//

>//    This will add the item to the visible list and notifies the tree that

s/notifies/notify/

>void
>nsNavHistoryResult::RowChanged(PRInt32 aVisibleIndex)
>{
>  if (! mTree) {
>    NS_NOTREACHED("RowAdded should only be called when there is a tree");

You mean RowChanged, right?

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 209035 [details] [diff] [review]
Gigantor patch

>--- src/nsNavHistoryResult.h	4 Jan 2006 19:08:25 -0000	1.5
>+++ src/nsNavHistoryResult.h	20 Jan 2006 00:24:24 -0000
>+// nsNavHistoryUrlResultNode
>+
>+#define NS_IMPLEMENT_URLRESULT \
>+  NS_IMETHOD GetUrl(nsACString& aUrl) { aUrl = mUrl; return NS_OK; }
>+
>+class nsNavHistoryUrlResultNode : public nsNavHistoryResultNode,
>+                                  public nsINavHistoryUrlResultNode
>+{
>+public:
>+  nsNavHistoryUrlResultNode(const nsACString& aTitle, PRUint32 aAccessCount,
>+    PRTime aTime, const nsACString& aIconURI, const nsACString& aUrl);
>+
>+  NS_DECL_ISUPPORTS

NS_DECL_ISUPPORTS_INHERITED.  Same for the other result node subclasses.

not using _INHERITED will actually cause there to be a duplicate mRefCnt in the derived class that shadows the one in the base class.

Ok, so, r=me with all of that addressed.

Comment 11

[Brett Wilson]

Attachment: New patch with Brian's suggestions

Comment 12

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

Comment on attachment 209414 [details] [diff] [review]
New patch with Brian's suggestions

rs=[e-mail] for the api updates in the fe code.

Comment 13

[Brett Wilson]

On trunk

Comment 14

[Brett Wilson]

On 1.8 branch

Comment 15

[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