1271801 - Change the moz_places.typed column from a boolean to a visit count

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1269398 +++

We need to track the number of times that a URL was typed, rather than just whether it was ever typed, in order to meet the contract of the Chrome extension history API.

Comment 1

[Kris Maglione [:kmag]]

Bob, I don't mind handling the implementation side of this, but I don't really have the time to write the proper tests right now. Would you mind handling that part, if I handle the native code changes?

Comment 2

[Bob Silverberg [:bsilverberg]]

Sure, Kris, I'd be happy to. For the record, this change is only part of what is needed for bug 1269398, correct? In addition to providing access to a typedCount for a url, we also need to provide visit data when doing a search via executeQuery.

Comment 3

[Bob Silverberg [:bsilverberg]]

This is actually required for browser.history.search, not for browser.history.getVisits. We had discussed in bug 1265834 that we could potentially postpone implementing this, but if we want 100% Chrome parity for search we do need this.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #2)
> Sure, Kris, I'd be happy to. For the record, this change is only part of
> what is needed for bug 1269398, correct? In addition to providing access to
> a typedCount for a url, we also need to provide visit data when doing a
> search via executeQuery.

Correct, but they're separate bugs. Feel free to file another bug for that part and assign it to me. I'll take care of it when I'm done with this one.

Comment 5

[Bob Silverberg [:bsilverberg]]

Thanks Kris. I see you've already created a patch for bug 1269398, which I think covers getVisits, so I don't think we need another bug. That one can cover the changes needed for getVisits and this one can cover the changes needed for search.

Comment 6

[Bob Silverberg [:bsilverberg]]

Kris, this is one of two things that are outstanding for a complete implementation of the history API. I don't think it's terribly urgent, but would be nice to get it finished off. Do you foresee having time in the near future to work on this?

Comment 7

[Thomas Wisniewski]

Attachment: Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

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

Comment 8

[Thomas Wisniewski]

I just took a shot at this, given that it was recently unassigned. This seems to be what the comments in bug 1269398 implied should be attempted, and indeed it seems to work (with this patch, the "typedCount" in a simple browser.history.onVisited-logging WebExtension is going up as I would expect). I'm not at all familiar with these parts of the codebase though, so I'm not sure what else I'm missing, or this is even truly correct, so I thought I should request a review before committing to it any further.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

https://reviewboard.mozilla.org/r/155942/#review164152

CREATE_PLACES_AFTERINSERT_TRIGGER should also be fixed, it should store a bool (1/0 actually) in moz_hosts, and not the last typed value (so it should likely just change to NEW.typed > 0)
HOSTS_PREFIX_PRIORITY_FRAGMENT is also wrongly doing typed = 1
UnifiedComplete.js also has some code doing typed = 1 on moz_places, that should now be "typed > 0"
Also Database.cpp has a typed = 1 case.

To sum up: everything comparing moz_places.typed = 1 should be changed to > 0.
moz_hosts.typed should keep being bool-like instead.

This will also need a schema migration in Database.cpp to update the existing typed values to the correct count, along with a test in toolkit/components/places/tests/migration/ (also an updated places.sqlite should be added there).
It will also need a test to check that typed is properly incremented/decremented on visits addition and removal, as well as that moz_hosts is kept in sync. The test should live in toolkit/components/places/tests/history/

::: toolkit/components/places/History.cpp:131
(Diff revision 1)
>     *        TRANSITION_ constants on nsINavHistoryService.
>     */
>    void SetTransitionType(uint32_t aTransitionType)
>    {
> -    typed = aTransitionType == nsINavHistoryService::TRANSITION_TYPED;
> +    if (transitionType != aTransitionType) {
> +      typed += (aTransitionType == nsINavHistoryService::TRANSITION_TYPED ? 1 : -1);

I'm not sure I understand the -1, why subtracting?

Comment 10

[Thomas Wisniewski]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

https://reviewboard.mozilla.org/r/155942/#review164152

> I'm not sure I understand the -1, why subtracting?

Just in case SetTransitionType is ever called to change the transitionType of a visit away from being "typed" (in which case we would have one fewer typed transitions). I wasn't sure if that could ever happen, but thought it would be better to be safe than sorry. If you'd rather I drop the clause, I'm all for it.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

https://reviewboard.mozilla.org/r/155942/#review164152

> Just in case SetTransitionType is ever called to change the transitionType of a visit away from being "typed" (in which case we would have one fewer typed transitions). I wasn't sure if that could ever happen, but thought it would be better to be safe than sorry. If you'd rather I drop the clause, I'm all for it.

You can probably just MOZ_ASSERT(transitionType == UINT32_MAX, "SetTransitionTyped should only be invoked once"); inside SetTransitionType to ensure we don't do that.

Comment 12

[Thomas Wisniewski]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

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

Comment 13

[Thomas Wisniewski]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

https://reviewboard.mozilla.org/r/155942/#review164152

> You can probably just MOZ_ASSERT(transitionType == UINT32_MAX, "SetTransitionTyped should only be invoked once"); inside SetTransitionType to ensure we don't do that.

It turns out that with your comments addressed, the triggers do the job of keeping "typed" correct, so I just left the assertion in SetTransitionType without the other changes.

Comment 14

[Marco Bonardo [:mak]]

Considered how close we are to the merge day, I think we should delay landing this to the beginning of the 57 cycle. I'll still try to review it along this week in case it needs further work.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8885092 [details]
Bug 1271801 - Change moz_places.typed column from a boolean into a visit count;

https://reviewboard.mozilla.org/r/155942/#review167736

We're closer, still some things to check.

::: toolkit/components/places/Database.cpp:2349
(Diff revision 2)
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "UPDATE moz_places SET typed = ( "
> +      "SELECT COUNT(*) FROM moz_historyvisits WHERE visit_type = 2 "
> +    ") "));

This looks wrong, won't it just set all the typed values for every place to the same value (global count of typed visits). Sounds like this is not covered by the unit test.

::: toolkit/components/places/Database.cpp:2356
(Diff revision 2)
> +
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "UPDATE moz_hosts SET typed = 1 WHERE host IN ( "
> +      "SELECT fixup_url(get_unreversed_host(rev_host)) "
> +      "FROM moz_places WHERE typed > 0 "
> +    ") "

I don't think we need to touch moz_hosts, even if typed was not a count, it was still (hopefully) correct

::: toolkit/components/places/History.cpp:666
(Diff revision 2)
>                                  mPlace.referrerVisitId, mPlace.transitionType,
>                                  mPlace.guid, mPlace.hidden,
>                                  mPlace.visitCount + 1, // Add current visit.
> -                                static_cast<uint32_t>(mPlace.typed),
> +                                mPlace.typed + (mPlace.transitionType ==
> +                                  nsINavHistoryService::TRANSITION_TYPED ?
> +                                  1 : 0), // Add 1 to typed if visit is typed.

We should probably update typed in insertVisitedURIs after the insertion, so we reach this point with a correct value.

::: toolkit/components/places/nsPlacesTriggers.h:34
(Diff revision 2)
>    "CREATE TEMP TRIGGER moz_historyvisits_afterinsert_v2_trigger " \
>    "AFTER INSERT ON moz_historyvisits FOR EACH ROW " \
>    "BEGIN " \
>      "SELECT store_last_inserted_id('moz_historyvisits', NEW.id); " \
>      "UPDATE moz_places SET " \
> +      "typed = typed + (SELECT NEW.visit_type = " TYPED_VISIT_TYPE "), "\

I may be enough to do
typed = typed + NEW.visit_type = " TYPED_VISIT_TYPE "

::: toolkit/components/places/nsPlacesTriggers.h:46
(Diff revision 2)
>  #define CREATE_HISTORYVISITS_AFTERDELETE_TRIGGER NS_LITERAL_CSTRING( \
>    "CREATE TEMP TRIGGER moz_historyvisits_afterdelete_v2_trigger " \
>    "AFTER DELETE ON moz_historyvisits FOR EACH ROW " \
>    "BEGIN " \
>      "UPDATE moz_places SET " \
> +      "typed = typed - (SELECT OLD.visit_type = " TYPED_VISIT_TYPE "), "\

ditto

::: toolkit/components/places/nsPlacesTriggers.h:111
(Diff revision 2)
>      "INSERT OR REPLACE INTO moz_hosts (id, host, frecency, typed, prefix) " \
>      "SELECT " \
>          "(SELECT id FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), " \
>          "fixup_url(get_unreversed_host(NEW.rev_host)), " \
>          "MAX(IFNULL((SELECT frecency FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), -1), NEW.frecency), " \
> -        "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), 0), NEW.typed), " \
> +        "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(NEW.rev_host))), 0), MIN(NEW.typed, 1)), " \

Would "NEW.typed > 0" do?

::: toolkit/components/places/tests/history/test_typed_counts_kept_in_sync.js:4
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests for `History.insert` as implemented in History.jsm

This comment is just a copy/paste leftover

::: toolkit/components/places/tests/history/test_typed_counts_kept_in_sync.js:30
(Diff revision 2)
> +        transition: TRANSITION_TYPED
> +      }]
> +    });
> +    Assert.ok(PlacesUtils.isValidGuid(result.guid), "guid for pageInfo object is valid");
> +    return result.guid;
> +  }

You don't need all of this boilerplate, you could just await PlacesUtils.history.insert in add_task...
But actually, I'd suggest to use PlacesTestUtils.addVisits that has an even more compact API.

::: toolkit/components/places/tests/history/test_typed_counts_kept_in_sync.js:47
(Diff revision 2)
> +    await PlacesUtils.history.removeVisitsByFilter({ beginDate: new Date(2001, 0, 0, 0, 0),
> +                                                     endDate: new Date() });
> +    await confirm_typed_visits(db, 1);
> +
> +    await PlacesUtils.history.removeVisitsByFilter({ beginDate: new Date(1998, 0, 0, 0, 0),
> +                                                     endDate: new Date() });

You should also add some non-typed visits, and check an url with no typed visits, and an url with some typed visits and some not.

::: toolkit/components/places/tests/history/test_typed_counts_kept_in_sync.js:50
(Diff revision 2)
> +
> +    await PlacesUtils.history.removeVisitsByFilter({ beginDate: new Date(1998, 0, 0, 0, 0),
> +                                                     endDate: new Date() });
> +    await confirm_typed_visits(db, 0);
> +  } finally {
> +    await PlacesTestUtils.clearHistory();

instead of a try/finally, you can use do_register_cleanup to register a cleanup async function that runs at the end of the test.
Note that xpcshell tests run in their own single process, so the cleanup is not strictly needed (but still nice to have).

::: toolkit/components/places/tests/migration/test_current_from_v39.js:19
(Diff revision 2)
> +add_task(async function test_select_new_fields() {
> +  let db = await PlacesUtils.promiseDBConnection();
> +  rows = await db.execute("SELECT typed FROM moz_places WHERE id=1 AND typed=2");
> +  Assert.equal(1, rows.length, "moz_place found with typed=2");
> +  rows = await db.execute("SELECT typed FROM moz_hosts WHERE id=1 AND typed=1");
> +  Assert.equal(1, rows.length, "moz_place found with typed=1");

You should probably modify the database through raw SQL queries by inserting some places and visits with old values (I'm sure other migration tests have code examples of that), and then do the upgrade and check the migration did its job. That would have caught the mistake in the migration.

::: toolkit/components/places/tests/migration/xpcshell.ini:27
(Diff revision 2)
>    places_v34.sqlite
>    places_v35.sqlite
>    places_v36.sqlite
>    places_v37.sqlite
>    places_v38.sqlite
> +  places_v39.sqlite

I didn' check yet, please ensure to VACUUM the file before adding it to the patch, since we artificially grow places.sqlite to make it cheaper to insert, but we don't need a grown file here

Comment 16

[Marco Bonardo [:mak]]

As a separate suggestion, I'd suggest to go through the issue in mozreview, and close them once you have handled them, that may help you tracking the remaining work in a cleaner way.