975211 - Create backend logic to provide list of Tiles and associated metadata (image, text, background color)

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

New Tab's current thumbnail service fetches tiles only from background tab thumbnailing.  Fetching should be augmented to handle three different sources:

1. Background tab screenshots (current state)
2. Partner/Sponsored tile images and text (possibly background color also)
3. Mozilla internal tile images and text

Comment 1

[Dão Gottwald [:dao]]

What's the difference between this bug and bug 975210? The descriptions are identical.

Comment 2

[Ed Lee :Mardak]

(In reply to Dão Gottwald [:dao] from comment #1)
> What's the difference between this bug and bug 975210?
This bug will focus on packaging the data/metadata for the Directory tiles we want to show in a format similar to the output of PlacesProvider.getLinks.

Comment 3

[Ed Lee :Mardak]

clarkbw, I'm curious if we want to tag affinity-type tiles as sponsored. The current copy is "This site is being suggested because it has sponsored Mozilla, helping us promote openness, innovation, and opportunity on the Web." and marking say.. Wikipedia as sponsored doesn't seem too wrong in that they do help promote openness, innovation, and opportunity on the Web.

Another way to look at it is arguably Mozilla is sponsoring these like-minded affiliates because theoretically missions, etc. align.

Hopefully this would also clear up confusion that Directory Tiles is only for commercial advertising when it's not.

Comment 4

[Ed Lee :Mardak]

There were some engineering notes from now-duped bug 965437 comment 0 regarding potential approaches to provide the appropriate tile data.

Comment 5

[Olivier Yiptong [:oyiptong]]

From what I gather, the first version of Directory Tiles will feature a hard-coded list.

This hard-coded list is locale specific, and the first version will only contain the Tiles for en-US.

Will we abstain from showing the tiles for other locales?

Comment 6

[Olivier Yiptong [:oyiptong]]

clarkbw: will we also have affinity tiles in version 1?

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Ed Lee :Mardak from comment #3)
> clarkbw, I'm curious if we want to tag affinity-type tiles as sponsored.

That's an interesting question.

> Hopefully this would also clear up confusion that Directory Tiles is only
> for commercial advertising when it's not.

Maybe another way could be to have a very similar "affinity mark" which helps explain them separately.  I like the idea of being explicit about the tiles that aren't sponsored.  I'm not sure that we want to mark Tiles as sponsored who we don't have the same agreement with, I'm worried we'd dilute that concept.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Olivier Yiptong [:oyiptong] from comment #5)
> From what I gather, the first version of Directory Tiles will feature a
> hard-coded list.

Yes, the first version will have a set list that ships with the version of Firefox it's built into.

> This hard-coded list is locale specific, and the first version will only
> contain the Tiles for en-US.

Yes.

> Will we abstain from showing the tiles for other locales?

Yes.

(In reply to Olivier Yiptong [:oyiptong] from comment #6)
> clarkbw: will we also have affinity tiles in version 1?

The first list is a mix of affinity, top sites, and sponsored.  Currently only 3 tiles at maximum would be sponsored, the rest are just tiles we feel are of interest to Firefox users.

Comment 9

[Ed Lee :Mardak]

adw, do you have preference on how we add this data the Link type object? I see the structure will be:

{
  url: ..,
  title: ..,
  rank: {
    frecency: ..,
    placeID: ..
  }
}

oyiptong is currently implementing this data as:

{
  url: ..,
  title: ..,
  meta: {
    thumbUrl : ..,
    bgColor: ..,
    type: .. (one of "sponsored", "organic", etc.)
  }
}

Comment 10

[Drew Willcoxon :adw]

That sounds OK to me, except "meta" is a vague name, and isn't all the data in the structure metadata?  It's hard to say without seeing this in context in a patch, though.

Comment 11

[Ed Lee :Mardak]

Was there a particular reason to not directly include frecency and placeID at the top level? Perhaps we should name "meta" "directory" instead.

Comment 12

[Drew Willcoxon :adw]

I'm trying to define an interface between providers and the front end by keeping provider-specific data distinct from data that NewTabUtils.jsm and the front-end newtab code need to do their jobs.  Frecency and placeID are both relevant to PlacesProvider but not necessarily to other hypothetical providers, which would store other data in `rank` that they would need in order to compare two link objects.

If meta is supposed to be opaque, provider-specific data, then I think it would be clearer to call it providerData.  In that case, rank ought to be renamed providerData in my patch, too.

Now, if you end up modifying the front-end newtab code so that it assumes that all link objects have bgColor, etc. properties, then those properties would become part of the interface between providers and the front end that all providers would be expected to implement.

Comment 13

[Olivier Yiptong [:oyiptong]]

Yes, I agree that "meta" is vague. providerData sounds like a good place to store this.

Afaik, it is not in our plan for other link objects to have the properties.

Comment 14

[Tracy Walker [:tracy]]

This looks like design bug, yes? if so [qa-] is correct.  please change it to [qa+] if this is the bug the work will be done in.

Comment 15

[Ed Lee :Mardak]

adw, our current approach is to package a directoryTiles.json that is referenced via chrome://global/content/directoryTiles.json then cached. Down the line we'll want to fetch a dyanmic/remotely hosted file that we would probably store in the profile directory. Should the code for now just directly reference chrome://..json and then we add in logic to first check the profile directory then fall back to the chrome file?

Any preference?

Comment 16

[Drew Willcoxon :adw]

Sure, that sounds fine.  Since you're packaging a json file, does that mean the data provided by the file isn't going to change between Firefox releases, at least for now?

Comment 17

[Ed Lee :Mardak]

Yup, I believe the plan was to update the list on the 6-week cycle, but the timeline for remotely hosted json might be sped up.

Comment 18

[Ed Lee :Mardak]

Attachment: v1

Ignore the data in the json file as they're just placeholders as show in https://bug975210.bugzilla.mozilla.org/attachment.cgi?id=8386468

adw, is the pref for tiles source pointing to chrome:// okay or should it be hardcoded? The other use like this is for the hidden window. Potentially we'll use this for the remotely hosted.

Also, any preference on the task/promise/callback setup?

Comment 19

[Ed Lee :Mardak]

Attachment: v1 (no json)

Just making the patch more readable with -U8 and without the json reference data file.

Comment 20

[Drew Willcoxon :adw]

Comment on attachment 8387745 [details] [diff] [review]
v1 (no json)

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

I posted a big comment to bug 975228 that talks about overall architecture and touches on a lot of this patch.  So I'll clear the review for now, pending discussion about my comment, but I do have a few comments inline.

::: browser/app/profile/firefox.js
@@ +1306,5 @@
>  
>  // number of columns of newtab grid
>  pref("browser.newtabpage.columns", 3);
>  
> +pref("browser.newtabpage.directory_tiles_source", "chrome://global/content/directoryTiles.json");

directoryTilesSource

::: toolkit/modules/NewTabUtils.jsm
@@ +12,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");

resource://gre/modules/Promise.jsm

@@ +79,5 @@
> + * @return  the selected locale or "en-US" if none is selected
> + */
> +function getLocale() {
> +  try {
> +    if (Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE)) {

I think you're only catching the pref getter, not the getLocaleComponentForUserAgent(), right?  If so, please catch only it so we don't eat unexpected errors.  Something like:

try {
  var matchOS = getBoolPref();
}
catch (e) {}
if (matchOS)
  return ...;

@@ +89,5 @@
> +  try {
> +    let locale = Services.prefs.getComplexValue(PREF_SELECTED_LOCALE,
> +                                                Ci.nsIPrefLocalizedString);
> +    if (locale) {
> +      return locale;

Does this work?  locale is an nsIPrefLocalizedString, so don't you need to return locale.data?  If it works because getLocale's return value is always coerced into a string, and locale.toString() is the same as locale.data, then please return locale.data here.

@@ +646,5 @@
> +  },
> +
> +  _removePrefsObserver: function DirectoryTilesProvider_removeObserver() {
> +    for (let pref in this._prefs) {
> +      let prefValue = this._prefs[pref];

prefValue -> prefName

@@ +659,5 @@
> +  _fetchLinks: function DirectoryTilesProvider_fetchLinks() {
> +    let deferred = Promise.defer();
> +
> +    try {
> +      NetUtil.asyncFetch(this._tilesUrl, (aInputStream, aResult, aRequest) => {

Please use the OS.File API instead of NetUtil now: https://developer.mozilla.org/en-US/docs/JavaScript_OS.File

@@ +673,5 @@
> +            let locale = getLocale();
> +
> +            let links;
> +            if (data.hasOwnProperty(locale)) {
> +              links = data[locale];

1) Is it possible for this conditional to be false?  Wouldn't that be kind of a problem?  2) It seems wasteful to ship data for all locales within each Firefox package.  I wonder if we could ship only the relevant locale the same way other localized files are.

Comment 21

[Olivier Yiptong [:oyiptong]]

(In reply to Drew Willcoxon :adw from comment #20)
> Comment on attachment 8387745 [details] [diff] [review]

> @@ +79,5 @@
> > + * @return  the selected locale or "en-US" if none is selected
> > + */
> > +function getLocale() {
> > +  try {
> > +    if (Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE)) {
> 
> I think you're only catching the pref getter, not the
> getLocaleComponentForUserAgent(), right?  If so, please catch only it so we
> don't eat unexpected errors.  Something like:
> 
> try {
>   var matchOS = getBoolPref();
> }
> catch (e) {}
> if (matchOS)
>   return ...;
> 

Can you please elaborate? In this code, we don't care why Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE) or getLocaleComponentForUserAgent() failed.

If either fail and the code tries something else. In that case, PREF_SELECTED_LOCALE.

Would logging via Cu.reportError in the catch clause assuage your concern?

> @@ +659,5 @@
> > +  _fetchLinks: function DirectoryTilesProvider_fetchLinks() {
> > +    let deferred = Promise.defer();
> > +
> > +    try {
> > +      NetUtil.asyncFetch(this._tilesUrl, (aInputStream, aResult, aRequest) => {
> 
> Please use the OS.File API instead of NetUtil now:
> https://developer.mozilla.org/en-US/docs/JavaScript_OS.File

By the way it looks, we're going to land with remote-hosted data. That requires us to us NetUtil.

> @@ +673,5 @@
> > +            let locale = getLocale();
> > +
> > +            let links;
> > +            if (data.hasOwnProperty(locale)) {
> > +              links = data[locale];
> 
> 1) Is it possible for this conditional to be false?  Wouldn't that be kind
> of a problem?  2) It seems wasteful to ship data for all locales within each
> Firefox package.  I wonder if we could ship only the relevant locale the
> same way other localized files are.

1) it is written that way, because it _is_ possible for this condition to be false. For instance, we were only planning to land with en-US tiles.

Now, wrt 2) it now seems that we're going to land with remote data hosting. We were discussing pre-seeding firefox with data, and you're right, we can ship the data with the locales.

There was a reason why we had put all tiles in one file, as opposed to a per-locale location, but that reason now escapes me.

Comment 22

[Drew Willcoxon :adw]

(In reply to Olivier Yiptong [:oyiptong] from comment #21)
> Can you please elaborate? In this code, we don't care why
> Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE) or
> getLocaleComponentForUserAgent() failed.

It's common to catch pref service getters because they're expected to throw, because they throw for the dumb, unexceptional reason of their prefs not existing.  Do you have a reason to expect that getLocaleComponentForUserAgent might throw?  If not, please don't catch it for the same reason we don't catch everything, so unexpected errors fail obviously and terminate the normal code flow.

Comment 23

[Ed Lee :Mardak]

Attachment: wip v2

Comment 24

[Ed Lee :Mardak]

Attachment: wip v2 (no json)

Comment 25

[Ed Lee :Mardak]

wip as oyiptong is in the process of converting to OS.File and removing the linksCache

Comment 26

[Ed Lee :Mardak]

Attachment: v2 (no json)

Comment 27

[Olivier Yiptong [:oyiptong]]

based on a conversation we've had on IRC, adw OK'd the use of NetUtils.asyncFetch to obtain the json payload.

Since we're storing the json payload in the omnijar, NetUtils looks more appropriate.

Comment 28

[Drew Willcoxon :adw]

Comment on attachment 8391673 [details] [diff] [review]
v2 (no json)

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

This looks OK overall.  I need to look at the tests still, but in the meantime here are some comments.

::: toolkit/modules/NewTabUtils.jsm
@@ +74,4 @@
>  
>  // The timeout period used in scheduleUpdateForHiddenPages.
>  const SCHEDULE_UPDATE_TIMEOUT = 1000;
> +//

Nit: Please make this line blank.

@@ +76,5 @@
>  const SCHEDULE_UPDATE_TIMEOUT = 1000;
> +//
> +// The threshold when a Places link becomes a history tile and pushes out a
> +// directory tile.
> +const TILES_FRECENCY_THRESHOLD = 1000;

"Tiles" is ambiguous since really all the sites in the grid are tiles, so please use the word "sponsored" or "directory," whatever we're calling these.

Also, "threshold" isn't great, since this value actually becomes the sponsored links' frecency.

SPONSORED_FRECENCY is the name I'd prefer.

@@ +736,5 @@
> + * Singleton that serves as the provider of directory tiles.
> + * Directory Tiles are a hard-coded set of links shown if a user's tile
> + * inventory is empty.
> + */
> +let DirectoryTilesProvider = {

Can we call these "sponsored tiles" instead?  So SponsoredProvider?  That's much clearer than "directory tiles."  Or are there plans to have directory tiles that aren't sponsored?  If we have to use the word directory, then could we just say DirectoryProvider?  Like I said above, "tiles" applies to all the sites in the grid.

@@ +738,5 @@
> + * inventory is empty.
> + */
> +let DirectoryTilesProvider = {
> +
> +  __tilesUrl: null,

Please capitalize like tilesURL, here and everywhere else.

@@ +743,5 @@
> +
> +  _observers: [],
> +
> +  get _prefs() Object.freeze({
> +    tilesUrl: "browser.newtabpage.directoryTilesSource",

Please keep this pref name in a const at the top of the file like you do for the other prefs.

@@ +753,5 @@
> +    if (!this.__tilesUrl) {
> +      try {
> +        this.__tilesUrl = Services.prefs.getCharPref(this._prefs["tilesUrl"]);
> +      }
> +      catch(e) {

Nit: catch (e)

@@ +764,5 @@
> +  observe: function DirectoryTilesProvider_observe(aSubject, aTopic, aData) {
> +    if (aTopic == "nsPref:changed") {
> +      if (aData == this._prefs["tilesUrl"]) {
> +        try {
> +          this.__tilesUrl = Services.prefs.getCharPref(this._prefs["tilesUrl"]);

Please do simply

  delete this.__tilesURL;

instead so that the pref is still lazily fetched, and is fetched in one place.  You don't need the try-catch then either, and in fact you could call _callObservers unconditionally since you call it for all pref changes.

@@ +797,5 @@
> +   * Fetches the current set of directory links.
> +   * @returns a set of links in a promise.
> +   */
> +  _fetchLinks: function DirectoryTilesProvider_fetchLinks() {
> +    let deferred = Promise.defer();

Is it really necessary to use a promise here?  The only caller is getLinks, who effectively passes in a callback.

@@ +818,5 @@
> +          deferred.reject(new Error("the fetch of " + this._tilesUrl + "was unsuccessful"));
> +        }
> +      });
> +    }
> +    catch(e) {

Nit: catch (e)

@@ +852,5 @@
> +  /**
> +   * Return the object to its pre-init state
> +   */
> +  reset: function DirectoryTilesProvider_reset() {
> +    this.__tilesUrl = undefined;

delete this.__tilesURL;

@@ +874,5 @@
> +    }
> +  },
> +
> +  _removeObservers: function() {
> +    while(this._observers.length) {

Nit: while (

Comment 29

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #28)
> > +let DirectoryTilesProvider = {
> Can we call these "sponsored tiles" instead?  So SponsoredProvider?  That's
> much clearer than "directory tiles."  Or are there plans to have directory
> tiles that aren't sponsored?
"Directory Tiles" can include sponsored, organic (popular), and affiliate (unpaid sponsored), and I believe we'll want to set the type value accordingly. We don't need to use the feature name as the variable names, so if we can think of something clearer than "DirectoryProvider," that should be good.

Comment 30

[Olivier Yiptong [:oyiptong]]

Attachment: wip v3

Comment 31

[Olivier Yiptong [:oyiptong]]

Attachment: v3 (no json)

Comment 32

[Ed Lee :Mardak]

Comment on attachment 8393124 [details] [diff] [review]
v3 (no json)

Here's a github interdiff/compare: https://github.com/Mardak/tiles-dev/compare/846ac0f...bug975211

Comment 33

[Olivier Yiptong [:oyiptong]]

Attachment: v4

Comment 34

[Olivier Yiptong [:oyiptong]]

Attachment: v4 (no json)

small change that corrects a typo from v3

Comment 35

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8393908 [details] [diff] [review]
v4 (no json)

Same git diff url as Ed posted works: https://github.com/Mardak/tiles-dev/compare/846ac0f...bug975211

Comment 36

[Drew Willcoxon :adw]

Comment on attachment 8393908 [details] [diff] [review]
v4 (no json)

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

I didn't think of this earlier, but the directory tiles stuff should be in browser/, not toolkit/.  Places lives in toolkit so it makes sense for PlacesProvider to be in toolkit, but directory tiles are specific to Firefox.

So please move DirectoryProvider to a browser/modules/DirectoryTilesProvider.jsm and rename it back to DirectoryTilesProvider.  Then you can add your test to browser/modules/test/unit/, and it can test DirectoryTilesProvider in isolation without needing NewTabUtils.  You'll need to add the provider to NewTabUtils.links where NewTabUtils is inited here: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#473

Sorry for this, my fault.

Other than that, this looks OK except for some of the tests.

::: toolkit/modules/NewTabUtils.jsm
@@ +77,5 @@
>  // The timeout period used in scheduleUpdateForHiddenPages.
>  const SCHEDULE_UPDATE_TIMEOUT = 1000;
>  
> +// The threshold when a Places link becomes a history tile and pushes out a
> +// directory tile.

This comment is kind of weird.  All Places links are history links, right?  So they don't become history links at some late point.  Second, this value just means that directory links are given a frecency ranking of 1000, so they're ranked above other links with frecencies < 1000 and below other links with frecencies > 1000.  There's not really any "pushing out."

@@ +828,5 @@
> +      // reached by a history tile, the last tile is pushed out
> +      aCallback(rawLinks.map((link, position) => {
> +        link.frecency = DIRECTORY_FRECENCY;
> +        link.lastVisitDate = rawLinks.length - position;
> +        return link;

Is link.type defined in the JSON?

::: toolkit/modules/tests/xpcshell/newtab/test_DirectoryProvider.js
@@ +70,5 @@
> +
> +  let links;
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 1);

And you should make sure the link's properties are right.

@@ +77,5 @@
> +
> +  links = yield fetchData(provider);
> +
> +  // results should be unchanged
> +  do_check_true(links.length == 1);

I don't see the point of this.  You're setting the links variable to the array returned from fetchData, so how would it contain the new URL you pushed to the previous value of links?

@@ +103,5 @@
> +
> +  let links;
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 1)

And you should make sure the link's properties are right.

@@ +108,5 @@
> +
> +  Services.prefs.setCharPref('general.useragent.locale', 'cn-ZH');
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 2)

Here too, both of the links in this case.

@@ +110,5 @@
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 2)
> +
> +  Services.prefs.setCharPref('general.useragent.locale', 'en-US');

Use clearUserPref instead.

@@ +116,5 @@
> +});
> +
> +add_task(function test_DirectoryProvider__prefObserver_url() {
> +  let provider = NewTabUtils._providers.directory;
> +  Services.prefs.setCharPref(provider._prefs['tilesURL'], kDefaultTileSource);

This isn't necessary since each test here calls provider.reset() at the end, which makes sure that the tilesURL is the default URL.

@@ +136,5 @@
> +
> +  provider.reset();
> +});
> +
> +add_task(function test_DirectoryProvider_getLinks() {

This is exactly the same as the first part of the previous test, so it's not necessary at all.

@@ +145,5 @@
> +  do_check_true(links.length > 0);
> +  provider.reset();
> +});
> +
> +add_task(function test_DirectoryProvider_getLinks_invalid() {

And this is the same as the second part, so it's not necessary either.

@@ +162,5 @@
> +  Services.prefs.setCharPref(provider._prefs['tilesURL'], dataURI);
> +
> +  let links = yield fetchData(provider);
> +  do_check_eq(links.length, 0);
> +  provider.reset();

Need to clearUserPref the local pref, too.

Comment 37

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #36)
> @@ +162,5 @@
> > +  Services.prefs.setCharPref(provider._prefs['tilesURL'], dataURI);
> > +
> > +  let links = yield fetchData(provider);
> > +  do_check_eq(links.length, 0);
> > +  provider.reset();
> 
> Need to clearUserPref the local pref, too.

locale pref

Comment 38

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #36)
> I didn't think of this earlier, but the directory tiles stuff should be in
> browser/, not toolkit/
Is that true if this code might be used for Android? Can this code be reused for Android?

Comment 39

[Drew Willcoxon :adw]

Yeah, good point.  But directory links shouldn't yet be included in NewTabUtils.links on Android, since they haven't updated their front-end to work with them, right?  So I still think there should be a separate DirectoryLinksProvider.jsm, in toolkit/modules/, and DirectoryLinksProvider should be added to NewTabUtils.links in nsBrowserGlue.js.  I don't think there's any need for a new toolkit/modules/newtab/ directory like the patch makes.

About the name -- DirectoryLinksProvider instead of DirectoryTilesProvider since, if it's in the back end, in toolkit, there shouldn't be an assumption that the front end will display the links as tiles; and DirectoryLinksProvider instead of DirectoryProvider just for better clarity.  Also, all the comments and identifiers in DirectoryLinksProvider ought to use "links" instead of "tiles," so linksURL instead of tilesURL, "directory links" instead of "directory tiles," etc.  Does that sound OK?

Comment 40

[Olivier Yiptong [:oyiptong]]

That sounds good.

Comment 41

[Olivier Yiptong [:oyiptong]]

(In reply to Drew Willcoxon :adw from comment #36)
> @@ +828,5 @@
> > +      // reached by a history tile, the last tile is pushed out
> > +      aCallback(rawLinks.map((link, position) => {
> > +        link.frecency = DIRECTORY_FRECENCY;
> > +        link.lastVisitDate = rawLinks.length - position;
> > +        return link;
>
> Is link.type defined in the JSON?

Yes, the type is defined in the JSON, and can be different, depending on the link.

> @@ +77,5 @@
> > +
> > +  links = yield fetchData(provider);
> > +
> > +  // results should be unchanged
> > +  do_check_true(links.length == 1);
>
> I don't see the point of this.  You're setting the links variable to the
> array returned from fetchData, so how would it contain the new URL you
> pushed to the previous value of links?

It is a vestige of the cached implementation, and should be removed.

> @@ +116,5 @@
> > +});
> > +
> > +add_task(function test_DirectoryProvider__prefObserver_url() {
> > +  let provider = NewTabUtils._providers.directory;
> > +  Services.prefs.setCharPref(provider._prefs['tilesURL'], kDefaultTileSource);
>
> This isn't necessary since each test here calls provider.reset() at the end,
> which makes sure that the tilesURL is the default URL.

It seems that in xpcshell tests, browser/app/profile/firefox.js is not loaded, therefore the default url is never loaded.
reset() just deletes tilesURL.

I side stepped the issue anyway, by choosing a less confusing name for the test in the upcoming patch.

Comment 42

[Olivier Yiptong [:oyiptong]]

Attachment: v5

Comment 43

[Olivier Yiptong [:oyiptong]]

Attachment: v5 (no json)

Comment 44

[Olivier Yiptong [:oyiptong]]

Attachment: v5a

Comment 45

[Olivier Yiptong [:oyiptong]]

Attachment: v5a (no json)

Comment 46

[Olivier Yiptong [:oyiptong]]

Attachment: v5b

Comment 47

[Olivier Yiptong [:oyiptong]]

Attachment: v5b (no json)

Comment 48

[Drew Willcoxon :adw]

Comment on attachment 8395168 [details] [diff] [review]
v5b (no json)

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

r+ with the changes below.

::: toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ +87,5 @@
> +
> +  let links;
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 1)

You need to make sure the fetched link matches data["en-US"][0].  You have the isIdentical helper.

@@ +92,5 @@
> +
> +  Services.prefs.setCharPref('general.useragent.locale', 'cn-ZH');
> +
> +  links = yield fetchData(provider);
> +  do_check_eq(links.length, 2)

Same here but for data["cn-ZH"].

@@ +110,5 @@
> +  do_check_eq(provider._linksURL, kTestSource);
> +
> +  let links = yield fetchData(provider);
> +  do_check_eq(links.length, 1);
> +  do_check_eq(links[0].title, "TestSource");

You should check the url property, too.  It would be simple to define the TestSource's links as an array at the top of the file.  Then you could generate kTestSource from JSON.stringify(kTestSourceLinks), and you could use isIdentical to compare the fetched links array with kTestSourceLinks.

@@ +131,5 @@
> +
> +add_task(function test_DirectoryLinksProvider_getLinks_noLocaleData() {
> +  let provider = DirectoryLinksProvider;
> +  Services.prefs.setCharPref('general.useragent.locale', 'cn-ZH');
> +  let dataURI = 'data:application/json,{"en-US":[{"url":"http://example.com","title":"example"}]}';

This looks like kTestSource?

Comment 49

[Drew Willcoxon :adw]

Oh, the test needs to also make sure that link.frecency and lastVisitDate are correct.

Comment 50

[Ed Lee :Mardak]

Attachment: for checkin

Comment 51

[Ed Lee :Mardak]

Seems like this patch for check-in doesn't require bug 911307 to land, and new tests are passing fine:

https://tbpl.mozilla.org/?tree=Try&rev=e9c1919664c5

Comment 52

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8e27e02893

Comment 53

[Carsten Book [:Tomcat]]

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