Expose frecency in query result nodes

RESOLVED INACTIVE

Status

()

Firefox
Bookmarks & History
RESOLVED INACTIVE
10 years ago
29 days ago

People

(Reporter: dietrich, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
The frecency value for a URI should be exposed as a property of nsINavHistoryResultNode.
(Reporter)

Updated

10 years ago
Priority: -- → P1
Target Milestone: --- → Firefox 3.1
(Reporter)

Updated

10 years ago
Blocks: 437245
(Reporter)

Updated

10 years ago
Blocks: 413944
(Reporter)

Updated

10 years ago
Priority: P1 → P2
(Reporter)

Updated

10 years ago
Priority: P2 → P1
(Reporter)

Updated

10 years ago
Blocks: 455651
(Reporter)

Comment 1

10 years ago
Created attachment 347089 [details] [diff] [review]
fix

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)
(Reporter)

Updated

10 years ago
Blocks: 411591
(Reporter)

Comment 2

10 years ago
sorting will be taken care of in bug 411591.

Comment 3

10 years ago
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)
(Reporter)

Comment 4

10 years ago
Created attachment 349036 [details] [diff] [review]
v2

comments fixed
Attachment #347089 - Attachment is obsolete: true
Attachment #349036 - Flags: review?(mak77)

Comment 5

10 years ago
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+
(Reporter)

Comment 6

10 years ago
Created attachment 349475 [details] [diff] [review]
v3

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 7

10 years ago
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+
(Reporter)

Updated

10 years ago
Target Milestone: Firefox 3.1 → Firefox 3.2a1

Comment 8

10 years ago
> i would also set QUERY_TYPE_BOOKMARKS
nevermind this!
(Reporter)

Updated

9 years ago
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
(Reporter)

Updated

9 years ago
Blocks: 437306
(Reporter)

Comment 10

8 years ago
Not working on this atm. Patch is probably long-ago rotted.
Assignee: dietrich → nobody

Updated

3 years ago
Priority: P1 → --

Comment 11

29 days ago
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
Last Resolved: 29 days ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.