395735 - Instrument the location bar auto-complete to report accuracy

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

As we tweak the algorithm for auto completion in the location bar, we need actual data to understand which techniques work better than others.  For each time the user enters text in the location bar, what if we recorded which result they selected, and then aggregated and reported this information back?  During the beta testing phase of Firefox 3 we could then change the algorithm in the nightly builds, and get data on if we made things better or worse.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Note: I am only proposing we have this instrumentation in for beta testing, we would remove it before actually shipping Firefox 3.  And by "which result they selected" I mean in order, not the actual result (like they selected the third result).

Comment 2

[AndrewM]

Is this still wanted for beta; Beta 2 is coming soon :)

Comment 3

[Ed Lee :Mardak]

How much different is this from the adaptive feedback logging for bug 395739? It /could/ make use of 'how far down' the selected item is, but right now it just bumps up a counter pushing the result higher up for next time.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>How much different is this from the adaptive feedback logging for bug 395739?

adaptive feedback essentially invalidates the overall goal of this bug, which was for us to find ways to tweak our algorithm to produce better results.  If the results are adapting themselves based on user behavior, than that is likely to be the optimal solution.

Comment 5

[Ed Lee :Mardak]

Attachment: v1

This is the feedback part of the adaptive learning url bar autocomplete.

Adds nsIAutoCompleteFeedback and updates AutoCompleteController to call the feedback mechanism, but no actual implementation of the feedback (other than the testcase).

Comment 6

[Ed Lee :Mardak]

Bug 395739 will implement nsIAutoCompleteFeedback for nsNavHistory.

Comment 7

[Alex Polvi]

Just wanted to point out that we implemented this for Fx2... not sure how much of it is still relevant... but it may serve as a model for what we are trying to do.

http://mxr.mozilla.org/seamonkey/source/extensions/metrics/src/nsAutoCompleteCollector.cpp

Updating this code would allow us to integrate this into the Spectator project.

Comment 8

[Ed Lee :Mardak]

Attachment: v1.1

Fix up the testcase to correctly break references when it's done.

Comment 9

[Ed Lee :Mardak]

Comment on attachment 298855 [details] [diff] [review]
v1.1

I'll probably switch the instrumentation to use the observer service that the AutoCompleteController is already notifying.

Comment 10

[Ed Lee :Mardak]

Attachment: v2

Use the observer service instead of creating a new interface.

Comment 11

[Ed Lee :Mardak]

Attachment: v3

Consolidate all the moz_inputhistory feedback stuff here instead of having the dummy AutoCompleteFeedback do nothing.

Testcase is in bug 395739.

Comment 12

[Ed Lee :Mardak]

Comment on attachment 302076 [details] [diff] [review]
v3

Use the existing observer and store the data in moz_inputhistory.

Comment 13

[Ed Lee :Mardak]

Attachment: v3.1

Unbitrotting assuming that bug 401660 will land first which will help blocking bug 405320. Whereas this bug is in the line to help blocking bug 411293.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

Dietrich: this bug is blocking a review and landing of adaptive learning, which would make the awesome bar so much more awesome, any chance you will have cycles to get the review done?

Requesting blocking in the event that bug 393739 also receives blocking.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 304078 [details] [diff] [review]
v3.1


>+  // moz_inputhistory
>+  rv = mDBConn->TableExists(NS_LITERAL_CSTRING("moz_inputhistory"), &tableExists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!tableExists) {
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE TABLE moz_inputhistory ("
>+        "place_id INTEGER NOT NULL, "
>+        "input LONGVARCHAR NOT NULL, "
>+        "use_count INTEGER, "
>+        "PRIMARY KEY (place_id, input))"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+        "CREATE INDEX moz_inputhistory_placeindex ON moz_inputhistory (place_id)"));
>     NS_ENSURE_SUCCESS(rv, rv);
>   }

this index is redundant, i think. the index created by your primary key should be able to be utilized here. you should do EXPLAIN QUERY PLAN to confirm this though (and if i'm wrong, to confirm that your new index is in fact being used, if you haven't done this already).

>+  sql = NS_LITERAL_CSTRING(
>+    // Leverage the PRIMARY KEY (place_id, input) to insert/update entries
>+    "INSERT OR REPLACE INTO moz_inputhistory "
>+    // use_count will asymptotically approach the max of 10
>+    "SELECT h.id, IFNULL(i.input, ?2), IFNULL(i.use_count, 0) * .9 + 1 "
>+    "FROM moz_places h "
>+    "LEFT OUTER JOIN moz_inputhistory i ON i.place_id = h.id AND i.input = ?2 "
>+    "WHERE h.url = ?1");
>+  rv = mDBConn->CreateStatement(sql, getter_AddRefs(mDBFeedbackIncrease));
>   NS_ENSURE_SUCCESS(rv, rv);

nit: why ?2 before ?1

r=me with these fixed.

Comment 16

[Ed Lee :Mardak]

Attachment: v3.2

(In reply to comment #15)
> >+        "CREATE INDEX moz_inputhistory_placeindex ON moz_inputhistory (place_id)"));
> this index is redundant. you should do EXPLAIN QUERY PLAN to confirm this
Neato. For the INSERT OR REPLACE query to update use_counts, it's using moz_places_url_uniqueindex and sqlite_autoindex_moz_inputhistory as expected.

But the query to select adaptive results for bug 395739 uses the primary key of moz_places on moz_places.id because the query is a moz_inputhistory JOIN moz_places not a LEFT OUTER JOIN.

So I've removed the index because it's redundant in both cases.

> >+    "INSERT OR REPLACE INTO moz_inputhistory "
> >+    "SELECT h.id, IFNULL(i.input, ?2), IFNULL(i.use_count, 0) * .9 + 1 "
> >+    "FROM moz_places h "
> >+    "LEFT OUTER JOIN moz_inputhistory i ON i.place_id = h.id AND i.input = ?2 "
> >+    "WHERE h.url = ?1");
> nit: why ?2 before ?1
Well, I think I wrote the code to use the parameters before the query, but I reordered it.

Comment 17

[Damon Sicore (:damons)]

Comment on attachment 305377 [details] [diff] [review]
v3.2

a1.9+=damons

Comment 18

[Ed Lee :Mardak]

Testcase will land with bug 395739.

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.260; previous revision: 1.259
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.142; previous revision: 1.141
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.42; previous revision: 1.41
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.h,v  <--  nsNavHistoryExpire.h
new revision: 1.10; previous revision: 1.9
done