Closed Bug 986521 Opened 6 years ago Closed 6 years ago

Fetch Directory Links data from a network location to cache locally

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: oyiptong, Assigned: emtwo)

References

()

Details

(Whiteboard: p=8 s=it-32c-31a-30b.1 [qa-])

Attachments

(1 file, 2 obsolete files)

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.
Summary: Directory Tiles data is obtained from a network location and periodically self-updates → Directory Links data is obtained from a network location and periodically self-updates
With the browser calling DirectoryLinksProvider.init(); from _finalUIStartup, perhaps we should kick off a request for the remotely hosted from there.
Summary: Directory Links data is obtained from a network location and periodically self-updates → Fetch Directory Links data from a network location to cache locally
Blocks: 993901
Blocks: 993900, tiles-dev
Blocks: 993904
Blocks: 993906
Flags: firefox-backlog?
Version: unspecified → Trunk
Depends on: 993908
Flags: firefox-backlog? → firefox-backlog+
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`.
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.
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.
Assignee: nobody → msamuel
Whiteboard: p=13
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.
Whiteboard: p=13 → p=8
The fetching needs to follow redirects because the server is likely to send a 302 HTTP response to the location of the payload.
Most requests from Firefox should automatically follow redirects, so you don't need to do anything special. Would be good to check though.
Status: NEW → ASSIGNED
Whiteboard: p=8 → p=8 s=it-31c-30a-29b.3 [qa?]
Please needinfo me if you'd like QA feedback on this bug.
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa?] → p=8 s=it-31c-30a-29b.3 [qa-]
oyhttps://bugzilla.mozilla.org/show_bug.cgi?id=993906#c5
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
Should be able to read out nsIRequest to check for .statusCode for the http request
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?
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.
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.
The URL will not have a trailing slash.

It will look as thus:

https://tiles.data.mozilla.com/v1/links/newtab
Depends on: 1001619
* 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
Attachment #8413984 - Flags: review?(adw)
Uploaded the wrong diff before.
Attachment #8413984 - Attachment is obsolete: true
Attachment #8413984 - Flags: review?(adw)
Attachment #8413996 - Flags: review?(adw)
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-] → p=8 s=it-32c-31a-30b.1 [qa-]
No longer blocks: 993904
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.
Attachment #8413996 - Flags: review?(adw)
(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.
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.
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.
Attachment #8413996 - Attachment is obsolete: true
Attachment #8416759 - Flags: review?(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
Attachment #8416759 - Flags: review?(adw) → review+
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
https://hg.mozilla.org/integration/fx-team/rev/7dc573d87fa8
Whiteboard: p=8 s=it-32c-31a-30b.1 [qa-] → [fixed-in-fx-team] p=8 s=it-32c-31a-30b.1 [qa-]
https://hg.mozilla.org/mozilla-central/rev/7dc573d87fa8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] p=8 s=it-32c-31a-30b.1 [qa-] → p=8 s=it-32c-31a-30b.1 [qa-]
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
Hi, is https://hg.mozilla.org/mozilla-central/diff/7dc573d87fa8/bug986521 an accidentally committed file?
Flags: needinfo?(msamuel)
(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.
Flags: needinfo?(msamuel)
Depends on: 1075620
You need to log in before you can comment on or make changes to this bug.