1388149 - PlacesUtils.history.insertMany ignores provided GUIDs

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

This is causing TPS failures on stage. Trivial patch.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1388149 - Make PlacesUtils.history.insertMany respect provided GUIDs

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

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8894632 [details]
Bug 1388149 - Make PlacesUtils.history.insertMany respect provided GUIDs

https://reviewboard.mozilla.org/r/165792/#review170962

LGTM, although I think we should add a test to toolkit/components/places/tests/history/test_insert.js

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8894632 [details]
Bug 1388149 - Make PlacesUtils.history.insertMany respect provided GUIDs

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8894632 [details]
Bug 1388149 - Make PlacesUtils.history.insertMany respect provided GUIDs

https://reviewboard.mozilla.org/r/165792/#review171506

Looks great, thanks!

::: toolkit/components/places/tests/history/test_insertMany.js:166
(Diff revision 2)
> +      ]
> +    }
> +  ]);
> +
> +  Assert.ok(await PlacesUtils.history.fetch(guidA),
> +            "Record not inserted with correct guid");

We typically phrase these messages for the success case rather than the failure case - ie, "Record is inserted with correct GUID".

::: toolkit/components/places/tests/history/test_insertMany.js:193
(Diff revision 2)
> +    expectedGuids.delete(pageInfo.guid);
> +  });
> +  Assert.equal(expectedGuids.size, 0);
> +
> +
> +  Assert.ok(await PlacesUtils.history.fetch(guidB), "Record B not fetchable after insertMany");

ditto for these 2 lines.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8894632 [details]
Bug 1388149 - Make PlacesUtils.history.insertMany respect provided GUIDs

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a1579259a05
Make PlacesUtils.history.insertMany respect provided GUIDs r=markh

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a1579259a05

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Since bug 1377944 was backed out I'm making this about places rather than Sync