Closed Bug 1213078 Opened 4 years ago Closed 4 years ago

PlacesProvider.jsm module for remote new tab page

Categories

(Firefox :: New Tab Page, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: oyiptong, Assigned: oyiptong)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
changes reviewed elsewhere
Bug 1213078 - PlacesProvider.jsm module for remote new tab page r?emtwo
Attachment #8671643 - Flags: review?(msamuel)
Attachment #8671642 - Attachment is obsolete: true
Attachment #8671643 - Flags: review?(msamuel) → review+
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?
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
investigating
Flags: needinfo?(oyiptong)
carrying over r+ from emtwo.

this fixes an intermittent test failure
Attachment #8671643 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3012b7a2c97c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.