Closed Bug 826639 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

20 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox20 affected, fennec+)

RESOLVED DUPLICATE of bug 859584
Tracking Status
firefox20 --- affected
fennec + ---

People

(Reporter: pretzer, Assigned: wesj)

References

Details

Attachments

(3 files, 3 obsolete files)

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).
Duplicate of this bug: 826659
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #698169 - Flags: review?(lucasr.at.mozilla)
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.
about:home customization seems to be an important feature for MWC, therefore I figured this might be worth tracking...
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 20+
(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.
s/Awesomescreen/Preferences/
(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?
Any progress on a new patch?
Flags: needinfo?(wjohnston)
Attached patch PatchSplinter Review
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?
Attachment #698169 - Attachment is obsolete: true
Attachment #698169 - Flags: review?(lucasr.at.mozilla)
Attachment #715795 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(wjohnston)
ping?
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?
Attached patch Patch 2/2Splinter Review
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.
Attachment #718768 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch 2b/2 (obsolete) — Splinter Review
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.
Attachment #720128 - Flags: review?(lucasr.at.mozilla)
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 on attachment 715795 [details] [diff] [review]
Patch

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

Would we still need this patch if 2b/2 lands?
Flags: needinfo?(wjohnston)
Attached patch Patch 2b/2 (obsolete) — Splinter Review
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).
Attachment #720128 - Attachment is obsolete: true
Attachment #720128 - Flags: review?(lucasr.at.mozilla)
Attachment #727324 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(wjohnston)
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).
Attachment #727324 - Flags: review?(lucasr.at.mozilla) → review-
(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 :-)
Attached patch Patch 2b/2Splinter Review
Attachment #727324 - Attachment is obsolete: true
Attachment #729853 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #729853 - Flags: review?(lucasr.at.mozilla) → review+
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]'
tracking-fennec: 20+ → ?
Wesj - ETA?
tracking-fennec: ? → +
Flags: needinfo?(wjohnston)
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).
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(wjohnston)
Resolution: --- → DUPLICATE
Duplicate of bug: 859584
Attachment #718768 - Flags: review?(lucasr.at.mozilla) → review-
Attachment #715795 - Flags: review?(lucasr.at.mozilla)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.