986521 - Fetch Directory Links data from a network location to cache locally

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Olivier Yiptong [:oyiptong]]

This makes DirectoryProvider source data remote-only.

The data source needs to be fetched from a remote network location, and obtains data specific to the user agent's locale.

This data is cached locally and is periodically updated.

Comment 1

[Ed Lee :Mardak]

With the browser calling DirectoryLinksProvider.init(); from _finalUIStartup, perhaps we should kick off a request for the remotely hosted from there.

Comment 2

[Olivier Yiptong [:oyiptong]]

The code fetches data from a network location. 
 
It obtains this network location, a URI, from a pref.
If the data fetch is successful, store the data locally.

We use this now-local data as a cache and it is stored in the profD's root directory in a file named `directoryLinks.json`.

Comment 3

[Ed Lee :Mardak]

Noted in bug 993904 comment 2, if the cache changes (from empty or from previous on-disk value), we'll want to trigger something like Links.resetCache() and maybe also populateCache to have it ready for the newtab pages.

Comment 4

[Ed Lee :Mardak]

This bug is to implement the functionality to fetch the links from bug 993908 and cache to disk. This will allow for bug 993901 to have some logic to determine when to fetch, bug 993904 to switch from the local/packaged links to the cached links, and bug 993906 to augment the data we pass in to the fetch request.

Comment 5

[maxim zhilyaev]

emtwo and maksik think that:
- browser.newtabpage.directorySource stays and points to a server to download links from
- supposedly, "data:" and "chrome:" urls should still work
- we should refactor DirectoryLinksProvider._fetchLinks() to download links and write the file out

Also note that this constant: const DIRECTORY_LINKS_FILE = "directoryLinks.json";
will be independently defined in bug 993904 implementation, which may potentially cause a merge conflict.

Comment 6

[Olivier Yiptong [:oyiptong]]

The fetching needs to follow redirects because the server is likely to send a 302 HTTP response to the location of the payload.

Comment 7

[Ed Lee :Mardak]

Most requests from Firefox should automatically follow redirects, so you don't need to do anything special. Would be good to check though.

Comment 8

[u279076]

Please needinfo me if you'd like QA feedback on this bug.

Comment 9

[maxim zhilyaev]

oyhttps://bugzilla.mozilla.org/show_bug.cgi?id=993906#c5

Comment 10

[maxim zhilyaev]

oyiptong suggest that we need to  handle 204 return code from the server: https://bugzilla.mozilla.org/show_bug.cgi?id=993906#c5.
Hence, we need a unit-test for 204 response

Comment 11

[Ed Lee :Mardak]

Should be able to read out nsIRequest to check for .statusCode for the http request

Comment 12

[Ed Lee :Mardak]

adw, when the DirectoryLinks cache is updated, how should we update the Links cache? Should we just blow away the full cache and call populateCache or trigger onManyLinksChanged where we might need to delete/set frecency=0 of removed links and the appropriate frecency for new links?

Comment 13

[Drew Willcoxon :adw]

DirectoryLinksProvider would handle this fetching internally, right?  Once it fetches a new set of links, it would call onManyLinksChanged on its observers.  That would prompt Links to repopulate its DirectoryLinksProvider cache by calling DirectoryLinksProvider.getLinks, which would return the new entire set of links.

Comment 14

[Olivier Yiptong [:oyiptong]]

The request sent to the server needs to be a POST, due to additional data being sent in bug 993906.

See comment: https://bugzilla.mozilla.org/show_bug.cgi?id=993906#c7

The server will respond with a 303 if there is data found for the requested locale, telling the browser that the redirect should be a GET.

If there is no data for that locale, a 204 response will be returned.

Comment 15

[Olivier Yiptong [:oyiptong]]

The URL will not have a trailing slash.

It will look as thus:

https://tiles.data.mozilla.com/v1/links/newtab

Comment 16

[Marina Samuel [:emtwo]]

Attachment: v1: Fetch directory links and cache them locally

* Fetches may be for data, chrome or http URIs
* For http fetches, these are done with POST, and the body contains JSON with the locale

Comment 17

[Marina Samuel [:emtwo]]

Attachment: v2: Fetch directory links and cache them locally

Uploaded the wrong diff before.

Comment 18

[Marina Samuel [:emtwo]]

try build: https://tbpl.mozilla.org/?tree=Try&rev=3304c16bdd86

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8413996 [details] [diff] [review]
v2: Fetch directory links and cache them locally

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

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +12,4 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

Please make this lazy, too, or maybe I'm not seeing why there's no benefit to that?

@@ -103,3 @@
>          delete this.__linksURL;
> -      }
> -      this._callObservers("onManyLinksChanged");

Just want to make sure I understand this change.  It used to be that changing the links URL would trigger the consumer to call getLinks -- so in desktop Firefox about:newtab would be updated to show the new links.  With this change, that won't happen anymore, right?  Is that what we want?

@@ +158,5 @@
> +  _addRequestBody: function DirectoryLinksProvider_addRequestBody(channel) {
> +    if (channel instanceof Ci.nsIHttpChannel) {
> +      let payload = Cc["@mozilla.org/io/string-input-stream;1"]
> +                      .createInstance(Ci.nsIStringInputStream);
> +      let data = "{\"locale\": \"" + this.locale + "\"}";

let data = JSON.stringify({ locale: this.locale });

@@ +168,5 @@
> +
> +  _fetchAndCacheLinks: function DirectoryLinksProvider_fetchAndCacheLinks(uri) {
> +    let deferred = Promise.defer();
> +    try {
> +      let channel = NetUtil.newChannel(uri);

Instead of NetUtil, please use nsIXMLHttpRequest, which is how we usually fetch remote data in the front end.  It ought to be a little nicer to use, too.

@@ +178,5 @@
> +            json = NetUtil.readInputStreamToString(inputStream,
> +                                                   inputStream.available(),
> +                                                   {charset: "UTF-8"});
> +          }
> +          let directoryLinksFilePath = OS.Path.join(OS.Constants.Path.profileDir, DIRECTORY_LINKS_FILE);

Please use OS.Constants.Path.localProfileDir for cached, replaceable data like this.  (do_get_profile sets that up, too.)

::: toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ +24,3 @@
>  const DIRECTORY_FRECENCY = 1000;
> +const kSourceData = {"en-US": [{"url":"http://example.com","title":"LocalSource"}]};
> +const kTestSource = 'data:application/json,' + JSON.stringify(kSourceData);

Nit: Would be easier to follow if this s/Source/URL/.

@@ +35,5 @@
> +const kBaseUrl = "http://localhost:" + kDefaultServerPort;
> +const kExamplePath = "/exampleTest/";
> +const kFailPath = "/fail/";
> +const kExampleSource = kBaseUrl + kExamplePath;
> +const kFailSource = kBaseUrl + kFailPath;

These, too.

@@ +40,5 @@
> +
> +const kHttpHandlerData = {};
> +kHttpHandlerData[kExamplePath] = {"en-US": [{"url":"http://example.com","title":"RemoteSource"}]};
> +
> +const bodyData = "{\"locale\": \"" + DirectoryLinksProvider.locale + "\"}";

JSON.stringify

@@ +105,5 @@
> +
> +function setupDirectoryLinksProvider(options = {}) {
> +  DirectoryLinksProvider.init();
> +  Services.prefs.setCharPref(kLocalePref, options.locale || "en-US");
> +  Services.prefs.setCharPref(kSourceUrlPref, options.linksURL || kTestSource);

These prefs need to be cleared before the test finishes.  Before your changes, they were cleared within each test task, so I think you need to clear them in cleanDirectoryLinksProvider.  And could you please add a comment above this function that says every call to it must be paired with a call to cleanDirectoryLinksProvider?

@@ +129,3 @@
>  }
>  
> +add_task(function test_DirectoryLinksProvider_fetchAndCacheLinks_local() {

Nit: I don't think you need the DirectoryLinksProvider part in the function name.  Makes it a little hard to read.  Not asking you to go and change all of these, but if you'd like to, please do.

@@ +131,5 @@
> +add_task(function test_DirectoryLinksProvider_fetchAndCacheLinks_local() {
> +  yield cleanJsonFile();
> +  // Trigger cache of data or chrome uri files in profD
> +  yield DirectoryLinksProvider._fetchAndCacheLinks(kTestSource);
> +  let fileObject = yield readJsonFile();

Nit: Maybe call this data or sourceData or linksObject instead of fileObject?  fileObject threw me for a minute because it made me think this was some object that represented the file and not its contents.  Below, too.

@@ +146,5 @@
> +
> +add_task(function test_DirectoryLinksProvider_fetchAndCacheLinks_malformedURI() {
> +  let someJunk = "some junk";
> +  DirectoryLinksProvider._fetchAndCacheLinks(someJunk)
> +    .then(() => do_throw("Malformed URIs should fail"),

If for some reason the fetch doesn't immediately fail like it should, then the harness will move on to the next test task while this one is still going.  So please write this as:

try {
  yield DirectoryLinksProvider._fetchAndCacheLinks(someJunk);
  do_throw("Malformed...");
}
catch (err) {
  do_check_eq(...);
}

Also, I think you should readJsonFile() at the end to make sure the file wasn't created.

@@ +153,5 @@
> +
> +add_task(function test_DirectoryLinksProvider_fetchAndCacheLinks_unknownHost() {
> +  let nonExistentServer = "http://nosuchhost";
> +  DirectoryLinksProvider._fetchAndCacheLinks(nonExistentServer)
> +    .then(() => do_throw("BAD URIs should fail"),

Same here.  This test task doesn't yield, so the harness will move on to the next task before the async work here has finished.  It needs to use yield and try-catch.

And a readJsonFile() to make sure the file wasn't written.

@@ +196,5 @@
>    };
>    let dataURI = 'data:application/json,' + JSON.stringify(data);
>  
> +  setupDirectoryLinksProvider({linksURL: dataURI});
> +  do_check_eq(DirectoryLinksProvider._linksURL, dataURI);

Maybe setupDirectoryLinksProvider should do this check.

@@ +232,5 @@
>    // tests these 2 things:
> +  // 1. _linksURL is properly set after the pref change
> +  // 2. invalid source url is correctly handled
> +  let exampleUrl = 'http://nosuchhost/bad';
> +  Services.prefs.setCharPref(kSourceUrlPref, exampleUrl);

This task should probably clear this pref when done instead of relying on another task or cleanup function to do it.  Actually, clearing it in cleanDirectoryLinksProvider like I suggested should do it since you call that below.

Comment 20

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #19)
> > +      let channel = NetUtil.newChannel(uri);
> Instead of NetUtil, please use nsIXMLHttpRequest, which is how we usually
> fetch remote data in the front end.  It ought to be a little nicer to use,
> too.
emtwo originally implemented it with XHR, but we want to support data/chrome/other URIs too, so they're not necessarily HTTP requests.

Comment 21

[Drew Willcoxon :adw]

nsIXMLHttpRequest works with chrome and data URIs, too.  At least I just tried, to make sure, and it did.  And reading http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp, it looks like it'll work with anything.

Comment 22

[Marina Samuel [:emtwo]]

Attachment: v3: Fetch directory links and cache them locally

Addressed changes.

(In reply to Drew Willcoxon :adw from comment #19)
> Just want to make sure I understand this change.  It used to be that
> changing the links URL would trigger the consumer to call getLinks -- so in
> desktop Firefox about:newtab would be updated to show the new links.  With
> this change, that won't happen anymore, right?  Is that what we want?

That's right. In bug 993901, the logic to download will decide when we have new links to provide (one download case is the links URL pref change). And so "onManyLinksChanged" call is moved to _fetchAndCacheLinks() when a download is successful.

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 8416759 [details] [diff] [review]
v3: Fetch directory links and cache them locally

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

Thanks, sorry for the delay.

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +180,5 @@
> +        });
> +    };
> +
> +    xmlHttp.onerror = function(e) {
> +      deferred.reject("Fetching " + uri + " results in error code: " + e.target.status);

Nit: results -> resulted

Comment 24

[Marina Samuel [:emtwo]]

Replaced "onManyLinksChanged" call where it was before so that this bug will not affect current functionality. Try push looks good. https://tbpl.mozilla.org/?tree=Try&rev=938e34915e85

Comment 25

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/7dc573d87fa8

Comment 26

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/25245190d711

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dc573d87fa8

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Hi, is https://hg.mozilla.org/mozilla-central/diff/7dc573d87fa8/bug986521 an accidentally committed file?

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/25245190d711

Comment 30

[Ed Lee :Mardak]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> Hi, is bug986521 an accidentally committed file?
Yes, that file was removed with the merge into m-c from fx-team some hours ago.