Bug 1075620 - Switch to GET for fetch to allow caching of links data from redirect. r=ttaubert, a=sledru
authorEd Lee <edilee@mozilla.com>
Wed, 01 Oct 2014 09:44:32 -0700
changeset 218097 a7f2d0803533
parent 218096 7fac9b913000
child 218098 be43cc1b2373
push id539
push userryanvm@gmail.com
push date2014-10-21 20:12 +0000
treeherdermozilla-release@45dd53a5354b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert, sledru
bugs1075620
milestone33.0.1
Bug 1075620 - Switch to GET for fetch to allow caching of links data from redirect. r=ttaubert, a=sledru Use GET instead of POST and update the fetch endpoint to use %LOCALE%.
browser/app/profile/firefox.js
toolkit/modules/DirectoryLinksProvider.jsm
toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1516,17 +1516,17 @@ pref("browser.newtabpage.enabled", true)
 
 // number of rows of newtab grid
 pref("browser.newtabpage.rows", 3);
 
 // number of columns of newtab grid
 pref("browser.newtabpage.columns", 5);
 
 // directory tiles download URL
-pref("browser.newtabpage.directory.source", "https://tiles.services.mozilla.com/v2/links/fetch");
+pref("browser.newtabpage.directory.source", "https://tiles.services.mozilla.com/v2/links/fetch/%LOCALE%");
 
 // endpoint to send newtab click and view pings
 pref("browser.newtabpage.directory.ping", "https://tiles.services.mozilla.com/v2/links/");
 
 // Enable the DOM fullscreen API.
 pref("full-screen-api.enabled", true);
 
 // True if the fullscreen API requires approval upon a domain entering fullscreen.
--- a/toolkit/modules/DirectoryLinksProvider.jsm
+++ b/toolkit/modules/DirectoryLinksProvider.jsm
@@ -182,16 +182,19 @@ let DirectoryLinksProvider = {
   _removePrefsObserver: function DirectoryLinksProvider_removeObserver() {
     for (let pref in this._observedPrefs) {
       let prefName = this._observedPrefs[pref];
       Services.prefs.removeObserver(prefName, this);
     }
   },
 
   _fetchAndCacheLinks: function DirectoryLinksProvider_fetchAndCacheLinks(uri) {
+    // Replace with the same display locale used for selecting links data
+    uri = uri.replace("%LOCALE%", this.locale);
+
     let deferred = Promise.defer();
     let xmlHttp = new XMLHttpRequest();
 
     let self = this;
     xmlHttp.onload = function(aResponse) {
       let json = this.responseText;
       if (this.status && this.status != 200) {
         json = "{}";
@@ -205,24 +208,22 @@ let DirectoryLinksProvider = {
         });
     };
 
     xmlHttp.onerror = function(e) {
       deferred.reject("Fetching " + uri + " results in error code: " + e.target.status);
     };
 
     try {
-      xmlHttp.open('POST', uri);
+      xmlHttp.open("GET", uri);
       // Override the type so XHR doesn't complain about not well-formed XML
       xmlHttp.overrideMimeType(DIRECTORY_LINKS_TYPE);
       // Set the appropriate request type for servers that require correct types
       xmlHttp.setRequestHeader("Content-Type", DIRECTORY_LINKS_TYPE);
-      xmlHttp.send(JSON.stringify({
-        locale: this.locale,
-      }));
+      xmlHttp.send();
     } catch (e) {
       deferred.reject("Error fetching " + uri);
       Cu.reportError(e);
     }
     return deferred.promise;
   },
 
   /**
--- a/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
+++ b/toolkit/modules/tests/xpcshell/test_DirectoryLinksProvider.js
@@ -51,32 +51,29 @@ const kPingUrl = kBaseUrl + kPingPath;
 Services.prefs.setCharPref(kLocalePref, "en-US");
 Services.prefs.setCharPref(kSourceUrlPref, kTestURL);
 Services.prefs.setCharPref(kPingUrlPref, kPingUrl);
 Services.prefs.setBoolPref(kNewtabEnhancedPref, true);
 
 const kHttpHandlerData = {};
 kHttpHandlerData[kExamplePath] = {"en-US": [{"url":"http://example.com","title":"RemoteSource"}]};
 
-const expectedBodyObject = {locale: DirectoryLinksProvider.locale};
 const BinaryInputStream = CC("@mozilla.org/binaryinputstream;1",
                               "nsIBinaryInputStream",
                               "setInputStream");
 
+let gLastRequestPath;
 function getHttpHandler(path) {
   let code = 200;
   let body = JSON.stringify(kHttpHandlerData[path]);
   if (path == kFailPath) {
     code = 204;
   }
   return function(aRequest, aResponse) {
-    let bodyStream = new BinaryInputStream(aRequest.bodyInputStream);
-    let bodyObject = JSON.parse(NetUtil.readInputStreamToString(bodyStream, bodyStream.available()));
-    isIdentical(bodyObject, expectedBodyObject);
-
+    gLastRequestPath = aRequest.path;
     aResponse.setStatusLine(null, code);
     aResponse.setHeader("Content-Type", "application/json");
     aResponse.write(body);
   };
 }
 
 function isIdentical(actual, expected) {
   if (expected == null) {
@@ -129,17 +126,19 @@ function promiseDirectoryDownloadOnPrefC
   let oldValue = Services.prefs.getCharPref(pref);
   if (oldValue != newValue) {
     // if the preference value is already equal to newValue
     // the pref service will not call our observer and we
     // deadlock. Hence only setup observer if values differ
     let observer = new LinksChangeObserver();
     DirectoryLinksProvider.addObserver(observer);
     Services.prefs.setCharPref(pref, newValue);
-    return observer.deferred.promise;
+    return observer.deferred.promise.then(() => {
+      DirectoryLinksProvider.removeObserver(observer);
+    });
   }
   return Promise.resolve();
 }
 
 function promiseSetupDirectoryLinksProvider(options = {}) {
   return Task.spawn(function() {
     let linksURL = options.linksURL || kTestURL;
     yield DirectoryLinksProvider.init();
@@ -294,17 +293,18 @@ add_task(function test_fetchAndCacheLink
   let data = yield readJsonFile();
   isIdentical(data, kURLData);
 });
 
 add_task(function test_fetchAndCacheLinks_remote() {
   yield DirectoryLinksProvider.init();
   yield cleanJsonFile();
   // this must trigger directory links json download and save it to cache file
-  yield DirectoryLinksProvider._fetchAndCacheLinks(kExampleURL);
+  yield promiseDirectoryDownloadOnPrefChange(kSourceUrlPref, kExampleURL + "%LOCALE%");
+  do_check_eq(gLastRequestPath, kExamplePath + "en-US");
   let data = yield readJsonFile();
   isIdentical(data, kHttpHandlerData[kExamplePath]);
 });
 
 add_task(function test_fetchAndCacheLinks_malformedURI() {
   yield DirectoryLinksProvider.init();
   yield cleanJsonFile();
   let someJunk = "some junk";
@@ -334,17 +334,18 @@ add_task(function test_fetchAndCacheLink
   // File should be empty.
   let data = yield readJsonFile();
   isIdentical(data, "");
 });
 
 add_task(function test_fetchAndCacheLinks_non200Status() {
   yield DirectoryLinksProvider.init();
   yield cleanJsonFile();
-  yield DirectoryLinksProvider._fetchAndCacheLinks(kFailURL);
+  yield promiseDirectoryDownloadOnPrefChange(kSourceUrlPref, kFailURL);
+  do_check_eq(gLastRequestPath, kFailPath);
   let data = yield readJsonFile();
   isIdentical(data, {});
 });
 
 // To test onManyLinksChanged observer, trigger a fetch
 add_task(function test_DirectoryLinksProvider__linkObservers() {
   yield DirectoryLinksProvider.init();
 
@@ -506,16 +507,17 @@ add_task(function test_DirectoryLinksPro
   yield cleanJsonFile();
   // ensure that provider does not think it needs to download
   do_check_false(DirectoryLinksProvider._needsDownload);
 
   // change the source URL, which should force directory download
   yield promiseDirectoryDownloadOnPrefChange(kSourceUrlPref, kExampleURL);
   // then wait for testObserver to fire and test that json is downloaded
   yield testObserver.deferred.promise;
+  do_check_eq(gLastRequestPath, kExamplePath);
   let data = yield readJsonFile();
   isIdentical(data, kHttpHandlerData[kExamplePath]);
 
   yield promiseCleanDirectoryLinksProvider();
 });
 
 add_task(function test_DirectoryLinksProvider_fetchDirectoryOnShow() {
   yield promiseSetupDirectoryLinksProvider();