740556 - reload the mobile homepage on inbound tab syncs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Tracy Walker [:tracy]]

updates to synced tabs don't appear on the mobile homepage unless a manual reload of the page is done.

Perhaps fennec should be doing a timed reload or should sync fire a reload if inbound tabs are detected?

Comment 1

[Richard Newman [:rnewman]]

The Androidy way to do this is for Fennec to register as an observer for the tabs ContentProvider, and for the CP to correctly notify its observers on change.

I believe this is already done for the bookmarks provider.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

not blocking unless someone from product or UX wants to advocate otherwise

Comment 3

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

Margaret, Lucas: Should we make this a "mentor" bug?

Comment 4

[:Margaret Leibovic]

Sure, I can make it into a mentor bug. This probably isn't a good first bug, unless the contributor is already familiar with Android, but I think it's good to start making non-trivial bugs into mentor bugs :)

rnewman was correct in comment 1, we do already register a content observer for bookmarks:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#102

We added an API to BrowserDB/LocalBrowserDB to do this for bookmarks/history:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#568

We'll probably want to add a similar API to TabsAccessor to register an observer for changes to BrowserContract.Tabs.CONTENT_URI.

Comment 5

[Nick Alexander :nalexander [he/him]]

I can help with this too.

Comment 6

[[e-mail]]

I would like to contribute to this bug.

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to [e-mail] from comment #6)
> I would like to contribute to this bug.

Great!  Do you have a Fennec build environment set up?  That's step one.  Can you let me know how the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android work out for you?  This is a frustrating process, but there's lots of people who can help on this part.

Comment 8

[Harshit Syal]

Hi,
I would like to contribute to this bug. I am done with downloading source code and building fennec. And also have successfully tested it on my droid.(Thanks to your detail instructions)
I have chosen this bug coz after solving it I would learn how firefox instant sync works !

Currently I am reading about content providers, sync adapters, Account Authenticator etc, after reading all these I would try to understand how fennec uses all this and provides sync service.

But I am having problems in reconstructing this particular bug.
Fist of all I would like to know on which version of fennec this bug is being encountered? I am using nightly build for reconstructing this bug.

In my version the sync is not working automatically at all. If I create any new tab on my desktop firefox I have to manually use tools->sync now and then in android I have to go to settings-> fennec account -> sync all, then only newly added tabs appear in tab bar of fennec !
But yeah this bug is also being encountered as I have to manually reload homepage in order to refresh tabs.

Please contact me on how I should approach.
IRC: harshit
Mail: [e-mail]

Comment 9

[Richard Newman [:rnewman]]

(In reply to Harshit Syal from comment #8)

> In my version the sync is not working automatically at all. If I create any
> new tab on my desktop firefox I have to manually use tools->sync now and
> then in android I have to go to settings-> fennec account -> sync all, then
> only newly added tabs appear in tab bar of fennec !

Tab changes on desktop aren't usually enough to cause a sync to occur. Syncs happen in the background every few minutes, and when you make major changes (e.g., to prefs or bookmarks).

Tools > Sync Now is perfectly acceptable.

On Android, syncs are even less controllable. They happen when Android thinks they should. We're tweaking that as we go.

The approach you're taking right now is fine.


Take a look at Margaret's Comment 4 for what kind of code changes you need to make.

Comment 10

[Harshit Syal]

I am facing a lot of problems in understanding code base,  
please assign me to this bug and provide me a mentor

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Harshit Syal from comment #10)
> I am facing a lot of problems in understanding code base,  
> please assign me to this bug and provide me a mentor

Hi Harshit,

I think Margaret is pretty busy at the moment, so I can be your point of contact.  Margaret's comment #4 (https://bugzilla.mozilla.org/show_bug.cgi?id=740556#c4) is definitely the place to start.  The code base is quite challenging -- can you suggest how you got started?  I think I would try to figure out how to trigger an about:home refresh (or better, just the synced tabs component) manually first, before trying to tie it to the DB update.  Have you tried doing that?

Comment 12

[Harshit Syal]

Attachment: Partial Patch

There are some unnecessary changes in this patch. 
The changes might not be perfect, Although this patch does solve this bug.
Please review it and tell me if my approach is right.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 701473 [details] [diff] [review]
Partial Patch

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

Good job getting started!

General comments:

* Trailing whitespace throughout. In general, never introduce a new line when you don't need to, and never leave trailing whitespace. It'll show up as red in the review.

* Also, please make sure to follow these instructions:

  https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

to add the appropriate headers and formatting to your patch.

* I think there's stuff in this patch that isn't necessary -- two new menu items, and logic for triggering a sync. The former shouldn't be included at all, and the latter isn't quite the right approach to the problem. So let's stick to redisplaying the UI when the DB changes, and leave the rest for another bug (perhaps Bug 814993, or Bug 726055).

::: mobile/android/base/AboutHomeContent.java
@@ +77,5 @@
>  
>      private static int mNumberOfTopSites;
>      private static int mNumberOfCols;
>  
> +

Please don't introduce unnecessary whitespace.

@@ +145,5 @@
>              public void onClick(View v) {
>                  Tabs.getInstance().loadUrl((String) v.getTag(), Tabs.LOADURL_NEW_TAB);
>              }
>          };
> +        

Please make sure to remove trailing whitespace before preparing a patch for review.

@@ +150,4 @@
>          mPrelimPromoBoxType = (new Random()).nextFloat() < 0.5 ? AboutHomePromoBox.Type.SYNC :
>                  AboutHomePromoBox.Type.APPS;
> +        
> +       

Same.

@@ +207,5 @@
>          if (mAccountListener != null) {
>              mAccountManager.removeOnAccountsUpdatedListener(mAccountListener);
>              mAccountListener = null;
>          }
> +       

Same.

::: mobile/android/base/BrowserApp.java
@@ +66,5 @@
>      private Boolean mAboutHomeShowing = null;
>      protected Telemetry.Timer mAboutHomeStartupTimer = null;
> +    
> +    private ContentResolver mContentResolver;
> +    private ContentObserver mContentObserver;

I don't like keeping this much state around in this object.

Use getContentResolver() directly instead of persisting the value -- this is a property of the Context, and we already rely on it not changing.

@@ +693,5 @@
> +        mContentObserver = new ContentObserver(null) {
> +            public void onChange(boolean selfChange) {
> +            	
> +            	 mAboutHomeContent.update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
> +            	 mAboutHomeContent.update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));

Indenting.

@@ +695,5 @@
> +            	
> +            	 mAboutHomeContent.update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
> +            	 mAboutHomeContent.update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
> +
> +            	 Log.d(LOGTAG,"About:home, Content observer trigered ! !");

Spelling and unnecessary punctuation… but this can probably be deleted.

@@ +698,5 @@
> +
> +            	 Log.d(LOGTAG,"About:home, Content observer trigered ! !");
> +            }
> +        };
> +        BrowserDB.registerRemoteTabsObserver(mContentResolver, mContentObserver);

Indenting.

@@ +709,5 @@
>          // to create an AboutHomeRunnable to hide the about:home content.
>          if (mAboutHomeShowing != null && !mAboutHomeShowing)
>              return;
>  
> +        BrowserDB.unregisterContentObserver(mContentResolver, mContentObserver);

Ugh, this API sucks. There's no reason why BrowserDB should be involved here at all: that call is defined as
  mContentResolver.unregisterContentObserver(mContentObserver);

so without having error handling or somesuch, there's no point in it existing.

@@ +1124,5 @@
>              case R.id.new_private_tab:
>                  Tabs.getInstance().loadUrl("about:home", Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE);
>                  return true;
> +            case R.id.refresh_chrome:
> +            	 mAboutHomeContent.update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));

Can you explain why this is necessary?

@@ +1131,5 @@
> +            	
> +            	
> +            	Account[] account = org.mozilla.gecko.sync.setup.SyncAccounts.syncAccounts(getApplicationContext());
> +
> +            	SyncAdapter.requestImmediateSync(account[0], new String[] { "tabs" });

This will misbehave if Sync isn't set up, or if there's more than one account. It's also unnecessary, if I'm not mistaken: Android *already* triggers a sync in this case, because Sync is sensitive to DB activity; Sync just ignores it because it's already synced recently, and isn't smart enough to realize that it might want to do a "mini sync".

The right fix is for Sync to actually perform a sync when Fennec starts watching the DB, rather than backing off.

::: mobile/android/base/resources/menu/gecko_app_menu.xml
@@ +8,5 @@
>            android:icon="@drawable/ic_menu_quit"
>            android:title="@string/quit"
>            android:orderInCategory="10" />
> +        <item android:id="@+id/refresh_chrome"
> +          android:title="Refresh chrome"   />

Don't do this, but also menu strings need to be localized.

@@ +10,5 @@
>            android:orderInCategory="10" />
> +        <item android:id="@+id/refresh_chrome"
> +          android:title="Refresh chrome"   />
> +             <item android:id="@+id/sync_now"
> +          android:title="Sync now"   />

Don't do this at all. Sync is supposed to be a background service, and this functionality is already exposed in Settings.

Comment 14

[Nick Alexander :nalexander [he/him]]

Thanks, rnewman, for jumping in, but be aware that I asked Harshit to submit this so I could see his WIP.  A lot of the UI stuff is so that we can test what's happening, and won't be final.  I'll take a further look at this tonight or tomorrow.

Comment 15

[Harshit Syal]

Attachment: Patch showing problem in TabsAcesssor.getTabs()

Patch for review, 
Steps to find problem in TabsAccessor.getTabs():
1.) please use adb logcat -s GeckoBrowserApp:d, As I am using android style logs.
2.) make some changes in desktop tabs, sync now on desktop firefox
3.) open fennec, go to settings then sync now fennec.
4.) during sync "sometimes" you'll observe that no of tabs returned is 0, even when they are supposed to be something else.

please notice "sometimes" because this doesn't happen always !

Comment 16

[Nick Alexander :nalexander [he/him]]

> 4.) during sync "sometimes" you'll observe that no of tabs returned is 0,
> even when they are supposed to be something else.

I figured out what was causing this problem.  In reality, you should *always* see at least one 0 returned.  What happens is that:

- Sync deletes all tabs
- TabsProvider.delete calls notifyChange
- The ContentObserver sees 0 tabs (unless timing errors delay this)
- Sync bulkInserts all tabs
- TabsProvider.bulkInsert *does not call* notifyChange.

The patch at the end of this comment fixes TabsProvider.java.in to actually call notifyChange.

Long term, we need to fix *all* bulkInsert calls to call notifyChange correctly.  But this should get you closer to addressing this ticket.

Also, there are two places that will need ContentObservers:

- the AboutHomeContent.mRemoteTabs AboutHomeSection
- the RemoteTabs PanelView in RemoteTabs.java (this is the SYNCED section of the TabsPanel that you see when you hit the "tabs arrow"; the other sections are TABS and PRIVATE).

I think we should register the observer for the AboutHomeSection in AboutHomeContent.init, probably after

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutHomeContent.java#149

I think we should register the observer for the RemoteTabs PanelView in RemoteTabs.show() and remove it in RemoteTabs.hide().

Any way, enough for now.  Here's what to do to make the new tabs generate an update:

diff --git a/mobile/android/base/db/TabsProvider.java.in b/mobile/android/base/db/TabsProvider.java.in
--- a/mobile/android/base/db/TabsProvider.java.in
+++ b/mobile/android/base/db/TabsProvider.java.in
@@ -641,11 +641,14 @@ public class TabsProvider extends Conten
                 }
             }
             trace("Flushing DB bulkinsert...");
             db.setTransactionSuccessful();
         } finally {
             db.endTransaction();
         }
 
+        if (successes > 0)
+            getContext().getContentResolver().notifyChange(uri, null);
+
         return successes;
     }
 }

Comment 17

[Harshit Syal]

Attachment: Final Patch, solves this bug

This patch solves the problem in about:home remote tabs component perfectly..
I was told to add content observer in RemoteTabs also but doing that crashes fennec, so I didn't do that.

For now I think this should be merged !

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 702759 [details] [diff] [review]
Final Patch, solves this bug

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

This is looking really good!  Several nits (that are easy to address) mean we can't land this as is, but after the dependent bug lands, we can get this in.  Thanks, Harshit!

::: mobile/android/base/AboutHomeContent.java
@@ -65,5 @@
> -import java.util.Map;
> -import java.util.Random;
> -import java.util.zip.ZipEntry;
> -import java.util.zip.ZipFile;
> -

This kind of re-ordering makes it really hard to see what has changed.  Please only add/remove the single lines your patch needs.

@@ +90,5 @@
>      private BrowserApp mActivity;
>      UriLoadCallback mUriLoadCallback = null;
>      VoidCallback mLoadCompleteCallback = null;
>      private LayoutInflater mInflater;
> +    private ContentObserver mContentObserver;

Initialize to null here.

@@ +145,5 @@
>              public void onAccountsUpdated(Account[] accounts) {
>                  updateLayoutForSync();
>              }
>          }, GeckoAppShell.getHandler(), false);
> +        // Bug 740556 - reload the mobile homepage on inbound tab syncs

nit: newline before.

@@ +148,5 @@
>          }, GeckoAppShell.getHandler(), false);
> +        // Bug 740556 - reload the mobile homepage on inbound tab syncs
> +        // Adding content observer for remote tabs component
> +        mContentObserver = new ContentObserver(null) {
> +        	public void onChange(boolean selfChange) {

nit: we indent 4 spaces per level.  This is 8, or perhaps a hard tab character.

@@ +150,5 @@
> +        // Adding content observer for remote tabs component
> +        mContentObserver = new ContentObserver(null) {
> +        	public void onChange(boolean selfChange) {
> +        		update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
> +        		Log.d(LOGTAG,"About:home, Content observer triggered ! !");

We don't log without checking log levels, but just remove this log entirely.

@@ +153,5 @@
> +        		update(EnumSet.of(AboutHomeContent.UpdateFlags.REMOTE_TABS));
> +        		Log.d(LOGTAG,"About:home, Content observer triggered ! !");
> +        	}
> +        };
> +        BrowserDB.registerRemoteTabsObserver(mActivity.getContentResolver(), mContentObserver);

nit: newline after.

I think we can register the observer directly here, rather than having this BrowserDB static interface:

|mActivity.getContentResolver().registerObserver(...)|

Or we can add a static interface TabsProvider.registerTabsObserver.  (Note that the observer is for all tabs, not just remote tabs.  It's based on the URI, not the query itself.)  I like that more, actually.  Then it's a little easier to track where things are registered.

@@ +256,5 @@
>              Cursor cursor = mTopSitesAdapter.getCursor();
>              if (cursor != null && !cursor.isClosed())
>                  cursor.close();
>          }
> +        if(mContentObserver != null) {

nit: newline before.
nit: space between if and (.

@@ +257,5 @@
>              if (cursor != null && !cursor.isClosed())
>                  cursor.close();
>          }
> +        if(mContentObserver != null) {
> +        	mActivity.getContentResolver().unregisterContentObserver(mContentObserver);

nit: 4 space tab.

Let's set |mContentObserver = null;| here.

::: mobile/android/base/db/BrowserDB.java
@@ +98,5 @@
>          public void registerBookmarkObserver(ContentResolver cr, ContentObserver observer);
>  
>          public void registerHistoryObserver(ContentResolver cr, ContentObserver observer);
> +        
> +        public void registerRemoteTabsObserver(ContentResolver cr, ContentObserver observer);

It's not at all obvious, but |BrowserDB| manages only the History and Bookmarks databases.  So this really doesn't make sense here.  Also, it just passes through to |LocalBrowserDB|.  I'll suggest a place where you can just use the ContentObserver interface directly.

@@ +253,5 @@
>  
>      public static void registerHistoryObserver(ContentResolver cr, ContentObserver observer) {
>          sDb.registerHistoryObserver(cr, observer);
>      }
> +    

Delete this too.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +642,5 @@
>  
>      public void registerHistoryObserver(ContentResolver cr, ContentObserver observer) {
>          cr.registerContentObserver(mHistoryUriWithProfile, false, observer);
>      }
> +    

As earlier, this is not in the correct place.  (Notice how other register calls are using member variables to get URIs, but your URI is static constant.)

::: mobile/android/base/db/TabsProvider.java.in
@@ +644,5 @@
>              db.setTransactionSuccessful();
>          } finally {
>              db.endTransaction();
>          }
> +        if (successes > 0)

nit: newline before, and 4 space tab.  Keep this for your testing, but we need to fix this bug for all the content providers, which is Bug 830884.  I'm working on fixing that this afternoon.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Harshit Syal from comment #17)
> Created attachment 702759 [details] [diff] [review]
> Final Patch, solves this bug
> 
> This patch solves the problem in about:home remote tabs component perfectly..
> I was told to add content observer in RemoteTabs also but doing that crashes
> fennec, so I didn't do that.

Yeah, I saw this too.  What is happening is that your register method expects the LocalBrowserDB to be initialized first.  This isn't necessary -- we can register directly with the content resolver -- so we should be able to make this happen.

I'm going to ask for UX input.  ibarlow, should we automatically update the SYNCED section of the tabs panel?  I expect users to only have that open for a short time before making a choice, so refreshing could be annoying if your rows suddenly change.  On the other hand, I don't expect a tabs refresh to happen while the SYNCED section is open very often.

Comment 20

[Richard Newman [:rnewman]]

Might I suggest pull to refresh, or similar? 

And won't write get the ping to sync when the awesome bar appears, which will be before the synced tabs section displays?

Comment 21

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #20)
> Might I suggest pull to refresh, or similar? 

We could do this, I suppose, but I don't think we use pull to refresh anywhere else in the Android UI.  ibarlow?

> And won't write get the ping to sync when the awesome bar appears, which
> will be before the synced tabs section displays?

I don't understand this.

Comment 22

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #21)
> > And won't write get the ping to sync when the awesome bar appears, which
> > will be before the synced tabs section displays?
> 
> I don't understand this.

Sorry, I wrote "awesome bar" when I meant "tab bar"

Current Nightly and Aurora has Synced Tabs in a section in the tab "+", but it opens on "Tabs" (local). We could trigger a background tab sync when the tab view opens.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 702759 [details] [diff] [review]
Final Patch, solves this bug

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

Drive by!

::: mobile/android/base/AboutHomeContent.java
@@ -65,5 @@
> -import java.util.Map;
> -import java.util.Random;
> -import java.util.zip.ZipEntry;
> -import java.util.zip.ZipFile;
> -

Also the import ordering should follow our coding style guidelines at https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices

Comment 24

[Harshit Syal]

Attachment: Patch v2

Made very few changes, tried to address all the problems in previous patch.

Here I have removed involvement of LocalBrowser and BrowserDb, and have confined changes to only AboutHomeContent.

Comment 25

[Harshit Syal]

Attachment: Patch v3

Made very few changes, tried to address all the problems in previous patch.

Here I have removed involvement of LocalBrowser and BrowserDb, and have confined changes to only AboutHomeContent.

Comment 26

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-i

This is a slightly stream-lined version of Harshit's patch.  Depends on the patch for Bug 830884.  Harshit, can you take a look at this and see if you disagree with any of my changes?  Thanks!

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 704012 [details] [diff] [review]
Patch against m-i

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

::: mobile/android/base/AboutHomeContent.java
@@ +149,5 @@
>                  updateLayoutForSync();
>              }
>          }, GeckoAppShell.getHandler(), false);
> +        
> +        // Bug 740556 - reload the mobile homepage on inbound tab syncs

We don't need the bug number listed here, but the other comments seem fine.

Comment 28

[Harshit Syal]

Thanks nick for making necessary changes. For me, all the changes you made looks fine.

Comment 29

[Nick Alexander :nalexander [he/him]]

Attachment: Updated patch against m-i

Updated comment to remove bug number.

Comment 30

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc97e5b91d5c

Thanks for your great work on this ticket, Harshit!

Comment 31

[Nick Alexander :nalexander [he/him]]

STR:

1) install Fennec
2) set up Sync
3) go to about:home
4) verify remote tabs exist
5) change tab configuration on remote device
6) force remote sync
7) go to Settings > Accounts & sync and force a local sync
8) return to Fennec about:home, using "task manager" rather than clicking on App icon
9) verify that remote tabs exist and *are not* updated
10) force reload of about:home, possibly by killing Fennec process
11) verify that remote tabs exist and are updated

After Bug 830884 and 740556:

9) verify that remote tabs exist and *are* updated

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc97e5b91d5c