1213078 - PlacesProvider.jsm module for remote new tab page

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Olivier Yiptong [:oyiptong]]

The remote newtab page will do decisioning remotely. Therefore, we don't need most of the code in NewTabUtils.jsm.

This module encapsulates access to the places functionality required

Comment 1

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: changes reviewed elsewhere

changes reviewed elsewhere

Comment 2

[Olivier Yiptong [:oyiptong]]

Attachment: MozReview Request: Bug 1213078 - PlacesProvider.jsm module for remote new tab page r?emtwo

Bug 1213078 - PlacesProvider.jsm module for remote new tab page r?emtwo

Comment 3

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f7906584a1a

Comment 4

[Marina Samuel [:emtwo]]

Comment on attachment 8671643 [details]
MozReview Request: Bug 1213078 - PlacesProvider.jsm module for remote new tab page r?emtwo

https://reviewboard.mozilla.org/r/21623/#review19461

r+ with just a few nits

::: browser/components/newtab/PlacesProvider.jsm:64
(Diff revision 1)
> +      // We got a weird URI or one that would inherit the caller's principal.

Can we log something here?

::: browser/components/newtab/tests/xpcshell/test_PlacesProvider.js:88
(Diff revision 1)
> +    // tests string ordering, a is before o

Nit: This one ends up having the URL being the only difference with firstOlder appearing first right? Did you mean 'f is before o' for the comment?

::: browser/components/newtab/tests/xpcshell/test_PlacesProvider.js:94
(Diff revision 1)
> +    // test ordering

Nit: test ordering by date?

::: browser/components/newtab/tests/xpcshell/test_PlacesProvider.js:150
(Diff revision 1)
> +  yield PlacesTestUtils.clearHistory();

A few of the test functions are directly calling `clearHistory()`. Do we need this here? Wouldn't do_register_cleanup() be clearing history already?

::: browser/components/newtab/tests/xpcshell/test_PlacesProvider.js:218
(Diff revision 1)
> +          ok(true, `all linkChanged events captured`);

There are a few spots in the tests with `ok(true, <msg>)`

Is this just for logging purposes? If so, maybe we can use `do_print(<messageText>)` instead?

Comment 5

[Olivier Yiptong [:oyiptong]]

https://reviewboard.mozilla.org/r/21623/#review19461

> There are a few spots in the tests with `ok(true, <msg>)`
> 
> Is this just for logging purposes? If so, maybe we can use `do_print(<messageText>)` instead?

it is the only assertion in this test, so, it needs to be kept

Comment 6

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6d4011acc3af469693ec35618bd3556032a323
Bug 1213078 - PlacesProvider.jsm module for remote new tab page r=emtwo

Comment 7

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9f20b91e4a29 for  bustage:
https://treeherder.mozilla.org/logviewer.html#?job_id=15465542&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=15465449&repo=mozilla-inbound

Comment 8

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f5c666e42f

Comment 9

[Olivier Yiptong [:oyiptong]]

investigating

Comment 10

[Olivier Yiptong [:oyiptong]]

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

Comment 11

[Olivier Yiptong [:oyiptong]]

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

Comment 12

[Olivier Yiptong [:oyiptong]]

Attachment: bug_1213078.v3.patch

carrying over r+ from emtwo.

this fixes an intermittent test failure

Comment 13

[Olivier Yiptong [:oyiptong]]

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

Comment 14

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c85a831bcf0

Comment 15

[Olivier Yiptong [:oyiptong]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f2559806bb5

Comment 16

[Olivier Yiptong [:oyiptong]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3012b7a2c97c19422775145cad67516d167c16b9
Bug 1213078 - PlacesProvider.jsm module for remote new tab page r=emtwo

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/3012b7a2c97c