Closed Bug 726018 Opened 8 years ago Closed 7 years ago

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

Categories

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

11 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 13
Tracking Status
firefox11 --- affected
firefox12 --- affected
firefox13 --- fixed
firefox20 --- verified
fennec 12+ ---

People

(Reporter: lmandel, Assigned: bnicholson)

References

Details

(Whiteboard: [mtd])

Attachments

(4 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
Assignee: nobody → bnicholson
tracking-fennec: ? → 12+
Priority: -- → P2
Attachment #597229 - Flags: review?(mark.finkle)
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.
Attachment #597229 - Flags: review?(mark.finkle) → review-
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 | ?
Attachment #597230 - Flags: review?(mark.finkle) → review+
(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 on attachment 597229 [details] [diff] [review]
(1/2) add tab load states

TESTS!
Attachment #597229 - Flags: review- → review+
Include dynamic server-side JS files in Robotium.
Attachment #602574 - Flags: review?(gbrown)
Attached patch test case for 404 thumbnails (obsolete) — Splinter Review
thumbnails test case
Attachment #602576 - Flags: review?(gbrown)
Attachment #602574 - Flags: review?(gbrown) → review+
(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.
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...
Attachment #602576 - Attachment is obsolete: true
Attachment #602576 - Flags: review?(gbrown)
Attachment #603007 - Flags: review?(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.
Attachment #603007 - Flags: review?(gbrown) → review+
Duplicate of this bug: 733976
https://hg.mozilla.org/mozilla-central/rev/6c85aa92f4ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 813107
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.