Closed Bug 1269398 Opened 4 years ago Closed 4 years ago

Update HistoryService.executeQuery method to return extra fields needed for browser.history API

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bsilverberg, Assigned: kmag)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

The current plan is to use `executeQuery` to provide data for both `history.search` [1] and `history.getVisits` [2], but the following additional data needs to be returned by `executeQuery` to support this:

For `history.search`, which returns one record per URL, we need:
- typedCount, which is the number of times the user visited this url by typing. This would be the equivalent of our `TRANSITION_TYPED` transition type. 

For `history.getVisits`, which returns one record per visit, we need:
- visitId, which is a unique id for this visit. We could simply provide the database pk, or could generate this in some other manner.
- referringVisitId, which is the unique id of the referring visit.
- transition, which is the transition type for the visit. Our API code will translate between Places transition types and the transition types expected by the API.

There has been some discussion of this over email, and Marco provided the following tips for implementing these changes:

> I found this old changeset that was adding guids to the same
> queries and should help figuring out which parts of the code need to be
> touched:
> http://hg.mozilla.org/mozilla-central/rev/a4fc8aae3053
> Basically, the queries must query 2 new fields, for most queries those will
> just be null (trivial change), the only one that will really select the
> values is the one for RESULTS_AS_VISITS
> Plus nsNavHistoryResultNode API will need some change.

[1] https://developer.chrome.com/extensions/history#method-search
[2] https://developer.chrome.com/extensions/history#method-getVisits
Blocks: 1265834, 1265835
Priority: -- → P2
(In reply to Bob Silverberg [:bsilverberg] from comment #0)
> For `history.search`, which returns one record per URL, we need:
> - typedCount, which is the number of times the user visited this url by
> typing. This would be the equivalent of our `TRANSITION_TYPED` transition
> type. 

As previously discussed, we don't track this information as a counter, and fetching the number of typed visits in a large list could be very expensive.  Additionally we mark as typed visits that were not typed (clicks on the history menu for example).

I'd suggest to start with the GetVisits work, and leave typedCount apart for now.
(In reply to Marco Bonardo [::mak] from comment #1)
> (In reply to Bob Silverberg [:bsilverberg] from comment #0)
> > For `history.search`, which returns one record per URL, we need:
> > - typedCount, which is the number of times the user visited this url by
> > typing. This would be the equivalent of our `TRANSITION_TYPED` transition
> > type. 
> 
> As previously discussed, we don't track this information as a counter, and
> fetching the number of typed visits in a large list could be very expensive.
> Additionally we mark as typed visits that were not typed (clicks on the
> history menu for example).
> 
> I'd suggest to start with the GetVisits work, and leave typedCount apart for
> now.

Thanks Marco. I thought we mightn't have a final solution for that yet, but didn't want to lose the requirement. I think we can probably implement `search` without `typedCount` for now, so yes, let's just move forward in this bug with the changes for `getVisits`.
the best path forward for typedCount, may be to change the "typed" column of moz_places from being a bool to being a counter. Then we could just select it in the queries. But it's something that needs separate handling.
Kris, this is the bug we discussed in today's meeting that needs native work. Thanks for offering to help. :)
Flags: needinfo?(kmaglione+bmo)
No longer blocks: 1265834
Blocks: 1271801
Flags: needinfo?(kmaglione+bmo)
No longer blocks: 1271801
Depends on: 1271801
Assignee: nobody → kmaglione+bmo
Comment on attachment 8752408 [details]
MozReview Request: Bug 1269398: Add visitType, visitId, and fromVisitId fields to history visit query results. r?mak

https://reviewboard.mozilla.org/r/52599/#review50896

It's pretty good, just some details to fix

::: toolkit/components/places/nsNavHistory.cpp:253
(Diff revision 1)
>  const int32_t nsNavHistory::kGetInfoIndex_Frecency = 12;
>  const int32_t nsNavHistory::kGetInfoIndex_Hidden = 13;
>  const int32_t nsNavHistory::kGetInfoIndex_Guid = 14;
> +const int32_t nsNavHistory::kGetInfoIndex_VisitId = 15;
> +const int32_t nsNavHistory::kGetInfoIndex_FromVisitId = 16;
> +const int32_t nsNavHistory::kGetInfoIndex_VisitType = 17;

these indices are connected to the indices at 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#25 that are basically a continuation of these.
So you'll have to update those as well, and update the queries in  nsNavBookmarks::GetDescendantChildren and nsNavBookmarks::QueryFolderChildren and nsNavBookmarks::QueryFolderChildrenAsync

::: toolkit/components/places/nsNavHistory.cpp:1574
(Diff revision 1)
>                       mHasSearchTerms,
>                       tagsSqlFragment);
>    mQueryString = NS_LITERAL_CSTRING(
>      "SELECT h.id, h.url, h.title AS page_title, h.rev_host, h.visit_count, "
>        "v.visit_date, f.url, null, null, null, null, ") +
> -      tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid "
> +      tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid, "

PlacesSQLQueryBuilder::SelectAsURI should also be updated.
The system works considering all the queries return the same columns

::: toolkit/components/places/nsNavHistory.cpp:4078
(Diff revision 1)
>        // Should match kGetInfoIndex_* (see GetQueryResults)
>        statement = mDB->GetStatement(NS_LITERAL_CSTRING(
>          "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
>                 "v.visit_date, f.url, null, null, null, null, "
> -               ) + tagsFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid "
> +               ) + tagsFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid, "
> +              "null, null, null "

These should be real values, not nulls

::: toolkit/components/places/tests/unit/test_visit_details.js:5
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

if it's ok with you, please don't provide any header, so this just fallbacks to a PD license.

::: toolkit/components/places/tests/unit/test_visit_details.js:44
(Diff revision 1)
> +  child = root.getChild(2);
> +  equal(child.visitType, history.TRANSITION_TYPED, "Visit type should be TRANSITION_TYPED");
> +  equal(child.visitId, 3, "Visit ID should be 3");
> +  equal(child.fromVisitId, -1, "Should have no referrer visit ID");
> +
> +  root.containerOpen = false;

Please also check the default values for a RESULTS_AS_URI and for a QUERY_TYPE_BOOKMARKS query.

::: toolkit/components/places/tests/unit/xpcshell.ini:153
(Diff revision 1)
>  # Bug 676989: test hangs consistently on Android
>  skip-if = os == "android"
>  [test_utils_backups_create.js]
>  [test_utils_getURLsForContainerNode.js]
>  [test_utils_setAnnotationsFor.js]
> +[test_visit_details.js]

let's name this test_resultsAsVisit_details.js
Attachment #8752408 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/52599/#review50896

> these indices are connected to the indices at 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavBookmarks.cpp#25 that are basically a continuation of these.
> So you'll have to update those as well, and update the queries in  nsNavBookmarks::GetDescendantChildren and nsNavBookmarks::QueryFolderChildren and nsNavBookmarks::QueryFolderChildrenAsync

Ah. I was wondering why that one query had an extra four columns. I guess I'll add comment to the end of the kGetInfoIndex constants for other people reading this code.

> PlacesSQLQueryBuilder::SelectAsURI should also be updated.
> The system works considering all the queries return the same columns

Yeah, I skipped that because it selected extra columns in place of these. Now I understand what was going on.

> These should be real values, not nulls

Oops. Good catch.

> if it's ok with you, please don't provide any header, so this just fallbacks to a PD license.

Fine with me. I just copied this from one of the other tests in the directory.
Comment on attachment 8752408 [details]
MozReview Request: Bug 1269398: Add visitType, visitId, and fromVisitId fields to history visit query results. r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52599/diff/1-2/
Attachment #8752408 - Flags: review?(mak77)
Attachment #8752408 - Flags: review?(mak77) → review+
Comment on attachment 8752408 [details]
MozReview Request: Bug 1269398: Add visitType, visitId, and fromVisitId fields to history visit query results. r?mak

https://reviewboard.mozilla.org/r/52599/#review51764

r=me, with a green Try run.

::: toolkit/components/places/nsNavHistory.cpp:260
(Diff revision 2)
> +// which must be kept in sync:
> +// nsNavBookmarks::kGetChildrenIndex_Guid = 18;
> +// nsNavBookmarks::kGetChildrenIndex_Position = 19;
> +// nsNavBookmarks::kGetChildrenIndex_Type = 20;
> +// nsNavBookmarks::kGetChildrenIndex_PlaceID = 21;
>  

I like your comment more than the one we have at line 234 (just before these consts), so please remove that old one

::: toolkit/components/places/tests/unit/test_resultsAsVisit_details.js:12
(Diff revision 2)
> +  let uri = NetUtil.newURI("http://test4.com/");
> +  yield PlacesTestUtils.addVisits([
> +    { uri },
> +    { uri, referrer: uri },
> +    { uri, transition: history.TRANSITION_TYPED },
> +  ]);

move visits addition into a test_setup (from there you can use do_register_cleanup to invoke clearHistory) and split the test into a test_results_as_visits and test_results_as_uri

::: toolkit/components/places/tests/unit/test_resultsAsVisit_details.js:21
(Diff revision 2)
> +  let query = history.getNewQuery();
> +
> +  query.uri = uri;
> +
> +
> +  // Check RESULTS_AS_VISIT node.

you could probably remove most of the newlines above and compact the code a little bit.

::: toolkit/components/places/tests/unit/test_resultsAsVisit_details.js:47
(Diff revision 2)
> +  equal(child.fromVisitId, -1, "Should have no referrer visit ID");
> +
> +  root.containerOpen = false;
> +
> +
> +  // Check RESULTS_AS_URI node.

double newline

::: toolkit/components/places/tests/unit/test_resultsAsVisit_details.js:68
(Diff revision 2)
> +});
> +
> +add_task(function* test_bookmarkFields() {
> +  let folder = bookmarks.createFolder(bookmarks.placesRoot, "test folder", bookmarks.DEFAULT_INDEX);
> +  let bm = bookmarks.insertBookmark(folder, uri("http://test4.com/"),
> +                                    bookmarks.DEFAULT_INDEX, "test4 title");

please use the new async API (PlacesUtils.bookmarks.insert()) instead of the old synchronous API.
https://hg.mozilla.org/integration/fx-team/rev/def937e0dda7f3e9409e3f5bd70a659c4232782f
Bug 1269398: Add visitType, visitId, and fromVisitId fields to history visit query results. r=mak
https://hg.mozilla.org/mozilla-central/rev/def937e0dda7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Thanks for this, Kris. This will enable me to implement `history.getVisits`. A fully implemented version of `history.search` is still waiting on bug 1271801.
Yep, I decided to put that off until after this had landed, since this seemed more important, and that's going to require a migration.
You need to log in before you can comment on or make changes to this bug.