564270 - Can not search Places by transition type

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

Querying Places does not expose the transition property. Exposing this allows one to search through Places for downloads or redirects, for example.

Comment 1

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch to add functionality along with one test.

Comment 2

[Marco Bonardo [:mak]]

if it's not too hard, would be cool to be able to search for more transitions, like has [4,5,6]. I guess it could be useful in a couple cases and we should just replace = with IN...
just a thought though :)

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

I thought about that earlier today but wasn't sure if it would be useful. I'll add it now along with a better test case (forgot to test tokenizing).

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 443948 [details] [diff] [review]
Patch to add functionality along with one test.

Unsetting review request since comment 3 indicates you're working on a new patch.

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Adds searching on a set of transitions.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 444176 [details] [diff] [review]
Adds searching on a set of transitions.

Totally my fault for not thinking about this sooner, but you should model transitions on {get,set}Folders and folderCount, rather than being an nsIVariant attribute.  Sorry. :(  (The reason we used an nsIVariant attribute for tags was that tags are strings, and that was the best choice for an array of strings.)

Except for some nits, though, which I'll address after a new patch, this patch looks pretty good so far.

Comment 7

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Adds searching on a set of transitions with minor revision on manner in which data is stored.

Comment 8

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changed uuid in interface.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 444514 [details] [diff] [review]
Changed uuid in interface.

This looks great.  Most of these comments are just nits.

>diff -r fe433627820d toolkit/components/places/public/nsINavHistoryService.idl

>-[scriptable, uuid(081452e5-be5c-4038-a5ea-f1f34cb6fd81)]
>+[scriptable, uuid(dc87ae79-22f1-4dcf-975b-852b01d210cb)]
> interface nsINavHistoryResultNode : nsISupports

This is the wrong iface.  You're modifying nsINavHistoryQuery, so its uuid is the one that needs changing.

>   /**
>+   * limits results to items that are marked with one of the transitions
>+   */
>+  void getTransitions([optional] out unsigned long count,
>+               	      [retval,array,size_is(count)] out unsigned long transitions);
>+  readonly attribute unsigned long transitionCount;

Align the second line of getTransitions() with the opening parenthesis, and never use tabs to indent, only spaces.

Please use full, capitalized and punctuated sentences in IDL comments, like the other comments in this file do.

>+  /**
>+   * For the special result type RESULTS_AS_TAG_CONTENTS we can define only
>+   * one folder that must be a tag folder. This is not recursive so results
>+   * will be returned from the first level of that folder.
>+   */
>+  void setTransitions([const,array, size_is(transitionCount)] in unsigned long transitions,
>+                      in unsigned long transitionCount);

That's setFolders()'s comment.

I know setFolders()'s second param is folderCount, but let's use "count" for the second param name here, to be consistent with getTransitions().

>diff -r fe433627820d toolkit/components/places/src/nsNavHistory.cpp

>+  // transitions
>+  const nsTArray<PRUint32> &transitions = aQuery->Transitions();

Keep & and * with the type, here and elsewhere, i.e.,

  const nsTArray<PRUint32>& transitions = aQuery->Transitions();

Older code doesn't do that, but we're using that style in new code.

You'll need to update nsNavHistory.cpp in a couple of more spots:

nsNavHistory::GetUpdateRequirements: Return QUERYUPDATE_COMPLEX if any of the queries specifies a transition, because EvaluateQueryForNode compares an nsNavHistoryResultNode to a query to see if it matches the query, but we don't expose transitions in result nodes, so that's not possible.  (nsINavHistoryFullVisitResultNode exposes it, but AFAICT we don't actually allow clients to get at those yet...)

IsOptimizableHistoryQuery: Return false if any of the queries specifies a transition.

>diff -r fe433627820d toolkit/components/places/src/nsNavHistoryQuery.cpp

>@@ -519,16 +520,24 @@ nsNavHistory::QueriesToQueryString(nsINa
>       AppendAmpersandIfNonempty(queryString);
>       queryString += NS_LITERAL_CSTRING(QUERYKEY_TAG "=");
>       queryString += escapedTag;
>     }
>     AppendBoolKeyValueIfTrue(queryString,
>                              NS_LITERAL_CSTRING(QUERYKEY_NOTTAGS),
>                              query,
>                              &nsINavHistoryQuery::GetTagsAreNot);
>+		
>+		//transitions

This first line is all whitespace, so please remove the whitespace, and use spaces to indent the second line.  Also, add a space between the // and T.

> // TokenizeQueryString
> 
> nsresult
> TokenizeQueryString(const nsACString& aQuery,
>                     nsTArray<QueryKeyValuePair>* aTokens)
> {
>+

Remove this extra line.

>@@ -795,30 +806,47 @@ nsNavHistory::TokensToQueries(const nsTA
>       if (!tags.Contains(tag)) {
>         NS_ENSURE_TRUE(tags.AppendElement(tag), NS_ERROR_OUT_OF_MEMORY);
>       }
> 
>     // not tags
>     } else if (kvp.key.EqualsLiteral(QUERYKEY_NOTTAGS)) {
>       SetQueryKeyBool(kvp.value, query, &nsINavHistoryQuery::SetTagsAreNot);
> 
>+      // transition
>+    } else if (kvp.key.EqualsLiteral(QUERYKEY_TRANSITION)) {

Align the comment with the closing brace, like the other comments around here.

>+      nsresult rv;

rv is already declared above.  Is there a reason you're shadowing it here?

>+      PRUint32 transition = kvp.value.ToInteger(reinterpret_cast<PRInt32*>(&rv));

Some of the code around here casts rv, but it's not needed.  (ToInteger takes an nsresult reference (and nsresult happens to be typedef'ed to PRUint32).)

>+      if (NS_SUCCEEDED(rv)) {
>+        if (!transitions.Contains(transition))
>+          NS_ENSURE_TRUE(transitions.AppendElement(transition), NS_ERROR_OUT_OF_MEMORY);

Lines should generally be 80 chars or less.  Here, move the second arg to a new line.

>+      } else {

Some old code does this, but don't cuddle else, i.e., do this instead:

  }
  else {

https://wiki.mozilla.org/Places/Coding_Style

>@@ -1059,16 +1092,48 @@ NS_IMETHODIMP nsNavHistoryQuery::GetMaxV
>   return NS_OK;
> }
> NS_IMETHODIMP nsNavHistoryQuery::SetMaxVisits(PRInt32 aVisits)
> {
>   mMaxVisits = aVisits;
>   return NS_OK;
> }
> 
>+// transitions
>+// this code is written exactly in the style of GetFolders and SetFolders
>+NS_IMETHODIMP nsNavHistoryQuery::GetTransitions(PRUint32 *aCount, PRUint32 **aTransitions)

No need for these comments.  Also, move the second param to a new line to keep under 80 chars per line.

>+  PRUint32 count = mTransitions.Length();
>+  PRUint32 *transitions = nsnull;
>+  if (count > 0) {
>+    transitions = static_cast<PRUint32*>
>+      (nsMemory::Alloc(count * sizeof(PRUint32)));

I know you're modeling this on GetFolders(), which is good, but use NS_Alloc with a reinterpret_cast here, like GetTags() does, since it's preferred.

>+    NS_ENSURE_TRUE(transitions, NS_ERROR_OUT_OF_MEMORY);
>+    for (PRUint32 i = 0; i < count; ++i) {
>+      transitions[i] = mTransitions[i];
>+    }
>+  }
>+  *aCount = count;
>+  *aTransitions = transitions;
>+	return NS_OK;

Replace the tabs with spaces.

>+NS_IMETHODIMP nsNavHistoryQuery::GetTransitionCount (PRUint32 *aCount)

Remove the space between the method name and opening paren.

>+{
>+  *aCount = mFolders.Length();
>+  return NS_OK;
>+}
>+NS_IMETHODIMP nsNavHistoryQuery::SetTransitions(const PRUint32 *aTransitions,
>+    PRUint32 aTransitionCount)
>+{
>+  if (!mTransitions.ReplaceElementsAt(0, mTransitions.Length(), aTransitions, aTransitionCount))
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  return NS_OK;
>+}

Add blank lines between GetTransitions(), GetTransitionCount(), and SetTransitions(), and move them down below SetFolders(), since the transitions and folders methods are similar.

>diff -r fe433627820d toolkit/components/places/src/nsNavHistoryQuery.h

>+  nsresult SetTransitions(const nsTArray<PRUint32>& aTransitions)
>+  {
>+    if (!mTransitions.ReplaceElementsAt(0, mTransitions.Length(), aTransitions))
>+      return NS_ERROR_OUT_OF_MEMORY;
>+
>+    return NS_OK;
>+  }
>   PRBool TagsAreNot() { return mTagsAreNot; }
>+  const nsTArray<PRUint32>& Transitions() const { return mTransitions; }

Move SetTransitions() down below Transitions() (like Tags() and SetTags()).  That header is becoming a little hard to read, so also add a blank line between those two and the rest.

>diff -r fe433627820d toolkit/components/places/tests/queries/test_transition.js

Let's call this file test_transitions.js -- plural, like the new attribute.

>diff -r fe433627820d toolkit/components/places/tests/queries/test_transition.js

>+ * The Initial Developer of the Original Code is Mehdi Mulani

This should read:

 * The Initial Developer of the Original Code is
 * the Mozilla Foundation.

>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ * Mehdi Mulani ([e-mail])

Really small nit while I'm here, indent your name two spaces:

 * Contributor(s):
 *   Mehdi Mulani ([e-mail])

>+//titles have a prime page # iff site is a download

What's that mean?  Could you remove or expand it?

>+var testData = [
>+  {isInQuery: false, isVisit: true, isDetails: true, title: "page 1",
>+   uri: "http://mozilla.com/", lastVisit: old, isTag: true,
>+   tagArray: ["moz"], transType: Ci.nsINavHistoryService.TRANSITION_TYPED },

Is there a reason you're tagging these?  Also, please format each like this so they're easier to read:

var testData = [
  {
    isInQuery: false,
    isVisit: true,
    ...
  },

>+function run_test() {

You do several distinct sub-tests inside here.  It would be better to break them up into separate functions.  For example, a lot of our tests create an array of functions (or objects with methods) and then use run_test() to call each one.  But, see my next comment.

>+
>+  // This function in head_queries.js creates our database with the above data
>+  populateDB(testData);
>+
>+  //first we check that that the query deserializes correctly
>+  var options = {};
>+  var queries = [];
>+  PlacesUtils.history.queryStringToQueries("place:transition=" + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD, queries, {}, options);

It's good that you're testing serialization and deserialization, but there's actually a whole other query (de)serialization test that you should update instead:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/queries/test_querySerialization.js

If it helps, here's how I updated it when I added tags to queries:

https://bugzilla.mozilla.org/attachment.cgi?id=375392&action=diff#a/toolkit/components/places/tests/queries/test_querySerialization.js_sec1

You just define a "matches" function to compare two queries, and add a "runs" array that contains your test functions, each of which updates the passed-in query.  The test handles everything else.  Then in your new test here, you only need to make some new queries with transitions and ensure that they return the proper results.

I'll take a closer look at the test once you post a new patch, but a couple of other comments:

Since you're adding transitions and not just downloads, you should check that all transition types work as expected.

Again, keep all lines 80 chars or less.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> You do several distinct sub-tests inside here.  It would be better to break
> them up into separate functions.  For example, a lot of our tests create an
> array of functions (or objects with methods) and then use run_test() to call
> each one.  But, see my next comment.
Example: http://mxr.mozilla.org/mozilla-central/source/storage/test/unit/test_js_helpers.js

Comment 11

[Drew Willcoxon :adw]

One other thing you should test is live update:  Execute a transitions query, open the result's root, add some visits with transitions that should match the query, and make sure those visits appear in the root.  That's relevant to GetUpdateRequirements that I mentioned.

Comment 12

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changed previous patch per adw's review.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 444964 [details] [diff] [review]
Changed previous patch per adw's review.

Looks great, one more small iteration should do it.

>diff -r fe433627820d toolkit/components/places/src/nsNavHistory.cpp

>@@ -2294,36 +2294,44 @@ nsNavHistory::GetUpdateRequirements(cons

>+  if (queryContainsTransitions)
>+    return QUERYUPDATE_COMPLEX;
>+
>   if (aOptions->ResultType() ==
>       nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY)
>     return QUERYUPDATE_COMPLEX_WITH_BOOKMARKS;

Let's swap the order of these two conditionals.  QUERYUPDATE_COMPLEX_WITH_BOOKMARKS is a subset of QUERYUPDATE_COMPLEX, so it needs to come first.

>diff -r fe433627820d toolkit/components/places/src/nsNavHistoryQuery.cpp

>+NS_IMETHODIMP nsNavHistoryQuery::GetTransitions(PRUint32* aCount,
>+    PRUint32** aTransitions)

Nit: Align the second line with the opening paren on the first.

>+{
>+  PRUint32 count = mTransitions.Length();
>+  PRUint32* transitions = nsnull;
>+  if (count > 0) {
>+    transitions = (PRUint32*)NS_Alloc(count);

No C-style casts -- here, use a reinterpret_cast.  NS_Alloc takes a PRSize (size_t), so you need to pass count * sizeof(PRUint32), like you did before.  GetTags() has an NS_Alloc example if you need.

>+NS_IMETHODIMP nsNavHistoryQuery::SetTransitions(const PRUint32* aTransitions,
>+    PRUint32 aCount)

Nit: Same as above.

>diff -r fe433627820d toolkit/components/places/tests/queries/test_querySerialization.js

>+  // transitions
>+  {
>+    desc: "tests nsINavHistoryQuery.getTransitions",
>+    matches: function (aQuery1, aQuery2) {
>+      var q1Trans = aQuery1.getTransitions();
>+      var q2Trans = aQuery2.getTransitions();
>+      if (q1Trans.length !== q2Trans.length)
>+        return false;
>+      for (let i = 0; i < q1Trans.length; i++) {
>+        if (q2Trans.indexOf(q1Trans[i]) < 0)
>+          return false;
>+      }
>+      for (let i = 0; i < q2Trans.length; i++) {
>+        if (q1Trans.indexOf(q1Trans[i]) < 0)

Typo: s/q1Trans[i]/q2Trans[i]/ :)

>+    runs: [
>+      function (aQuery, aQueryOptions) {
>+        aQuery.setTransitions([],0);

Nit: Put a space between args: aQuery.setTransitions([], 0);

>+      function (aQuery, aQueryOptions) {
>+        aQuery.setTransitions([Ci.nsINavHistoryService.TRANSITION_DOWNLOAD],1);

Nit: Space between args here too.  To keep from going over 80 chars you can move the second arg to a new line.

>+      function (aQuery, aQueryOptions) {
>+        aQuery.setTransitions([Ci.nsINavHistoryService.TRANSITION_TYPED,
>+                               Ci.nsINavHistoryService.TRANSITION_BOOKMARK],2);

Nit: ditto.

>diff -r fe433627820d toolkit/components/places/tests/queries/test_transitions.js

>+var testData = [
>+  {
>+    isInQuery: false,
>+    isVisit: true,
>+    isDetails: true,
>+    title: "page 0",
>+    uri: "http://mozilla.com/",
>+    lastVisit: old,

old doesn't seem to be defined anywhere.  And I don't think you need isDetails, but I could be wrong.

>+  {
>+    isInQuery: true,
>+    isVisit: true,
>+    isDetails: true,
>+    title: "page 10",
>+    uri: "http://arewefastyet.com/",
>+    lastVisit: old,
>+    transType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD
>+  }];

Nit: The array's closing bracket should be on a new line, left-aligned.  Also, please make a blank line after that so it doesn't run into the following line.

>+// sets of indices of testData array by transition type
>+var testDataTyped = [0,5,7,9];
>+var testDataDownload = [1,2,4,6,10]; 
>+var testDataBookmark = [3,8];

testDataDownload doesn't seem to be used anywhere.

Uber nit: spaces between array elements: [3, 8].  Also, the testDataDownload line has an unnecessary space at the end.

>+  

A couple of unnecessary spaces here too.

>+function run_test() {

>+  var data = [1,2,3,4,6,8,10];

Uber nit: same as above.

>+  compareQueryToTestData("place:transition=" +
>+                         Ci.nsINavHistoryService.TRANSITION_DOWNLOAD +
>+                         "&transition=" +
>+                         Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
>+                         data);
>+
>+  compareQueryToTestData("place:transition=" +
>+                         Ci.nsINavHistoryService.TRANSITION_TYPED +
>+                         "&transition=" +
>+                         Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
>+                         testDataTyped.concat(testDataBookmark).sort(
>+                            function(a,b) { return (a - b); }));

Could you add an additional, simpler compare whose query is only "place:transition=foo"?  i.e., no multiple terms.

>+function compareQueryToTestData(queryStr, data) {
>+  var query = [];
>+  var options = {};
>+  PlacesUtils.history.queryStringToQueries(queryStr, query, {}, options);
>+  query = (query.value)[0];

Nit: I guess this works OK, but please define query as an object, not an array, since it's customary to pass objects as out params -- like you do with options.  And the parens around query.value aren't needed.

>+  options = PlacesUtils.history.getNewQueryOptions();

Just do options = options.value here.

>+  var result = PlacesUtils.history.executeQuery(query, options);
>+  var root = result.root;
>+  for (var i = 0; i < data.length; i++) {
>+    data[i] = testData[data[i]];
>+    data[i].isInQuery = true;
>+  }
>+  compareArrayToResult(data, root);

Hmm, you're modifying data[i].isInQuery but not resetting it after you compareArrayToResult().  Doesn't that have the potential to throw off later callers of compareQueryToTestData()?  Seems like you should set isInQuery to false in every object in the big array above, do your set to true here, and then set them back to false after the compare.  What do you think?

Also, please add a live update test as I mentioned in comment 11.

And as we talked about, at the end of your test you have to clean up your changes to the DB to be kind to later tests.  You probably figured it out already, but you should do:

  PlacesUtils.bhistory.removeAllPages();

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changed previous patch per adw's review. Cleaned up test cases and added another to test live update.

About setting isInQuery to false after each test:
The compareQueryToTestData method only compares the subset of testData for which isInQuery must be true for each element. We could set everything in testData to have isInQuery true by default but this might not play well if new entries are added.

Comment 15

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixes problem with previous patch (test case did not work without blocked patch).

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 445467 [details] [diff] [review]
Fixes problem with previous patch (test case did not work without blocked patch).

Mano, could you take a quick look at this?  Everything looks good to me, but there are changes to live update and history observers that I'd like to confirm are OK (GetUpdateRequirements, AddVisit, IsOptimizableHistoryQuery).

Comment 17

[Marco Bonardo [:mak]]

a couple drive-by comments

+  void getTransitions([optional] out unsigned long count,
+                      [retval,array,size_is(count)] out unsigned long transitions);

making optional the second param is probably better no?

the code is changing framed_link and embed transitions but the test is only testing typed, download, and bookmark

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> +  void getTransitions([optional] out unsigned long count,
> +                      [retval,array,size_is(count)] out unsigned long
> transitions);
> 
> making optional the second param is probably better no?
The second param is the return value, so you can't make it optional.  This syntax means you can do this in JS:
var array = getTransitions();

Comment 19

[Marco Bonardo [:mak]]

Cool, I thought the first out param was used as a return value in js

Comment 20

[Marco Bonardo [:mak]]

Hm, I was thinking if we should make these OR or AND conditions... if we pass multiple queries they are ORed already, thus if we make this OR, there is no way to query for pages that have 2 transition types (like "give me pages that have some visit with transition typed and some with transition bookmark). This is not a big deal with bookmark folders because the same exact bookmark cannot be in 2 different folders (While they can have the same uri).
What this does with OR is already achievable passing 2 transitions=XX queries. Drew, what do you think?

Comment 21

[Drew Willcoxon :adw]

That's a good point Marco.  The tags attribute works like that for that reason.  Having already r+'ed this patch, I would say the best option is an AND'ed array, the next best a scalar, and the least best an OR'ed array... :\

Comment 22

[Mehdi Mulani [:mmm] (I don't check this)]

An AND'ed array makes the most sense to me. Should I implement this for the patch?

Comment 23

[Marco Bonardo [:mak]]

yes please

Comment 24

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changes transition searching to match all transitions provided rather than any.

This does not test live updating for hidden types and as such, leaves bug 325241 unresolved as per discussions with mak and sdwilsh.

Comment 25

[Drew Willcoxon :adw]

Comment on attachment 446800 [details] [diff] [review]
Changes transition searching to match all transitions provided rather than any.

Thanks for making this change.  Two small nits:

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  // transitions
>+  const nsTArray<PRUint32>& transitions = aQuery->Transitions();
>+  if (transitions.Length() == 1) {
>+    clause.Condition("v.visit_type =").Param(":transition0_");
>+  } else if (transitions.Length() > 1) {

  {
  else if (transitions.Length() > 1) {

Also, it might be worth a comment explaining that the first branch is just an optimization of the second.

Please ask Marco for review on this patch or, if you make a new one with these nits addressed, on that one.  Marco, see comment 16.

(Just for posterity, Marco and Mehdi and I discussed making the SQL in QueryToSelectClause similar to the tags case.  Marco was concerned that the HAVING would make the query slower than what's in this patch.  But I noted that we don't have a moz_historyvisits index on place_id and visit_type.  Without data it's hard to say.  Marco recommended that we land this and if we notice perf problems we can optimize it later.)

Comment 26

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed nits suggested by adw.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 446840 [details] [diff] [review]
Fixed nits suggested by adw.

API change looks fine, but I don't understand the IDL.  The attribute shouldn't be buried in between the two methods, and I'm not sure what the comment is referring to -- when what is not empty?

Comment 28

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #27)
> (From update of attachment 446840 [details] [diff] [review])
> API change looks fine, but I don't understand the IDL.  The attribute shouldn't
> be buried in between the two methods, and I'm not sure what the comment is
> referring to -- when what is not empty?

The comment is referring to when the transitions property of the query is not empty. Would changing to the text below be good?
> When the set of transitions is nonempty, results are limited to items that
> are marked with all of the transitions. For searching on more than one
> transition this can be very slow.

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, that's fine -- but please also add comments to say, for example, "Set the transitions to limit this query to", "Get the count of set query transitions", "Get the set query transitions" above the methods and the attribute.  I'd also put the "set" method first -- since that's the main thing that will get used.  The getters/count piece are helpful, but they aren't useful unless something has been set first.

Comment 30

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Added verbose comments in nsINavHistoryService.idl.

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 447436 [details] [diff] [review]
Added verbose comments in nsINavHistoryService.idl.


>-[scriptable, uuid(6f5668f0-da8e-4069-a0de-6680e5cd8570)]
>+[scriptable, uuid(dc87ae79-22f1-4dcf-975b-852b01d210cb)]
>   /**
>+   * When the set of transitions is nonempty, results are limited to items that
>+   * are marked with all of the transitions. For searching on more than one
>+   * transition this can be very slow.

"marked" is too generic and items is conflicting in our code (we use item to indicate something that is bookmarked :().
I'd say "...results are limited to pages with at least one visit for each one of the specified transition types." (feel free to make this better English in case)
The latter phrase should be a @note instead as "Searching for more than one transition type could be slow.".


>+   *
>+   * Set the transitions that this query will be limited to.

"Limit results to the specified list of transition types."


>+  void setTransitions([const,array, size_is(count)] in unsigned long transitions,
>+  /**
>+   * Get the transitions set for this query.

empty line after method definition please


>+   */
>+  void getTransitions([optional] out unsigned long count,
>+                      [retval,array,size_is(count)] out unsigned long transitions);
>+  /**
>+   * Get the count of the set query transitions.

ditto


>+   */
>+  readonly attribute unsigned long transitionCount;
>+                      in unsigned long count);

hm?? This looks broken!


>+  // transitions
>+  const nsTArray<PRUint32>& transitions = aQuery->Transitions();
>+  /**
>+   * We hard code an optimization for transition arrays of size one below
>+   * as this is the main use case.
>+   */

make this a one line comment:
"// Optimize single transition query, since this is the most common use case."


>+  if (transitions.Length() == 1) {
>+    clause.Condition("v.visit_type =").Param(":transition0_");
>+  }
>+  else if (transitions.Length() > 1) {
>+    for (PRUint32 i = 0; i < transitions.Length(); ++i) {
>+      nsPrintfCString param(":transition%d_", i);
>+      clause.Str("EXISTS (SELECT 1 FROM moz_historyvisits WHERE "
>+        "place_id = h.id AND visit_type = "

reindent please:

clause.Str("EXISTS (SELECT 1 FROM moz_historyvisits "
                   "WHERE place_id = h.id AND visit_type = "
          ).Param(param.get()).Str(" LIMIT 1)");


>diff --git a/toolkit/components/places/src/nsNavHistoryQuery.cpp b/toolkit/components/places/src/nsNavHistoryQuery.cpp

>+NS_IMETHODIMP nsNavHistoryQuery::GetTransitionCount(PRUint32* aCount)
>+{
>+  *aCount = mFolders.Length();

mFolders?


Ok, this is fine, but there is still one thing you should do, otherwise you will kill our views performances.
Since you have changed the way we notify onVisit (now we also notify embed visits), you have to check that our observers (http://mxr.mozilla.org/mozilla-central/search?string=onVisit() won't handle those notifications unless they really need to.
Most likely the bookmarks observer is not interested in _EMBED, _FRAMED_LINK or _DOWNLOAD and the results observers are only interested to handle the notification if the query that generated them had transitions set.
If you don't do that our history views will update also for visits that are not even visible.

Comment 32

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #31)
> Ok, this is fine, but there is still one thing you should do, otherwise you
> will kill our views performances.
> Since you have changed the way we notify onVisit (now we also notify embed
> visits), you have to check that our observers
> (http://mxr.mozilla.org/mozilla-central/search?string=onVisit() won't handle
> those notifications unless they really need to.
> Most likely the bookmarks observer is not interested in _EMBED, _FRAMED_LINK or
> _DOWNLOAD and the results observers are only interested to handle the
> notification if the query that generated them had transitions set.
> If you don't do that our history views will update also for visits that are not
> even visible.

With this patch, the logic to determine if a visit is hidden is unchanged. See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#2781 and lines 2816-18. After removing the check for TRANSITION_DOWNLOAD on line 2864, the transition checks on lines 2862-63 are duplicated by the earlier checks and were removed as well.

If I remember correctly, we discussed this earlier and found that having the hidden property of the visit passed down to observer and then notifier was too much work to add to this bug and should instead be done in a fix for bug 325241.

Comment 33

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed copy&paste fail and other bits pointed out by Marco.

This patch does not change anything from the last patch about how observers are notified. Holding off on changing that until Marco provides some feedback about comment 32.

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 448049 [details] [diff] [review]
Fixed copy&paste fail and other bits pointed out by Marco.

interface changes look fine to me, thanks for making the comment fixes!

Comment 35

[Marco Bonardo [:mak]]

I'm talking about this change:

-  if (!hidden && aTransitionType != TRANSITION_EMBED &&
-                 aTransitionType != TRANSITION_FRAMED_LINK &&
-                 aTransitionType != TRANSITION_DOWNLOAD) {
+  if (!hidden) {
     NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                      nsINavHistoryObserver,
                      OnVisit(aURI, *aVisitID, aTime, aSessionID,
                              referringVisitID, aTransitionType, &added));

clearly this could make so we fire far more notifications, for example looks like history sidebar now would refresh adding a download, or an embed visit to a non hidden page.

Comment 36

[Marco Bonardo [:mak]]

the problem we moved to another bug was removing the !hidden check.

Comment 37

[Marco Bonardo [:mak]]

Mehdi pointed me to previous code piece that makes this code just redundant, I'll review the patch as it is asap.

Comment 38

[Shawn Wilsher :sdwilsh]

(In reply to comment #37)
> Mehdi pointed me to previous code piece that makes this code just redundant,
> I'll review the patch as it is asap.
Thanks Marco!

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 448049 [details] [diff] [review]
Fixed copy&paste fail and other bits pointed out by Marco.

hte patch looks fine, sorry for the misunderstoodment on the notifications change.

Comment 40

[Justin Wood (:Callek)]

This failed both Xo and Xd on try, mmm said he would look into it. Dropping checkin-needed for now.

Comment 41

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed problem found by MozillaTry.

Found out that the previous patch only search moz_historyvisits which would intermittently fail a test on MozillaTry for linux. It would also fail both tests reliably on windows.

Changed the code so that moz_historyvisits and moz_historyvisits_temp are checked.

Could you do a tiny glance over the changed bit, Marco?

Comment 42

[Marco Bonardo [:mak]]

Comment on attachment 451981 [details] [diff] [review]
Fixed problem found by MozillaTry.

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  else if (transitions.Length() > 1) {
>+    for (PRUint32 i = 0; i < transitions.Length(); ++i) {
>+      nsPrintfCString param(":transition%d_", i);
>+      clause.Str("(EXISTS (SELECT 1 FROM moz_historyvisits "
>+                         "WHERE place_id = h.id AND visit_type = "
>+                ).Param(param.get()).Str(" LIMIT 1)");
>+      clause.Str("OR EXISTS (SELECT 1 FROM moz_historyvisits_temp "
>+                            "WHERE place_id = h.id AND visit_type = "
>+                ).Param(param.get()).Str(" LIMIT 1))");

you can use a single EXISTS(x UNION ALL y LIMIT 1) here, could be a bit faster

Comment 43

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Changed SQL query for multiple transitions and added export information.

Comment 44

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/bb28be604c95