1446951 - Stop supporting multiple OR-ed queries in Places

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

This just complicates a bunch of code for no benefit since we never used them in Core, and legacy add-ons are gone.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 1 - Remove nsINavHistory::ExecuteQueries.

Review commit: https://reviewboard.mozilla.org/r/229376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229376/

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 2 - Change nsINavHistoryQueryResultNode::GetQueries to a query attribute.

Review commit: https://reviewboard.mozilla.org/r/229378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229378/

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 3 - Make AsyncExecuteLegacyQueries singular.

Review commit: https://reviewboard.mozilla.org/r/229380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229380/

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 4 - Make QueriesToQueryString and QueryStringToQueries singular.

Review commit: https://reviewboard.mozilla.org/r/229382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229382/

Comment 5

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 5 - Remove QueryStringToQueryArray.

Review commit: https://reviewboard.mozilla.org/r/229384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229384/

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 6 - Change internal results code to support a single query.

Review commit: https://reviewboard.mozilla.org/r/229386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229386/

Comment 7

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 7 - Remove some internal use of GetHas* APIs.

Review commit: https://reviewboard.mozilla.org/r/229388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229388/

Comment 8

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 8 - Reduce the number of query node constructors to one.

Review commit: https://reviewboard.mozilla.org/r/229390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229390/

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 1446951 - 9 - Move EvaluateQueryForNode.

Review commit: https://reviewboard.mozilla.org/r/229392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229392/

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8960623 [details]
Bug 1446951 - 7 - Remove some internal use of GetHas* APIs.

https://reviewboard.mozilla.org/r/229388/#review235528

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8960617 [details]
Bug 1446951 - 1 - Remove nsINavHistory::ExecuteQueries.

https://reviewboard.mozilla.org/r/229376/#review235544

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8960618 [details]
Bug 1446951 - 2 - Change nsINavHistoryQueryResultNode::GetQueries to a query attribute.

https://reviewboard.mozilla.org/r/229378/#review235472

::: browser/components/places/content/controller.js:229
(Diff revision 1)
>        break;
>      case "placesCmd_deleteDataHost":
> -      var host;
> +      let host;
>        if (PlacesUtils.nodeIsHost(this._view.selectedNode)) {
> -        var queries = this._view.selectedNode.getQueries();
> -        host = queries[0].domain;
> +        let query = this._view.selectedNode.query;
> +        host = query.domain;

nit: could we drop the intermediate variable and do it all on one line now?

::: toolkit/components/places/PlacesUtils.jsm:802
(Diff revision 1)
>        return asQuery(aNode).folderItemId;
>      else if (PlacesUtils.nodeIsTagQuery(aNode)) {
>        // RESULTS_AS_TAG_CONTENTS queries are similar to folder shortcuts
>        // so we can still get the concrete itemId for them.
> -      var queries = aNode.getQueries();
> -      var folders = queries[0].getFolders();
> +      let query = aNode.query;
> +      let folders = query.getFolders();

nit: could probably drop the query intermediate.

::: toolkit/components/places/nsINavHistoryService.idl:331
(Diff revision 1)
>   */
>  [scriptable, uuid(62817759-4FEE-44A3-B58C-3E2F5AFC9D0A)]
>  interface nsINavHistoryQueryResultNode : nsINavHistoryContainerResultNode
>  {
>    /**
>     * Get the queries which build this node's children.

nit: Update the comment.

::: toolkit/components/places/nsNavHistoryResult.cpp:3095
(Diff revision 1)
>      aURI = mURI;
>      return NS_OK;
>    }
>  
> -  uint32_t queryCount;
> -  nsINavHistoryQuery** queries;
> +  nsCOMPtr<nsINavHistoryQuery> query;
> +  nsresult rv = GetQuery(getter_AddRefs<nsINavHistoryQuery>(query));

Did you mean for the template parameter on getter_AddRefs? We don't use it elsewhere...

::: toolkit/components/places/tests/unit/test_485442_crash_bug_nsNavHistoryQuery_GetUri.js:13
(Diff revision 1)
> -  var query = hs.getNewQuery();
> -  var options = hs.getNewQueryOptions();
> +  let query = PlacesUtils.history.getNewQuery();
> +  let options = PlacesUtils.history.getNewQueryOptions();
>    options.resultType = options.RESULT_TYPE_QUERY;
> -  var result = hs.executeQuery(query, options);
> -  result.root.containerOpen = true;
> -  var rootNode = result.root;
> +  let root = PlacesUtils.history.executeQuery(query, options).root;
> +  root.containerOpen = true;
> +  Assert.equal(PlacesUtils.asQuery(root).query.uri, null); // Should be null, instead of crashing the browser

As we're here, maybe make the comment into a string as the third parameter?

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8960619 [details]
Bug 1446951 - 3 - Make AsyncExecuteLegacyQueries singular.

https://reviewboard.mozilla.org/r/229380/#review235478

::: toolkit/components/places/nsPIPlacesDatabase.idl:41
(Diff revision 1)
>     *       executeQueries, because there is additional filtering and sorting
>     *       done by the latter.  Thus you should use executeQueries, unless you
>     *       are absolutely sure that the returned results are fine for
>     *       your use-case.
>     */
> -  mozIStoragePendingStatement asyncExecuteLegacyQueries(
> +  mozIStoragePendingStatement asyncExecuteLegacyQuery(

Nit: Docs need update (executeQueries -> executeQuery)

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8960620 [details]
Bug 1446951 - 4 - Make QueriesToQueryString and QueryStringToQueries singular.

https://reviewboard.mozilla.org/r/229382/#review235480

::: toolkit/components/places/tests/bookmarks/test_405938_restore_queries.js:112
(Diff revision 1)
> -      title: this._queryTitle3
> -    });
> -
>      // Create a query URI for most recent bookmarks with NO folders specified.
> -    this._queryURI4 = "place:queryType=1&sort=12&excludeItemIfParentHasAnnotation=livemark%2FfeedURI&maxResults=10&excludeQueries=1";
> +    this._queryURI3 = "place:queryType=1&sort=12&maxResults=10&excludeQueries=1";
>      this._queryTitle4 = "query4";

nit: maybe update the \_queryTitle number as well to match? Or not change the number, so this would still match validateQueryNode4

::: toolkit/components/places/tests/queries/test_querySerialization.js:711
(Diff revision 1)
>   * nsINavHistoryQueryOptions object aQueryOptions, de-serializes the
>   * serialization, and ensures (using do_check_* functions) that the
>   * de-serialized objects equal the originals.
>   *
> - * @param aQueryArr
> - *        an array containing nsINavHistoryQuery objects
> + * @param aQuery
> + *        an nsINavHistoryQuery objects

nit: objects -> object

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8960621 [details]
Bug 1446951 - 5 - Remove QueryStringToQueryArray.

https://reviewboard.mozilla.org/r/229384/#review235548

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8960622 [details]
Bug 1446951 - 6 - Change internal results code to support a single query.

https://reviewboard.mozilla.org/r/229386/#review235552

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8960625 [details]
Bug 1446951 - 9 - Move EvaluateQueryForNode.

https://reviewboard.mozilla.org/r/229392/#review235556

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8960624 [details]
Bug 1446951 - 8 - Reduce the number of query node constructors to one.

https://reviewboard.mozilla.org/r/229390/#review235562

r=Standard8 assuming I'm right in understanding that it is now safe to remove the Verify* methods because we're doing the work before we call the constructors (and passing the information into the constructors).

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8960618 [details]
Bug 1446951 - 2 - Change nsINavHistoryQueryResultNode::GetQueries to a query attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229378/diff/1-2/

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8960619 [details]
Bug 1446951 - 3 - Make AsyncExecuteLegacyQueries singular.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229380/diff/1-2/

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8960620 [details]
Bug 1446951 - 4 - Make QueriesToQueryString and QueryStringToQueries singular.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229382/diff/1-2/

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8960621 [details]
Bug 1446951 - 5 - Remove QueryStringToQueryArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229384/diff/1-2/

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8960622 [details]
Bug 1446951 - 6 - Change internal results code to support a single query.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229386/diff/1-2/

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8960623 [details]
Bug 1446951 - 7 - Remove some internal use of GetHas* APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229388/diff/1-2/

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8960624 [details]
Bug 1446951 - 8 - Reduce the number of query node constructors to one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229390/diff/1-2/

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8960625 [details]
Bug 1446951 - 9 - Move EvaluateQueryForNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229392/diff/1-2/

Comment 27

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #18)
> r=Standard8 assuming I'm right in understanding that it is now safe to
> remove the Verify* methods because we're doing the work before we call the
> constructors (and passing the information into the constructors).

That's correct, we pass all the info to the constructor, there's no reason to build it lazily later.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fd763443412
1 - Remove nsINavHistory::ExecuteQueries. r=standard8
https://hg.mozilla.org/integration/autoland/rev/528c24b35b1a
2 - Change nsINavHistoryQueryResultNode::GetQueries to a query attribute. r=standard8
https://hg.mozilla.org/integration/autoland/rev/85d301fe0bb0
3 - Make AsyncExecuteLegacyQueries singular. r=standard8
https://hg.mozilla.org/integration/autoland/rev/cae9cbaae29e
4 - Make QueriesToQueryString and QueryStringToQueries singular. r=standard8
https://hg.mozilla.org/integration/autoland/rev/ea8cf03dc221
5 - Remove QueryStringToQueryArray. r=standard8
https://hg.mozilla.org/integration/autoland/rev/53282ceffd41
6 - Change internal results code to support a single query. r=standard8
https://hg.mozilla.org/integration/autoland/rev/7a3803b96fd1
7 - Remove some internal use of GetHas* APIs. r=standard8
https://hg.mozilla.org/integration/autoland/rev/2c4a64ef1e70
8 - Reduce the number of query node constructors to one. r=standard8
https://hg.mozilla.org/integration/autoland/rev/d509647cc1d7
9 - Move EvaluateQueryForNode. r=standard8

Comment 29

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0fd763443412
https://hg.mozilla.org/mozilla-central/rev/528c24b35b1a
https://hg.mozilla.org/mozilla-central/rev/85d301fe0bb0
https://hg.mozilla.org/mozilla-central/rev/cae9cbaae29e
https://hg.mozilla.org/mozilla-central/rev/ea8cf03dc221
https://hg.mozilla.org/mozilla-central/rev/53282ceffd41
https://hg.mozilla.org/mozilla-central/rev/7a3803b96fd1
https://hg.mozilla.org/mozilla-central/rev/2c4a64ef1e70
https://hg.mozilla.org/mozilla-central/rev/d509647cc1d7