726018 - Don't update top site screenshots when receiving 404, 500, etc results

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

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)]

Attachment: (1/2) add tab load states

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: (2/2) update thumbnails only for successful loads

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

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)]

(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?)]

Comment on attachment 597229 [details] [diff] [review]
(1/2) add tab load states

TESTS!

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: include sjs files in robotium

Include dynamic server-side JS files in Robotium.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: test case for 404 thumbnails

thumbnails test case

Comment 9

[Brian Nicholson (:bnicholson)]

(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)]

Attachment: 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]]

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 12

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa2e0bdbf7f5
http://hg.mozilla.org/integration/mozilla-inbound/rev/17822859e270
http://hg.mozilla.org/integration/mozilla-inbound/rev/5fda501e38b6
http://hg.mozilla.org/integration/mozilla-inbound/rev/7a86dc056fe5

Comment 13

[Brian Nicholson (:bnicholson)]

Backed out test case for failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9894488&tree=Mozilla-Inbound

Comment 14

[Brian Nicholson (:bnicholson)]

Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/62eecc2b684d

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fa2e0bdbf7f5
https://hg.mozilla.org/mozilla-central/rev/17822859e270
https://hg.mozilla.org/mozilla-central/rev/5fda501e38b6

Leaving open for testcase.

Comment 17

[Brian Nicholson (:bnicholson)]

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 (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6c85aa92f4ab

Comment 19

[Cristian Nicolae (:xti)]

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