Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for e10s. r?dao draft
authorDrew Willcoxon <adw@mozilla.com>
Thu, 12 May 2016 15:48:15 -0700
changeset 366573 820dd545217f21f6e2b5d1c1014f24a147617b00
parent 364228 8000d7a6ff44b128850bd4d08e0d1867f6df557d
child 520801 3a9f1bd2e3328a451934220e62a2bcce397f1e0a
push id18013
push userdwillcoxon@mozilla.com
push dateThu, 12 May 2016 22:48:42 +0000
reviewersdao
bugs1245118, 1088203
milestone49.0a1
Bug 1245118 - [e10s] Fix browser_thumbnails_update.js for e10s. r?dao Bug 1088203 broke this test (and the PageThumbs API, IMO) in e10s. MozReview-Commit-ID: FN2nonCqw3W
toolkit/components/thumbnails/PageThumbs.jsm
toolkit/components/thumbnails/test/browser_thumbnails_update.js
toolkit/components/thumbnails/test/thumbnails_update.sjs
--- a/toolkit/components/thumbnails/PageThumbs.jsm
+++ b/toolkit/components/thumbnails/PageThumbs.jsm
@@ -369,17 +369,19 @@ this.PageThumbs = {
     let channelError = false;
 
     if (!aBrowser.isRemoteBrowser) {
       let channel = aBrowser.docShell.currentDocumentChannel;
       originalURL = channel.originalURI.spec;
       // see if this was an error response.
       channelError = this._isChannelErrorResponse(channel);
     } else {
-      // We need channel info (bug 1073957)
+      // In this case the thumbnail will be unconditionally stored, and any
+      // existing thumbnail will be overwritten.  If that's not what you want,
+      // you should call shouldStoreThumbnail before calling this method.
       originalURL = url;
     }
 
     Task.spawn((function* task() {
       let isSuccess = true;
       try {
         let blob = yield this.captureToBlob(aBrowser);
         let buffer = yield TaskUtils.readBlob(blob);
--- a/toolkit/components/thumbnails/test/browser_thumbnails_update.js
+++ b/toolkit/components/thumbnails/test/browser_thumbnails_update.js
@@ -4,20 +4,19 @@
 /**
  * These tests check the auto-update facility of the thumbnail service.
  */
 
 function* runTests() {
   // A "trampoline" - a generator that iterates over sub-iterators
   let tests = [
     simpleCaptureTest,
-    capIfStaleErrorResponseUpdateTest,
+    shouldStoreThumbnailErrorResponseTest,
     capIfStaleGoodResponseUpdateTest,
-    regularCapErrorResponseUpdateTest,
-    regularCapGoodResponseUpdateTest
+    shouldStoreThumbnailOKResponseTest
   ];
   for (let test of tests) {
     info("Running subtest " + test.name);
     for (let iterator of test())
       yield iterator;
   }
 }
 
@@ -38,17 +37,19 @@ function getThumbnailModifiedTime(url) {
   file.initWithPath(fname);
   return file.lastModifiedTime;
 }
 
 // The tests!
 /* Check functionality of a normal captureAndStoreIfStale request */
 function* simpleCaptureTest() {
   let numNotifications = 0;
-  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?simple";
+  // Set nostore to disable gBrowserThumbnails's on-load capture, which would
+  // interfere with this test.
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?status=200&nostore";
 
   function observe(subject, topic, data) {
     is(topic, "page-thumbnail:create", "got expected topic");
     is(data, URL, "data is our test URL");
     if (++numNotifications == 2) {
       // This is the final notification and signals test success...
       Services.obs.removeObserver(observe, "page-thumbnail:create");
       gBrowser.removeTab(gBrowser.selectedTab);
@@ -75,51 +76,41 @@ function* simpleCaptureTest() {
       PageThumbs.captureAndStoreIfStale(browser);
       // But it's async, so wait - our observer above will call next() when
       // the notification comes.
     });
   });
   yield undefined // wait for callbacks to call 'next'...
 }
 
-/* Check functionality of captureAndStoreIfStale when there is an error response
+/* Check functionality of shouldStoreThumbnail when there is an error response
    from the server.
  */
-function* capIfStaleErrorResponseUpdateTest() {
-  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?fail";
+function* shouldStoreThumbnailErrorResponseTest() {
+  // Do not pass nostore because otherwise
+  // PageThumbUtils.shouldStoreContentThumbnail would return false based on that
+  // alone.
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?status=401";
   yield addTab(URL);
-
-  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
-  // update the thumbnail to be stale, then re-request it.  The server will
-  // return a 400 response and a red thumbnail.
-  // The service should not save the thumbnail - so we (a) check the thumbnail
-  // remains green and (b) check the mtime of the file is < now.
-  ensureThumbnailStale(URL);
-  yield navigateTo(URL);
-  // now() returns a higher-precision value than the modified time of a file.
-  // As we set the thumbnail very stale, allowing 1 second of "slop" here
-  // works around this while still keeping the test valid.
-  let now = Date.now() - 1000 ;
-  PageThumbs.captureAndStoreIfStale(gBrowser.selectedBrowser, () => {
-    ok(getThumbnailModifiedTime(URL) < now, "modified time should be < now");
-    retrieveImageDataForURL(URL, function ([r, g, b]) {
-      is("" + [r,g,b], "" + [0, 255, 0], "thumbnail is still green");
-      gBrowser.removeTab(gBrowser.selectedTab);
-      next();
-    });
+  PageThumbs.shouldStoreThumbnail(gBrowser.selectedBrowser, shouldStore => {
+    ok(!shouldStore, "Should not store thumbnail");
+    gBrowser.removeTab(gBrowser.selectedTab);
+    // In non-e10s, shouldStoreThumbnail is sync.  next must be called asyncly.
+    executeSoon(next);
   });
   yield undefined; // wait for callback to call 'next'...
 }
 
 /* Check functionality of captureAndStoreIfStale when there is a non-error
-   response from the server.  This test is somewhat redundant - although it is
-   using a http:// URL instead of a data: url like most others.
+   response from the server.
  */
 function* capIfStaleGoodResponseUpdateTest() {
-  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?ok";
+  // Set nostore to disable gBrowserThumbnails's on-load capture, which would
+  // interfere with this test.
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?nostore";
   yield addTab(URL);
   let browser = gBrowser.selectedBrowser;
 
   yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
   // update the thumbnail to be stale, then re-request it.  The server will
   // return a 200 response and a red thumbnail - so that new thumbnail should
   // end up captured.
   ensureThumbnailStale(URL);
@@ -135,35 +126,23 @@ function* capIfStaleGoodResponseUpdateTe
     retrieveImageDataForURL(URL, function ([r, g, b]) {
       is("" + [r,g,b], "" + [255, 0, 0], "thumbnail is now red");
       next();
     });
   });
   yield undefined; // wait for callback to call 'next'...
 }
 
-/* Check functionality of captureAndStore when there is an error response
+/* Check functionality of shouldStoreThumbnail when there is an OK response
    from the server.
  */
-function* regularCapErrorResponseUpdateTest() {
-  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?fail";
+function* shouldStoreThumbnailOKResponseTest() {
+  // Do not pass nostore because otherwise
+  // PageThumbUtils.shouldStoreContentThumbnail would return false.
+  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?status=200";
   yield addTab(URL);
-  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
-  gBrowser.removeTab(gBrowser.selectedTab);
-  // do it again - the server will return a 400, so the foreground service
-  // should not update it.
-  yield addTab(URL);
-  yield captureAndCheckColor(0, 255, 0, "we still have a green thumbnail");
+  PageThumbs.shouldStoreThumbnail(gBrowser.selectedBrowser, shouldStore => {
+    ok(shouldStore, "Should store thumbnail");
+    gBrowser.removeTab(gBrowser.selectedTab);
+    // In non-e10s, shouldStoreThumbnail is sync.  next must be called asyncly.
+    executeSoon(next);
+  });
 }
-
-/* Check functionality of captureAndStore when there is an OK response
-   from the server.
- */
-function* regularCapGoodResponseUpdateTest() {
-  const URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_update.sjs?ok";
-  yield addTab(URL);
-  yield captureAndCheckColor(0, 255, 0, "we have a green thumbnail");
-  gBrowser.removeTab(gBrowser.selectedTab);
-  // do it again - the server will return a 200, so the foreground service
-  // should  update it.
-  yield addTab(URL);
-  yield captureAndCheckColor(255, 0, 0, "we now  have a red thumbnail");
-}
--- a/toolkit/components/thumbnails/test/thumbnails_update.sjs
+++ b/toolkit/components/thumbnails/test/thumbnails_update.sjs
@@ -1,56 +1,57 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
-// This server-side script is used for browser_thumbnails_update.  One of the
-// main things it must do in all cases is ensure a Cache-Control: no-store
-// header, so the foreground capture doesn't interfere with the testing.
-
-// If the querystring is "simple", then all it does it return some content -
-// it doesn't really matter what that content is.
-
-// Otherwise, its main role is that it must return different *content* for the
-// second request than it did for the first.
-// Also, it should be able to return an error response when requested for the
-// second response.
-// So the basic tests will be to grab the thumbnail, then request it to be
-// grabbed again:
-// * If the second request succeeded, the new thumbnail should exist.
-// * If the second request is an error, the new thumbnail should be ignored.
+// This server-side script is used for browser_thumbnails_update.  It recognizes
+// the following URL parameters:
+//
+// * status=<status>: The returned response will use this status.
+// * nostore: The returned response will have Cache-Control: no-store.  That's
+//   important because the foreground capturer (gBrowserThumbnails) will capture
+//   pages as they load, which can interfere with tests, depending on what they
+//   do.  Setting no-store prevents gBrowserThumbnails from capturing.
+//
+// If no argument is given for the status parameter, then the returned response
+// is more complicated.  In that case it's expected that two requests will be
+// made.  The response to the first request will return a green page, and the
+// response to the second request will return a red page.  Both responses will
+// be 200s.
 
 function handleRequest(aRequest, aResponse) {
   aResponse.setHeader("Content-Type", "text/html;charset=utf-8", false);
-  // we want to disable gBrowserThumbnails on-load capture for these responses,
-  // so set as a "no-store" response.
-  aResponse.setHeader("Cache-Control", "no-store");
+
+  let argPairs = (aRequest.queryString || "").split("&");
+  let args = argPairs.reduce((memo, str) => {
+    let [key, value] = str.split("=");
+    memo[key] = value;
+    return memo;
+  }, {});
 
-  // for the simple test - just return some content.
-  if (aRequest.queryString == "simple") {
-    aResponse.write("<html><body></body></html>");
-    aResponse.setStatusLine(aRequest.httpVersion, 200, "Its simply OK");
+  if ("nostore" in args) {
+    aResponse.setHeader("Cache-Control", "no-store");
+  }
+
+  let status = parseInt(args.status);
+  if (status) {
+    aResponse.setStatusLine(aRequest.httpVersion, status);
     return;
   }
 
   // it's one of the more complex tests where the first request for the given
   // URL must return different content than the second, and possibly an error
   // response for the second
   let doneError = getState(aRequest.queryString);
   if (!doneError) {
     // first request - return a response with a green body and 200 response.
     aResponse.setStatusLine(aRequest.httpVersion, 200, "OK - It's green");
     aResponse.write("<html><body bgcolor=00ff00></body></html>");
     // set the  state so the next request does the "second request" thing below.
     setState(aRequest.queryString, "yep");
   } else {
     // second request - this will be done by the b/g service.
-    // We always return a red background, but depending on the query string we
-    // return either a 200 or 401 response.
-    if (aRequest.queryString == "fail")
-      aResponse.setStatusLine(aRequest.httpVersion, 401, "Oh no you don't");
-    else
-      aResponse.setStatusLine(aRequest.httpVersion, 200, "OK - It's red");
+    aResponse.setStatusLine(aRequest.httpVersion, 200, "OK - It's red");
     aResponse.write("<html><body bgcolor=ff0000></body></html>");
     // reset the error state incase this ends up being reused for the
     // same url and querystring.
     setState(aRequest.queryString, "");
   }
 }