Closed Bug 1446951 Opened 6 years ago Closed 6 years ago

Stop supporting multiple OR-ed queries in Places

Categories

(Toolkit :: Places, enhancement, P1)

Product:
Toolkit
Component:
Places
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(9 files)

Bug 1446951 - 1 - Remove nsINavHistory::ExecuteQueries.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 2 - Change nsINavHistoryQueryResultNode::GetQueries to a query attribute.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 3 - Make AsyncExecuteLegacyQueries singular.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 4 - Make QueriesToQueryString and QueryStringToQueries singular.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 5 - Remove QueryStringToQueryArray.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 6 - Change internal results code to support a single query.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 7 - Remove some internal use of GetHas* APIs.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 8 - Reduce the number of query node constructors to one.
59 bytes, text/x-review-board-request
standard8
: review+
Details
Bug 1446951 - 9 - Move EvaluateQueryForNode.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
This just complicates a bunch of code for no benefit since we never used them in Core, and legacy add-ons are gone.
Comment on attachment 8960623 [details]
Bug 1446951 - 7 - Remove some internal use of GetHas* APIs.

https://reviewboard.mozilla.org/r/229388/#review235528
Attachment #8960623 - Flags: review?(standard8) → review+
Comment on attachment 8960617 [details]
Bug 1446951 - 1 - Remove nsINavHistory::ExecuteQueries.

https://reviewboard.mozilla.org/r/229376/#review235544
Attachment #8960617 - Flags: review?(standard8) → review+
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?
Attachment #8960618 - Flags: review?(standard8) → review+
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)
Attachment #8960619 - Flags: review?(standard8) → review+
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
Attachment #8960620 - Flags: review?(standard8) → review+
Comment on attachment 8960621 [details]
Bug 1446951 - 5 - Remove QueryStringToQueryArray.

https://reviewboard.mozilla.org/r/229384/#review235548
Attachment #8960621 - Flags: review?(standard8) → review+
Comment on attachment 8960622 [details]
Bug 1446951 - 6 - Change internal results code to support a single query.

https://reviewboard.mozilla.org/r/229386/#review235552
Attachment #8960622 - Flags: review?(standard8) → review+
Comment on attachment 8960625 [details]
Bug 1446951 - 9 - Move EvaluateQueryForNode.

https://reviewboard.mozilla.org/r/229392/#review235556
Attachment #8960625 - Flags: review?(standard8) → review+
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).
Attachment #8960624 - Flags: review?(standard8) → review+
(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.
Pushed by mak77@bonardo.net:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: