Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

5 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

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

Comment 14

5 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.