1129978 - Replace custom addVisits() functions in the code base with the one in PlacesTestUtils.jsm

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

promiseAddVisits() should be replaced as well.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1129978-Replace-custom-addVisits-implementations.patch

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1129978-Replace-promiseAddVisits-usage-with-Plac.patch

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1129978-sed-i-s-promiseAddVisits-PlacesTestUtils.patch

Replaced all occurrences with sed. Ran xpcshell tests in toolkit/components/places to ensure this works.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=116719c0d28d

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8560059 [details] [diff] [review]
0001-Bug-1129978-Replace-custom-addVisits-implementations.patch

Review of attachment 8560059 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/test/head.js
@@ +9,5 @@
>  Cu.import("resource://gre/modules/FileUtils.jsm", tmp);
>  Cu.import("resource://gre/modules/osfile.jsm", tmp);
> +Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp);
> +let {PageThumbs, BackgroundPageThumbs, NewTabUtils, PageThumbsStorage,
> +     SessionStore, FileUtils, OS, PlacesTestUtils} = tmp;

I'm not sure why this is importing into tmp rather than just using defineLazyModuleGetter. The result is even better (you get the laziness and don't leak the symbol). oh well...

@@ +14,2 @@
>  
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");

This was only used by AddVisits, so you can remove it.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8560061 [details] [diff] [review]
0003-Bug-1129978-sed-i-s-promiseAddVisits-PlacesTestUtils.patch

Review of attachment 8560061 [details] [diff] [review]:
-----------------------------------------------------------------

I have only a big nit about the patches, that is indentation changes, stuff like:
PlacesTestUtils.addVisits({ uri: something,
                  transition: ...
But there are many point where now it's wrong so, up to you. It's going to be boring as hell.

I think it's more important to land stuff before it bitrots.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #7)
> I have only a big nit about the patches, that is indentation changes, stuff
> like:
> PlacesTestUtils.addVisits({ uri: something,
>                   transition: ...
> But there are many point where now it's wrong so, up to you. It's going to
> be boring as hell.

It was, but indentation is fixed now :)

https://hg.mozilla.org/integration/fx-team/rev/19e59777e22e
https://hg.mozilla.org/integration/fx-team/rev/7d614e61d099
https://hg.mozilla.org/integration/fx-team/rev/f2cbc477d036

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19e59777e22e
https://hg.mozilla.org/mozilla-central/rev/7d614e61d099
https://hg.mozilla.org/mozilla-central/rev/f2cbc477d036