Last Comment Bug 726018 - Don't update top site screenshots when receiving 404, 500, etc results
: Don't update top site screenshots when receiving 404, 500, etc results
Status: VERIFIED FIXED
[mtd]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 11 Branch
: ARM Android
: P2 normal (vote)
: Firefox 13
Assigned To: Brian Nicholson (:bnicholson)
:
:
Mentors:
: 733976 (view as bug list)
Depends on: 813107
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-10 08:24 PST by Lawrence Mandel [:lmandel] (use needinfo)
Modified: 2016-07-29 14:22 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed
verified
12+


Attachments
(1/2) add tab load states (12.90 KB, patch)
2012-02-14 16:04 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
(2/2) update thumbnails only for successful loads (6.28 KB, patch)
2012-02-14 16:06 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
include sjs files in robotium (792 bytes, patch)
2012-03-02 21:59 PST, Brian Nicholson (:bnicholson)
gbrown: review+
Details | Diff | Splinter Review
test case for 404 thumbnails (5.08 KB, patch)
2012-03-02 22:14 PST, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
test case for 404 thumbnails, v2 (5.19 KB, patch)
2012-03-05 12:07 PST, Brian Nicholson (:bnicholson)
gbrown: review+
Details | Diff | Splinter Review

Description Lawrence Mandel [:lmandel] (use needinfo) 2012-02-10 08:24:58 PST
The screenshots displayed for top sites in about:home refresh on page load. If the page is unsuccessful in loading (server timeout, connection reset) or the browser receives a 404 or 500 series response the screenshot will be updated as well. This leads to a poor experience on about:home. In the server timeout case I see a result that shows a screenshot of the server not found page with the caption "Problem loading page".

I think that it is preferable in the cases above to not update the screenshot but rather to rely on the screenshot from the last successful page load.
Comment 1 Brian Nicholson (:bnicholson) 2012-02-14 16:04:37 PST
Created attachment 597229 [details] [diff] [review]
(1/2) add tab load states
Comment 2 Brian Nicholson (:bnicholson) 2012-02-14 16:06:35 PST
Created attachment 597230 [details] [diff] [review]
(2/2) update thumbnails only for successful loads
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-14 16:24:37 PST
Comment on attachment 597229 [details] [diff] [review]
(1/2) add tab load states

>diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java
>+    private int mLoadState;

Let's hope we don't have several state flags and rename this to mState

>+    public static final int LOAD_STATE_ZOMBIE = 0;
>+    public static final int LOAD_STATE_LOADING = 1;
>+    public static final int LOAD_STATE_LOADED = 2;

Let's drop the "LOAD_" prefix and change ZOMBIE to DELAYED

Also, let's change Tab.getLoadState() to Tab.getState()

>diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java

>             } else if (event.equals("Tab:Added")) {
>                 Log.i(LOGTAG, "Received message from Gecko: " + SystemClock.uptimeMillis() + " - Tab:Added");
>                 Tab tab = addTab(message);
>                 if (message.getBoolean("selected"))
>                     selectTab(tab.getId());
>                 if (message.getBoolean("delayLoad"))
>-                    tab.setHasLoaded(false);
>+                    tab.setLoadState(Tab.LOAD_STATE_ZOMBIE);

There is a slight difference when moving from two flags to one. We need to be sure that we do not set STATE_LOADED on a Tab that we made STATE_DELAYED because the about:blank page loaded into the <browser> on the Gecko side causes a handleDocumentStop.

r- until you confirm with me that a delayed Tab is not marked LOADED by accident. I'll r+ after we get confirmation.

Also, I would love a test, maybe added to testNewTab.java.in that checks the states, at least LOADING and LOADED.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-14 16:32:41 PST
Comment on attachment 597230 [details] [diff] [review]
(2/2) update thumbnails only for successful loads


>     public static final int LOAD_STATE_ZOMBIE = 0;
>     public static final int LOAD_STATE_LOADING = 1;
>-    public static final int LOAD_STATE_LOADED = 2;
>+    public static final int LOAD_STATE_SUCCESS = 2;
>+    public static final int LOAD_STATE_ERROR = 3;

STATE_SUCCESS and STATE_ERROR

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      // true if the page loaded successfully (i.e., no 404s or other errors)
>+      let success = false; 
>+      try {
>+        success = aRequest.QueryInterface(Components.interfaces.nsIHttpChannel).requestSucceeded;
>+      } catch (e) { }

I wonder if we should default to | true | ?
Comment 5 Brian Nicholson (:bnicholson) 2012-02-14 18:02:10 PST
(In reply to Mark Finkle (:mfinkle) from comment #3)
> There is a slight difference when moving from two flags to one. We need to
> be sure that we do not set STATE_LOADED on a Tab that we made STATE_DELAYED
> because the about:blank page loaded into the <browser> on the Gecko side
> causes a handleDocumentStop.
> 
> r- until you confirm with me that a delayed Tab is not marked LOADED by
> accident. I'll r+ after we get confirmation.
> 
> Also, I would love a test, maybe added to testNewTab.java.in that checks the
> states, at least LOADING and LOADED.

We aren't loading any URI at all in the delayed tabs (including about:blank), so those tabs won't be producing any document stop events.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-14 18:07:09 PST
Comment on attachment 597229 [details] [diff] [review]
(1/2) add tab load states

TESTS!
Comment 7 Brian Nicholson (:bnicholson) 2012-03-02 21:59:40 PST
Created attachment 602574 [details] [diff] [review]
include sjs files in robotium

Include dynamic server-side JS files in Robotium.
Comment 8 Brian Nicholson (:bnicholson) 2012-03-02 22:14:23 PST
Created attachment 602576 [details] [diff] [review]
test case for 404 thumbnails

thumbnails test case
Comment 9 Brian Nicholson (:bnicholson) 2012-03-05 11:34:01 PST
(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Created attachment 602576 [details] [diff] [review]
> test case for 404 thumbnails
> 
> thumbnails test case

Oops, I just noticed that this line:
+        mAsserter.is(getTopSiteThumbnailColor(site2Title), Color.RED, "Top site thumbnail not updated for HTTP 404");

should be Color,GREEN, not Color.RED. I'll fix that along with any other comments you have.
Comment 10 Brian Nicholson (:bnicholson) 2012-03-05 12:07:04 PST
Created attachment 603007 [details] [diff] [review]
test case for 404 thumbnails, v2

Added sleeps between loadUrl() calls to make sure a screenshot gets taken. Fixed the typo from comment 9. I'm not sure how the first version passed when I ran it...
Comment 11 Geoff Brown [:gbrown] 2012-03-05 14:09:39 PST
Comment on attachment 603007 [details] [diff] [review]
test case for 404 thumbnails, v2

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

An ingenious test - very cool. r+ with just a couple of nits addressed.

::: mobile/android/base/tests/testThumbnails.java.in
@@ +10,5 @@
> +import android.view.ViewGroup;
> +import android.widget.ImageView;
> +import android.widget.TextView;
> +
> +public class testThumbnails extends BaseTest {

nit - add a brief comment here to explain the purpose of this test. see testLoad for an example / style.

@@ +30,5 @@
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();
> +
> +        // load sites; both will return HTTP 200 with a green background
> +        loadUrl(site1Url);
> +        mSolo.sleep(3000);

Add a comment to explain why the sleep's are necessary and why you chose this amount of time.
Comment 13 Brian Nicholson (:bnicholson) 2012-03-07 16:56:37 PST
Backed out test case for failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9894488&tree=Mozilla-Inbound
Comment 14 Brian Nicholson (:bnicholson) 2012-03-07 17:08:19 PST
Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/62eecc2b684d
Comment 15 Kevin Brosnan [:kbrosnan] 2012-03-07 18:32:06 PST
*** Bug 733976 has been marked as a duplicate of this bug. ***
Comment 17 Brian Nicholson (:bnicholson) 2012-09-19 15:20:29 PDT
This appears to be passing try now: https://tbpl.mozilla.org/?tree=Try&rev=fe898bf01a4d

Pushed test case: http://hg.mozilla.org/integration/mozilla-inbound/rev/6c85aa92f4ab
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-09-20 04:51:13 PDT
https://hg.mozilla.org/mozilla-central/rev/6c85aa92f4ab
Comment 19 Cristian Nicolae (:xti) 2012-11-20 06:32:40 PST
Screenshots are not updated to Top Site for failed page loads on the latest Nightly. Closing bug as verified fixed on:

Firefox 20.0a1 (2012-11-20)
Device: Galaxy S2
OS: Android 4.0.3

Note You need to log in before you can comment on or make changes to this bug.