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

VERIFIED FIXED in Firefox 13

Status

()

Firefox for Android
General
P2
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: lmandel, Assigned: bnicholson)

Tracking

(Depends on: 1 bug)

11 Branch
Firefox 13
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 affected, firefox12 affected, firefox13 fixed, firefox20 verified, fennec12+)

Details

(Whiteboard: [mtd])

Attachments

(4 attachments, 1 obsolete attachment)

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: --- → ?
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
Assignee: nobody → bnicholson
tracking-fennec: ? → 12+
Priority: -- → P2
(Assignee)

Comment 1

6 years ago
Created attachment 597229 [details] [diff] [review]
(1/2) add tab load states
Attachment #597229 - Flags: review?(mark.finkle)
(Assignee)

Comment 2

6 years ago
Created attachment 597230 [details] [diff] [review]
(2/2) update thumbnails only for successful loads
Attachment #597230 - 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+
(Assignee)

Comment 5

6 years ago
(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+
(Assignee)

Comment 7

6 years ago
Created attachment 602574 [details] [diff] [review]
include sjs files in robotium

Include dynamic server-side JS files in Robotium.
Attachment #602574 - Flags: review?(gbrown)
(Assignee)

Comment 8

6 years ago
Created attachment 602576 [details] [diff] [review]
test case for 404 thumbnails

thumbnails test case
Attachment #602576 - Flags: review?(gbrown)
Attachment #602574 - Flags: review?(gbrown) → review+
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
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...
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+
(Assignee)

Comment 12

6 years ago
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
(Assignee)

Comment 13

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

Comment 14

6 years ago
Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/62eecc2b684d
Duplicate of this bug: 733976
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.
Status: NEW → ASSIGNED
status-firefox13: affected → fixed
Target Milestone: --- → Firefox 13
(Assignee)

Comment 17

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/6c85aa92f4ab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
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
status-firefox20: --- → verified
You need to log in before you can comment on or make changes to this bug.