826639 - about:home shows Firefox logo instead of 'plus' icon in empty top sites fields after clearing private data

Status: RESOLVED DUPLICATE of bug 859584

(Firefox for Android Graveyard :: General, defect)

Description

[Peter Retzer (:pretzer)]

Bug 783312 gave empty top sites fields on about:home a plus icon in the center to indicate that a field is empty and can be customized. 
After clearing private data the formerly occupied fields should show that plus icon to indicate that they are now empty (except the two default top sites entries that always stay, SUMO and AMO). 
Instead the Firefox logo is shown in the center (which is the default icon for top sites without a thumbnail).

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Not in love with this patch, but trying to think of something better.

The issue here is that our thumbnail database query actually returns before we create the Views for the TopSitesGrid in TopSiteCursorAdapter. To fix it I use the cursor (which should always be correct) to determine what to show instead.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 698169 [details] [diff] [review]
Patch v1

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

Are you sure this is the cause of the bug? The updates to the view are done in the UI thread. Same for the thumbnails update. It looks suspicious that thumbnails are updated before the gridview has updates all item views. Maybe there's some adapter change happening off UI thread somehow? Just waiting for a confirmation before giving r+.

::: mobile/android/base/AboutHomeContent.java
@@ +408,2 @@
>              }
> +            view.invalidate();

How's this related?

@@ +898,5 @@
>  
>              // Force the view to fit inside this slot in the grid
>              convertView.setLayoutParams(new AbsListView.LayoutParams(mTopSitesGrid.getColumnWidth(),
>                          Math.round(mTopSitesGrid.getColumnWidth()*ThumbnailHelper.THUMBNAIL_ASPECT_RATIO)));
> +            convertView.invalidate();

Ditto.

Comment 4

[Peter Retzer (:pretzer)]

about:home customization seems to be an important feature for MWC, therefore I figured this might be worth tracking...

Comment 5

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Are you sure this is the cause of the bug? The updates to the view are done
> in the UI thread. Same for the thumbnails update. It looks suspicious that
> thumbnails are updated before the gridview has updates all item views. Maybe
> there's some adapter change happening off UI thread somehow? Just waiting
> for a confirmation before giving r+.

Ahh. Figured this out. We're calling this from the Awesomescreen in this case, (i.e. it calls clear, which fires a notification back to about:home eventually to refresh itself. about:home gets a new cursor and new thumbnails, but since its not actually visible it doesn't create the views to show them until later.

Comment 6

[Wesley Johnston (:wesj)]

s/Awesomescreen/Preferences/

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Are you sure this is the cause of the bug? The updates to the view are done
> > in the UI thread. Same for the thumbnails update. It looks suspicious that
> > thumbnails are updated before the gridview has updates all item views. Maybe
> > there's some adapter change happening off UI thread somehow? Just waiting
> > for a confirmation before giving r+.
> 
> Ahh. Figured this out. We're calling this from the Awesomescreen in this
> case, (i.e. it calls clear, which fires a notification back to about:home
> eventually to refresh itself. about:home gets a new cursor and new
> thumbnails, but since its not actually visible it doesn't create the views
> to show them until later.

Then I guess the patch should be much simpler, no?

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Any progress on a new patch?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch

This is basically the same thing, slimmed down, and with some fixes from the recent overdraw stuff that seems like they were left out. I'm not sure what you mean by simpler? Do you have a better solution?

Better in my mind might be to delay getting these until the views are created/shown, but thats a larger refactor?

I could add this in an overriden setVisibility function instead (if I assume that I still run after the views are created?) If we move this to a fragment, updates can be delayed and batched when we're paused?

Comment 10

[Wesley Johnston (:wesj)]

ping?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 715795 [details] [diff] [review]
Patch

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

Feeling a bit mixed about this patch. It makes the update behaviour a bit less obvious. Wouldn't it make more sense to avoid any thumbnail related updates if about:home is refreshed while is not actually visible? Then refresh thumbnails once the about:home is visible again?

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This only updates the views if/when AboutHomeContent is visible. I used setVisibility() to control that, but I'm happy to hear if there are other ways. There is some strangeness when about:home is actually "visible" but the app is in the background, so I also had to update about:home in onResume().

I've been digging a bit and haven't seen any other better ways to know if a View is onScreen or not. If you know something, I'm happy to hear.

I'm leaving up both reviews, because I still think the first patch should go in, regardless of this one.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 2b/2

I looked into the gridview to see when/why its smart enough to not build views when not necessary. Basically, it only creates view when layout or dispatchDraw is called on them, which goes through some acrobatics to make sure the view is visible.

This is an alternative that does a similar thing here. Basically we queue ALL updates, and only process them when we get a dispatchDraw call on AboutHomeContent.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 720128 [details] [diff] [review]
Patch 2b/2

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

This one looks more solid to me. I'd just like to know if we're not blocking UI thread in any way with this change before giving r+

::: mobile/android/base/AboutHomeContent.java
@@ +448,5 @@
>          }).execute();
>      }
>  
>      void update(final EnumSet<UpdateFlags> flags) {
> +        synchronized (mPendingUpdates) {

Won't this potentially block the caller until processPendingUpdates() finishes? Are all update() calls made off main thread?

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 715795 [details] [diff] [review]
Patch

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

Would we still need this patch if 2b/2 lands?

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 2b/2

Good point. This uses a copy of the list instead (I've never been able to find good docs about using a background thread within a synchronized block, but I think this is fine?)

I moved some of the background color fixes (which we need regardless) into here as well, so I don't think we need 1 with 2b (but I still think 1 should go in).

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 727324 [details] [diff] [review]
Patch 2b/2

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

I'd like to see a final version of this patch before giving r+

::: mobile/android/base/AboutHomeContent.java
@@ +462,5 @@
>  
> +    void processPendingUpdates() {
> +        // during startup, this may be called before init()
> +        if (mPendingUpdates.isEmpty())
> +            return;

Shouldn't this if be inside the synchronized block?

@@ +467,4 @@
>  
> +        synchronized (mPendingUpdates) {
> +            final EnumSet<UpdateFlags> copiedUpdates = mPendingUpdates.clone();
> +            mPendingUpdates = EnumSet.noneOf(UpdateFlags.class);

I think the synchronized block should not contain the background thread bits as you're copying the contents of mPendingUpdates to final local variable (which is enough).

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #16)
> Created attachment 727324 [details] [diff] [review]
> I moved some of the background color fixes (which we need regardless) into
> here as well, so I don't think we need 1 with 2b (but I still think 1 should
> go in).

I'd prefer to avoid landing 1 if you can confirm that 2b is enough to fix this bug. However, if you know there are some edge cases that patch 1 covers, I'm fine with landing it.

In other words, I'd like to avoid landing 1 on a "just in case" basis :-)

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch 2b/2

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 729853 [details] [diff] [review]
Patch 2b/2

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

Looks good to me.

::: mobile/android/base/AboutHomeContent.java
@@ +461,5 @@
> +    }
> +
> +    void processPendingUpdates() {
> +        // during startup, this may be called before init()
> +        if (mPendingUpdates.isEmpty())

Doesn't this need to be inside the synchronized block?

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a57e0f94616

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Something in this push was causing robocop failures and talos crashes. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf175669197

https://tbpl.mozilla.org/php/getParsedLog.php?id=21356944&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testHistoryTab
2 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_big_link.html should equal http://mochi.test:8888/tests/robocop/robocop_big_link.html
3 INFO TEST-PASS | testHistoryTab | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_01.html
waitForText timeout on Settings
4 INFO TEST-PASS | testHistoryTab | bookmark added successfully - true should equal true
5 INFO TEST-PASS | testHistoryTab | checking that history list exists and has 3 children - android.widget.ExpandableListView@48684fb0 should not equal null
6 INFO TEST-PASS | testHistoryTab | TextView is filled in - Big Link
7 INFO TEST-PASS | testHistoryTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_big_link.html
8 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 1 should equal 1
9 INFO TEST-PASS | testHistoryTab | TextView is filled in - Browser Blank Page 01
10 INFO TEST-PASS | testHistoryTab | TextView is filled in - http://mochi.test:8888/tests/robocop/robocop_blank_01.html
11 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 2 should equal 2
12 INFO TEST-PASS | testHistoryTab | First row has Today header - Today
13 INFO TEST-PASS | testHistoryTab | Correct number of ImageViews visible - 0 should equal 0
INFO | automation.py | Application ran for: 0:01:36.113976
INFO | zombiecheck | Reading PID log: /tmp/tmp1lAcDrpidlog
PROCESS-CRASH | java-exception | java.lang.NullPointerException	at org.mozilla.gecko.AllPagesTab.setSearchEngines(AllPagesTab.java:625)

https://tbpl.mozilla.org/php/getParsedLog.php?id=21357237&tree=Mozilla-Inbound

Failed tcheckerboard: 
		Stopped Tue, 02 Apr 2013 12:30:53
    talos_results.add(mytest.runTest(browser_config, test))
  File "/builds/tegra-158/talos-data/talos/ttest.py", line 397, in runTest
    test_results.add(browser_log_filename, counter_results=counter_results)
  File "/builds/tegra-158/talos-data/talos/results.py", line 128, in add
    browserLog = BrowserLogResults(filename=results, counter_results=counter_results, global_counters=self.global_counters)
  File "/builds/tegra-158/talos-data/talos/results.py", line 324, in __init__
    self.error(match.group(1))
  File "/builds/tegra-158/talos-data/talos/results.py", line 339, in error
    raise utils.talosError(message)
talosError: 'browser non-zero return code (1) [browser_output.txt]'

Comment 23

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

Wesj - ETA?

Comment 24

[Brian Nicholson (:bnicholson)]

Talking to Wes, he said we can go with bug 859584 as long as it fixes the problems described here (which it does from my testing).