Last Comment Bug 702639 - Kill excludeItemsIfParentHasAnnotation query option
: Kill excludeItemsIfParentHasAnnotation query option
: addon-compat, dev-doc-complete, perf
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla13
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
Depends on: livemarksIO 736541 793523
Blocks: 386396 placesFolders 743677
  Show dependency treegraph
Reported: 2011-11-15 08:18 PST by Marco Bonardo [::mak]
Modified: 2012-09-23 14:34 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1.0 (27.03 KB, patch)
2011-11-30 13:12 PST, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
patch v1.1 (20.25 KB, patch)
2012-02-10 02:37 PST, Marco Bonardo [::mak] superreview+
Details | Diff | Splinter Review

Description User image Marco Bonardo [::mak] 2011-11-15 08:18:14 PST
This existed mostly for livemarks, it's complex and should just die for now.
Comment 1 User image Marco Bonardo [::mak] 2011-11-30 09:49:19 PST
I think I may be able to remove another rock from FilterResultSet, and that would be another step towards async queries, let's see what I'll end up with.
Comment 2 User image Marco Bonardo [::mak] 2011-11-30 13:12:37 PST
Created attachment 578053 [details] [diff] [review]
patch v1.0

So, the last vestiges of FilterResultSet() exist just to remove duplicate uris from tags and check if a node can be added to a query with search terms.
The former will be fixable changing tags schema, the latter is not a bug, once the former is fixed FilterResultSet can be merged into EvaluateQueryForNode (or become an helper like EvaluateSearchTermsForNode).
I wonder if I could subquery tags, so that the external query removes duplicates and completely kill FilterResultSet() now, would be fancy.
Comment 3 User image Marco Bonardo [::mak] 2011-11-30 13:14:29 PST
Comment on attachment 578053 [details] [diff] [review]
patch v1.0

note that this depends on the livemarks changes, so it can't land before.
Comment 4 User image Marco Bonardo [::mak] 2011-11-30 16:42:11 PST
Comment on attachment 578053 [details] [diff] [review]
patch v1.0

Review of attachment 578053 [details] [diff] [review]:

::: toolkit/components/places/nsNavHistory.cpp
@@ +4137,5 @@
>            .Str(", h.url, page_title, tags, ")
>            .Str(nsPrintfCString(17, "0, 0, 0, 0, %d, 0)",
>                                 mozIPlacesAutoComplete::MATCH_ANYWHERE_UNMODIFIED).get());
> +    // Serching by terms implicitly exclude queries.
> +    clause.Condition("NOT url BETWEEN 'place:' AND 'place;'");

self-comment: should be h.url
Comment 5 User image Dietrich Ayala (:dietrich) 2011-12-06 22:05:04 PST
Comment on attachment 578053 [details] [diff] [review]
patch v1.0

Review of attachment 578053 [details] [diff] [review]:

looks ok, r=me
Comment 6 User image Marco Bonardo [::mak] 2012-02-10 02:37:25 PST
Created attachment 596000 [details] [diff] [review]
patch v1.1

fixes a subtle bug with excludeQueries that was failing a test on Try.
Comment 7 User image Marco Bonardo [::mak] 2012-02-24 02:54:58 PST
Comment on attachment 596000 [details] [diff] [review]
patch v1.1

Review of attachment 596000 [details] [diff] [review]:

oops, I forgot I still need a SR here :)

This options was useful only to filter out livemark children from queries, now that load-on-demand livemarks exist it is no more useful.
The reason we can't just deprecate it is that its removal allows to speed up quite some queries, on the other side the places query strings just ignore unknown options, and this will just become one of those, so compatibility issues should be limited.
Comment 9 User image Marco Bonardo [::mak] 2012-02-24 14:46:23 PST
I don't expect any interesting breakage from this, btw adding the keyword.
Comment 10 User image Marco Bonardo [::mak] 2012-02-24 14:46:44 PST
and documentation may have some pointer to this
Comment 11 User image Marco Bonardo [::mak] 2012-02-25 02:24:01 PST

Note You need to log in before you can comment on or make changes to this bug.