Closed Bug 437243 Opened 12 years ago Closed 2 years ago

Expose frecency in query result nodes

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED INACTIVE

People

(Reporter: dietrich, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

The frecency value for a URI should be exposed as a property of nsINavHistoryResultNode.
Priority: -- → P1
Target Milestone: --- → Firefox 3.1
Blocks: 437245
Blocks: 413944
Priority: P1 → P2
Priority: P2 → P1
Blocks: 455651
Attached patch fix (obsolete) — Splinter Review
cleaning out my patch queue. will follow up w/ sorting, but want to get first-review going.
Assignee: nobody → dietrich
Attachment #347089 - Flags: review?(mak77)
Blocks: 411591
sorting will be taken care of in bug 411591.
Comment on attachment 347089 [details] [diff] [review]
fix

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl
>--- a/toolkit/components/places/public/nsINavHistoryService.idl
>+++ b/toolkit/components/places/public/nsINavHistoryService.idl
>@@ -187,6 +187,8 @@
>    * for the uri represented by this node. Otherwise this is an empty string.
>    */
>   readonly attribute AString tags;
>+
>+  readonly attribute long frecency;
> };

Are we sure we want to expose the "frecency" name? for Library most likely we will use "Relevance", i guess if we should do the same here hiding the "frecency" implementation.

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -6172,6 +6173,7 @@
> 
>   PRUint32 accessCount = aRow->AsInt32(kGetInfoIndex_VisitCount);
>   PRTime time = aRow->AsInt64(kGetInfoIndex_VisitDate);
>+  PRInt32 frecency = aRow->AsInt32(kGetInfoIndex_Frecency);
> 
>   // favicon
>   nsCAutoString favicon;
>@@ -6227,6 +6229,8 @@
>     if (!*aResult)
>       return NS_ERROR_OUT_OF_MEMORY;
> 
>+    (*aResult)->mFrecency = frecency;
>+
>     if (itemId != -1) {
>       (*aResult)->mItemId = itemId;
>       (*aResult)->mDateAdded = aRow->AsInt64(kGetInfoIndex_ItemDateAdded);
>@@ -6245,6 +6249,9 @@
>                                                favicon, session);
>     if (! *aResult)
>       return NS_ERROR_OUT_OF_MEMORY;
>+
>+    (*aResult)->mFrecency = frecency;
>+
>     NS_ADDREF(*aResult);
>     return NS_OK;
>   }

couldn't be frecency an optional input for new nsNavHistoryVisitResultNode or new nsNavHistoryResultNode instead of being added later? Do you think is overkill since resultNode is also basic class for containers?

also, mFrecency should be inited somewhere, i can't find the init in nsNavHistoryResult.cpp


>diff --git a/toolkit/components/places/src/nsNavHistoryResult.h b/toolkit/components/places/src/nsNavHistoryResult.h
>--- a/toolkit/components/places/src/nsNavHistoryResult.h
>+++ b/toolkit/components/places/src/nsNavHistoryResult.h
>@@ -262,6 +262,8 @@
>     { return nsNavHistoryResultNode::GetPropertyBag(aBag); } \
>   NS_IMETHOD GetTags(nsAString& aTags) \
>     { return nsNavHistoryResultNode::GetTags(aTags); } \
>+  NS_IMETHOD GetFrecency(PRInt32* aFrecency) \
>+    { return nsNavHistoryResultNode::GetFrecency(aFrecency); } \
> 
> #define NS_FORWARD_COMMON_RESULTNODE_TO_BASE \
>   NS_FORWARD_COMMON_RESULTNODE_TO_BASE_NO_GETITEMMID \
>@@ -291,6 +293,7 @@
>   NS_IMETHOD GetUri(nsACString& aURI)
>     { aURI = mURI; return NS_OK; }
>   NS_IMETHOD GetTags(nsAString& aTags);
>+  NS_IMETHOD GetFrecency(PRInt32* type);

type?

>diff --git a/toolkit/components/places/tests/unit/test_437243_frecency.js b/toolkit/components/places/tests/unit/test_437243_frecency.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/test_437243_frecency.js
>@@ -0,0 +1,121 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places unit test code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Dietrich Ayala <dietrich@mozilla.com>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Get history service
>+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+              getService(Ci.nsINavHistoryService);
>+var gh = Cc["@mozilla.org/browser/global-history;2"].
>+         getService(Ci.nsIBrowserHistory);
>+var dbConn = histsvc.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
>+
>+
>+
>+
>+/**
>+ * Adds a test URI visit to the database, and checks for a valid place ID.
>+ *
>+ * @param aURI
>+ *        The URI to add a visit for.
>+ * @param aReferrer
>+ *        The referring URI for the given URI.  This can be null.
>+ * @returns the place id for aURI.
>+ */
>+function add_visit(aURI, aReferrer) {
>+  var placeID = histsvc.addVisit(aURI,
>+                                 Date.now() * 1000,
>+                                 aReferrer,
>+                                 histsvc.TRANSITION_TYPED, // user typed in URL bar
>+                                 false, // not redirect
>+                                 0);
>+  dump("### Added visit with id of " + placeID + "\n");
>+  do_check_true(placeID > 0);
>+  do_check_true(gh.isVisited(aURI));
>+  return placeID;
>+}

s/placedID/visitId/
Yes we have this miscomprehension in other tests too :\

>+
>+// main
>+function run_test() {
>+  // add a visit
>+  var testURI = uri("http://mozilla.com");
>+  var placeId = add_visit(testURI);

s/placeId/visitId/ (And so on...)

>+
>+  // set starting value
>+  frecency = 999;
>+  setFrecency(placeId, frecency);

Well, this is wrong since we don't have the placeId, so remaining part of the test needs a revise
Attachment #347089 - Flags: review?(mak77)
Attached patch v2 (obsolete) — Splinter Review
comments fixed
Attachment #347089 - Attachment is obsolete: true
Attachment #349036 - Flags: review?(mak77)
Comment on attachment 349036 [details] [diff] [review]
v2

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -135,7 +135,8 @@
>   mDateAdded(0),
>   mLastModified(0),
>   mIndentLevel(-1),
>-  mViewIndex(-1)
>+  mViewIndex(-1),
>+  mFrecency(0)
> {
>   mTags.SetIsVoid(PR_TRUE);
> }
>@@ -222,6 +223,18 @@
>       result->AddAllBookmarksObserver(query);
>     }
>   }
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsNavHistoryResultNode::GetRelevance(PRInt32* aRelevance) {
>+  // Only URI nodes have frecency
>+  if (!IsURI()) {
>+    *aRelevance = -1;
>+    return NS_OK;
>+  }

is -1 wanted here? We init frecency to 0, -1 means "needs to be recalculated", maybe it's better returning 0? We will have negative frecency values (bookmarks whose frecency has not been recalculated yet) so -1 will overlaps with "had 1 visit and now we have to recalculate it", while 0 overlaps with "should not be showed if the query is based on frecency"...


>diff --git a/toolkit/components/places/tests/unit/test_437243_frecency.js b/toolkit/components/places/tests/unit/test_437243_frecency.js
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Get history service
>+var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+              getService(Ci.nsINavHistoryService);
>+var gh = Cc["@mozilla.org/browser/global-history;2"].
>+         getService(Ci.nsIBrowserHistory);
>+var dbConn = histsvc.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
>+
>+
>+
>+

much spacing here!


>+// main
>+function run_test() {
>+  // add a visit
>+  var testURI = uri("http://mozilla.com");
>+  add_visit(testURI);
>+
>+  // set starting value
>+  frecency = 999;
>+  setFrecency(testURI, frecency);
>+  do_check_eq(frecency, getFrecency(testURI));
>+
>+  // now query for the visit, setting sorting and limit such that
>+  // we should retrieve only the visit we just added
>+  var options = histsvc.getNewQueryOptions();
>+  options.sortingMode = options.SORT_BY_DATE_DESCENDING;
>+  options.maxResults = 1;
>+  options.resultType = options.RESULTS_AS_VISIT;
>+  var query = histsvc.getNewQuery();
>+  var result = histsvc.executeQuery(query, options);
>+  var root = result.root;
>+  root.containerOpen = true;
>+  var cc = root.childCount;
>+  do_check_true(cc, 1);
>+  var node = root.getChild(0);
>+  do_check_eq(node.uri, testURI.spec);
>+  do_check_eq(node.relevance, frecency);
>+  root.containerOpen = false;
>+}

can you put this inside a check function and do the check also for other cases (RESULTS_AS_VISITS, RESULTS_AS_URI and includeHidden). it's always better to be paranoid :)


>+
>+function getFrecency(aURL) {
>+  var sql = "SELECT frecency FROM moz_places_view WHERE url = :url LIMIT 1"

missing smemicolon


>+  var stmt = dbConn.createStatement(sql);
>+  var wrapper = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+                createInstance(Ci.mozIStorageStatementWrapper);

"Firefox 3.1 adds support for these features directly into the mozIStorageStatement interface, so this interface is essentially deprecated"


>+  wrapper.initialize(stmt);
>+  wrapper.params.url = aURL.spec;
>+  return wrapper.step() ? wrapper.row.frecency : -1;

stmt["url"] = aURL.spec;
do_check_true(stmt.executeStep());
var frecency = stmt.row.frecency;
stmt.finalize();
return frecency;

ok, a little more verbose, should be the new way of doing that


>+}
>+
>+function setFrecency(aURL, aFrecency) {
>+  var sql = "UPDATE moz_places_view SET frecency = :frecency WHERE url = :url"
>+  var stmt = dbConn.createStatement(sql);
>+  var wrapper = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+                createInstance(Ci.mozIStorageStatementWrapper);
>+  wrapper.initialize(stmt);
>+  wrapper.params.url = aURL.spec;
>+  wrapper.params.frecency = aFrecency;
>+  wrapper.execute();

as above


>+  dump("DONE\n");

uhm, what?

apart those, looks fine.
Attachment #349036 - Flags: review?(mak77) → review+
Attached patch v3Splinter Review
comments fixed. test added, as well as adding proper support for bookmark queries. i added a test for hidden, but probably not necessary, since frecency of zero doesn't mean hidden=true for queries here, only for the awesomebar.
Attachment #349036 - Attachment is obsolete: true
Attachment #349475 - Flags: review?(mak77)
Comment on attachment 349475 [details] [diff] [review]
v3

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -6262,6 +6263,10 @@
> 
>   PRUint32 accessCount = aRow->AsInt32(kGetInfoIndex_VisitCount);
>   PRTime time = aRow->AsInt64(kGetInfoIndex_VisitDate);
>+  PRInt32 frecency = aRow->AsInt32(kGetInfoIndex_Frecency);
>+  PRInt64 id = aRow->AsInt64(kGetInfoIndex_PageID);
>+  printf("GOT FRECENCY: %i for %lld\n", frecency, id);
>+

ifdef DEBUG?


>diff --git a/toolkit/components/places/tests/unit/test_437243_frecency.js b/toolkit>+
>+  // set starting value
>+  frecency = 999;
>+  setFrecency(testURI, frecency);
>+  do_check_eq(frecency, getFrecency(testURI));

i'd move this check inside setFrecency, so it's self contained

>+
>+  // test visit, not hidden
>+  checkQuery(testURI, frecency, Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT, false);
>+  // test visit, hidden
>+  checkQuery(testURI, frecency, Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT, true);
>+  // test uri, not hidden
>+  checkQuery(testURI, frecency, Ci.nsINavHistoryQueryOptions.RESULTS_AS_URI, false);
>+  // test bookmark query
>+  checkQuery(testURI, frecency, null, false, bmsvc.toolbarFolder);

where is test uri, hidden?

>+}
>+
>+function checkQuery(aURI, aFrecency, aResultType, aIncludeHidden, aBookmarkParentFolder) {
>+  var options = histsvc.getNewQueryOptions();
>+  options.sortingMode = options.SORT_BY_DATE_DESCENDING;
>+  var query = histsvc.getNewQuery();
>+  // If a bookmark folder is provided, query for it's first child.

nit: it's -> its

>+  // Otherwise query for the visit, setting sorting and limit such
>+  // that we should retrieve only the visit we just added.
>+  if (aBookmarkParentFolder) {
>+    query.setFolders([aBookmarkParentFolder], 1);

i would also set QUERY_TYPE_BOOKMARKS


a last thing could be check that for a folder/separator we get correctly a 0 frecency
Attachment #349475 - Flags: review?(mak77) → review+
Target Milestone: Firefox 3.1 → Firefox 3.2a1
> i would also set QUERY_TYPE_BOOKMARKS
nevermind this!
Target Milestone: Firefox 3.6a1 → ---
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Blocks: 437306
Not working on this atm. Patch is probably long-ago rotted.
Assignee: dietrich → nobody
Priority: P1 → --
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.