Last Comment Bug 849072 - Improve tab flushing and syncing behavior
: Improve tab flushing and syncing behavior
Status: RESOLVED FIXED
[good first bug][lang=java][mentor=na...
:
Product: Firefox for Android
Classification: Client Software
Component: Data Providers (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 24
Assigned To: Jamie Hewland
:
Mentors:
Depends on: 780279 730039
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-07 17:34 PST by Richard Newman [:rnewman]
Modified: 2013-05-20 13:28 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed all scoring code and changed to implement a timeout (6.45 KB, patch)
2013-04-18 05:21 PDT, justin.busby
no flags Details | Diff | Review
Removed redundant import spotted after online diff checked (6.42 KB, patch)
2013-04-18 05:23 PDT, justin.busby
rnewman: review-
Details | Diff | Review
Use the Android Handler to send a single delayed message to persist tabs (5.74 KB, patch)
2013-05-04 03:25 PDT, Jamie Hewland
rnewman: review+
nalexander: feedback+
Details | Diff | Review
Use the Android Handler to send a single delayed message to persist tabs (nits fixed) (6.00 KB, patch)
2013-05-16 14:46 PDT, Jamie Hewland
nalexander: review+
Details | Diff | Review

Description Richard Newman [:rnewman] 2013-03-07 17:34:08 PST
Right now Fennec uses a scoring algorithm to decide when to flush its open tabs to the tabs data provider. This essentially guarantees that Fennec's database of tabs is never the actual tab state. See Bug 730039 Comment 46.

We should implement some kind of idle timer to flush the current tab state, rather than using a scoring system. (By all means extend the tab system to do incremental writes, too.)

After the flush, Fennec can notify Sync that tab state has changed. See Bug 780279. This might even be done automatically by Android. Tada!

There might be a small amount of Sync-side work to make sure we don't sync too frequently, or ensure that we do sync.

Both halves of this are [good second bug], which I call a fun [good first bug] if mentoring is adequate. Nick's a great mentor, so here we go!
Comment 1 Daniel 2013-03-12 11:37:51 PDT
(In reply to Richard Newman [:rnewman] from comment #0)
> Right now Fennec uses a scoring algorithm to decide when to flush its open
> tabs to the tabs data provider. This essentially guarantees that Fennec's
> database of tabs is never the actual tab state. See Bug 730039 Comment 46.
> 
> We should implement some kind of idle timer to flush the current tab state,
> rather than using a scoring system. (By all means extend the tab system to
> do incremental writes, too.)
> 
> After the flush, Fennec can notify Sync that tab state has changed. See Bug
> 780279. This might even be done automatically by Android. Tada!
> 
> There might be a small amount of Sync-side work to make sure we don't sync
> too frequently, or ensure that we do sync.
> 
> Both halves of this are [good second bug], which I call a fun [good first
> bug] if mentoring is adequate. Nick's a great mentor, so here we go!

Hello,
I am trying to find a mentored bug to work on. I have just recently learned some of the basics of java in school. Please let me know what I need to do to move forward. I am willing to contribute in any way I can :)
Comment 2 Nick Alexander :nalexander 2013-03-12 12:33:03 PDT
(In reply to Daniel from comment #1)
> (In reply to Richard Newman [:rnewman] from comment #0)
> > Right now Fennec uses a scoring algorithm to decide when to flush its open
> > tabs to the tabs data provider. This essentially guarantees that Fennec's
> > database of tabs is never the actual tab state. See Bug 730039 Comment 46.
> > 
> > We should implement some kind of idle timer to flush the current tab state,
> > rather than using a scoring system. (By all means extend the tab system to
> > do incremental writes, too.)
> > 
> > After the flush, Fennec can notify Sync that tab state has changed. See Bug
> > 780279. This might even be done automatically by Android. Tada!
> > 
> > There might be a small amount of Sync-side work to make sure we don't sync
> > too frequently, or ensure that we do sync.
> > 
> > Both halves of this are [good second bug], which I call a fun [good first
> > bug] if mentoring is adequate. Nick's a great mentor, so here we go!
> 
> Hello,
> I am trying to find a mentored bug to work on. I have just recently learned
> some of the basics of java in school. Please let me know what I need to do
> to move forward. I am willing to contribute in any way I can :)

Hi Daniel,

The first step is to start building and testing Fennec.  Follow the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android.  Fennec is not easy to get started building (various people are trying to help here, but it's not a priority) so direct your questions to #mobile on IRC.

The source code changes would start with ripping out Fennec's existing scoring system.  That means modifying http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java to not use the SCORE_* constants and changing Tabs.onTabChanged (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#456) to "de-bounce" tab writes.  (Here, de-bounce means collect any writes and perform them all after some minimum quiet period has passed.)

That means onTabChanged should
* queue a persistLocalTabs call at some time (say 3 seconds) in the future, if none is already queued; or
* push back an existing queued persistLocalTabs call into the future.

I can elaborate as needed.  Good luck!
Comment 3 Daniel 2013-03-12 19:45:17 PDT
(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to Daniel from comment #1)
> > (In reply to Richard Newman [:rnewman] from comment #0)
> > > Right now Fennec uses a scoring algorithm to decide when to flush its open
> > > tabs to the tabs data provider. This essentially guarantees that Fennec's
> > > database of tabs is never the actual tab state. See Bug 730039 Comment 46.
> > > 
> > > We should implement some kind of idle timer to flush the current tab state,
> > > rather than using a scoring system. (By all means extend the tab system to
> > > do incremental writes, too.)
> > > 
> > > After the flush, Fennec can notify Sync that tab state has changed. See Bug
> > > 780279. This might even be done automatically by Android. Tada!
> > > 
> > > There might be a small amount of Sync-side work to make sure we don't sync
> > > too frequently, or ensure that we do sync.
> > > 
> > > Both halves of this are [good second bug], which I call a fun [good first
> > > bug] if mentoring is adequate. Nick's a great mentor, so here we go!
> > 
> > Hello,
> > I am trying to find a mentored bug to work on. I have just recently learned
> > some of the basics of java in school. Please let me know what I need to do
> > to move forward. I am willing to contribute in any way I can :)
> 
> Hi Daniel,
> 
> The first step is to start building and testing Fennec.  Follow the
> instructions at https://wiki.mozilla.org/Mobile/Fennec/Android.  Fennec is
> not easy to get started building (various people are trying to help here,
> but it's not a priority) so direct your questions to #mobile on IRC.
> 
> The source code changes would start with ripping out Fennec's existing
> scoring system.  That means modifying
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java
> to not use the SCORE_* constants and changing Tabs.onTabChanged
> (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.
> java#456) to "de-bounce" tab writes.  (Here, de-bounce means collect any
> writes and perform them all after some minimum quiet period has passed.)
> 
> That means onTabChanged should
> * queue a persistLocalTabs call at some time (say 3 seconds) in the future,
> if none is already queued; or
> * push back an existing queued persistLocalTabs call into the future.
> 
> I can elaborate as needed.  Good luck!

Hi Nick, Thanks!
I sent you an email asking for a little elaboration. Also, do I need to be running ubuntu or fedora to build fennec? I am running openSUSE 12.1.
Comment 4 Nick Alexander :nalexander 2013-03-14 14:33:46 PDT
> I sent you an email asking for a little elaboration. Also, do I need to be
> running ubuntu or fedora to build fennec? I am running openSUSE 12.1.

Just to not leave this dangling: OpenSUSE should be fine.
Comment 5 Agam 2013-04-07 02:06:29 PDT
okay.so,instead to scoring based call to persistAllTabs() ,we do queing for 3 seconds and then execute it once?
what is "* push back an existing queued persistLocalTabs call into the future."?
also,what is  Sync-side work to make sure we don't sync too frequently, or ensure that we do sync ?..is it the proper estimation of the queing time ? (which is 3 )
Comment 6 Nick Alexander :nalexander 2013-04-08 08:30:11 PDT
Hi Agam,

(In reply to Agam from comment #5)
> okay.so,instead to scoring based call to persistAllTabs() ,we do queing for
> 3 seconds and then execute it once?
> what is "* push back an existing queued persistLocalTabs call into the
> future."?

If we do several tabs operations, we only want to persist to disk once.  So we queue a persist for (say) 3 seconds in the future, and if in 1 second (before the 3 seconds are up) we get another tab operation, instead of persisting twice, we push back the queued persist for another 3 seconds.  Clearer?

> also,what is  Sync-side work to make sure we don't sync too frequently, or
> ensure that we do sync ?..is it the proper estimation of the queing time ?
> (which is 3 )

Android Sync should get a notification from Android when tabs are written to the database, but I'm pretty sure we don't have that configured correctly.  (Details: the sync adapter is configured for the browser provider but not the tabs provider.)  So we might need to ask for a tab sync manually.
Comment 7 Agam 2013-04-08 12:10:26 PDT
thanks :nalexander
what do you suggest ? we use a DelayedQueue or some other thing ?..cant you give me a general guideline
Comment 8 Nick Alexander :nalexander 2013-04-08 12:17:45 PDT
(In reply to Agam from comment #7)
> thanks :nalexander
> what do you suggest ? we use a DelayedQueue or some other thing ?..cant you
> give me a general guideline

Hi Agam,

Sure, we could use a DelayQueue (with a single element in it), but it seems overkill, to be honest: we'll only ever have one outstanding work item.

The hard part will be scheduling the work to check the queue: you should be able to use `ThreadUtils.getBackgroundHandler().postDelayed` from within the `persistLocalTabs` method to take some action in a certain amount of time.  You could keep a timestamp saying when to write to the DB, and in the postDelayed handler, check if enough time has elapsed.  If enough time has elapsed, write to the DB; if not, postDelayed again.
Comment 9 Agam 2013-04-09 12:13:05 PDT
is a solution of this type possible ?
http://www.pastebin.mozilla.org/2289356
Comment 10 justin.busby 2013-04-15 16:44:27 PDT
Hey, would like to take this on as my second bug if possible. had a look at the previous comments and the source so I think i'm up to speed with what is required.
Comment 11 Nick Alexander :nalexander 2013-04-16 13:23:21 PDT
(In reply to Agam from comment #9)
> is a solution of this type possible ?
> http://www.pastebin.mozilla.org/2289356

Hi Agam,

Sorry that I didn't get to this -- could you repost (the pastebin looks empty now).
Comment 12 Nick Alexander :nalexander 2013-04-16 13:24:14 PDT
(In reply to justin.busby from comment #10)
> Hey, would like to take this on as my second bug if possible. had a look at
> the previous comments and the source so I think i'm up to speed with what is
> required.

Hi Justin,

Thanks for your interest.  I'm not 100% clear on whether Agam is still looking at this -- in the meantime, please investigate and post work in progress.  We can all work together to make this happen.
Comment 13 justin.busby 2013-04-18 01:45:14 PDT
Will do - I'll submit a patch over the next few days and keep an eye out for if Agam updates.
Comment 14 Agam 2013-04-18 03:30:08 PDT
http://www.pastebin.mozilla.org/2316675
i have a doubt.
 should there be a flag which indicates already it is postdelayed ?..then we can just update the delayeduntil(long) whenever tabs are changed.This has a potential problem ,if we have frequent change of tabs.
Comment 15 justin.busby 2013-04-18 05:21:58 PDT
Created attachment 738992 [details] [diff] [review]
Removed all scoring code and changed to implement a timeout

New private method containing the runnable to check if we need to persist or wait a bit longer. Volatile variables added for time stamping and flag indicating if the persist has happened.
Comment 16 justin.busby 2013-04-18 05:23:33 PDT
Created attachment 738994 [details] [diff] [review]
Removed redundant import spotted after online diff checked
Comment 17 Agam 2013-04-18 06:42:38 PDT
hello justin,
i saw your patch,nice work
i just though of one issue.. we have a condition for persist 
"if(lastPersistTimestamp+PERSIST_DELAY<System.currentTimeMillis() && !mPersisted) "..according to which we would definitely call "persistAllTabs()" when lastPersistTimestamp < currenttime (+mpersisted is false)..which indicates that we would persist on the first time this holds,and not delay it.(given,we update lastPersistTimestamp only after this holds )
correct me if i am wrong?
Comment 18 justin.busby 2013-04-18 07:33:40 PDT
Thanks. Yes you are correct :) I will fix the patch to init that variable first time through. Afk of my dev box but will fix it this eve.
Comment 19 Nick Alexander :nalexander 2013-04-18 08:38:12 PDT
Justin, Agam,

Just wanted to say thanks for the patch and for reviewing it, respectively -- awesome to see you collaborate on this.  I'll take a look at the updated version when it's posted.  Thanks again!
Comment 20 Agam 2013-04-18 10:03:46 PDT
hey justin,
building upon your changes.instead of "lastPersistTimeStamp" if we have persistAt timestamp.it would be more purposeful.
have a look at my changes.and suggest if anything goes off track.
i have also used a boolean flag to indicate whether we have to make a new postDelayed or just extend the time of a previous one.

http://www.pastebin.mozilla.org/2317598

thanks Justin,Nick
Comment 21 justin.busby 2013-04-19 15:31:10 PDT
Hey Agam,

Sorry - that pastebin is empty. Agree however with the variable naming and the use of a boolean flag. Might be worth exporting a patch file and attaching it as we can't see the pastebin?

Cheers,
Justin.
Comment 22 Agam 2013-04-19 23:16:12 PDT
ah..sorry guys.Pastebin expired in a day.This should be fine
@justin:if you can review it, i would create the patch then :)
Comment 24 justin.busby 2013-04-21 10:05:52 PDT
Hi Agam,

In enqueue method after the flag is set false the first time the runnable will never be rescheduled as it will hit the final else block each time. Modifying the PERSIST_DELAY will not extend the runnable delay time.

persistAt never seems to be set/reset. I presume that is somewhere else in the code

How is the 'flag' variable declared as it's used it 2 different threads?

Make sure indenting is correct - perhaps you used tabs rather than spaces?

Thanks,
Justin.
Comment 25 Agam 2013-04-21 11:44:17 PDT
Thanks for reviewing.You were correct.
persistAt is being set when the flag is true.
flag is a class varible,init to true. 
indenting should be fine now.

http://www.pastebin.mozilla.org/2332209
Comment 26 Richard Newman [:rnewman] 2013-04-21 17:40:34 PDT
Comment on attachment 738994 [details] [diff] [review]
Removed redundant import spotted after online diff checked

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

Let's go around again to address nits and thread-safety before taking Nick's time to review this.

::: mobile/android/base/Tabs.java
@@ +59,5 @@
>  
>      private GeckoApp mActivity;
>      private ContentObserver mContentObserver;
>  
> +    private static final long PERSIST_DELAY = 1000 * 5; //5 second delay - might move this into a setting for about:config

Space after //, closing period, and either do the "might" or don't!

(Doing it would be hard, by the way. A shared preference is more feasible, but I don't think this is worthwhile.)

@@ +61,5 @@
>      private ContentObserver mContentObserver;
>  
> +    private static final long PERSIST_DELAY = 1000 * 5; //5 second delay - might move this into a setting for about:config
> +    private volatile long lastPersistTimestamp;
> +    private volatile boolean mPersisted;

Whenever you see two volatile references next to each other, your thread safety alarm should sound in your head.

Do we need two here? Can you not use the timestamp as a pseudo-boolean by using a sentinel value like 0?

@@ +101,5 @@
>  
>          mAccountListener = new OnAccountsUpdateListener() {
>              @Override
>              public void onAccountsUpdated(Account[] accounts) {
> +                Log.i(LOGTAG, "Persisting onAccountsUpdated");

Remove debug logging.

@@ +530,5 @@
>      private void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {
>          switch (msg) {
>              case LOCATION_CHANGE:
> +                mPersisted = false;
> +                queuePersist();         

Nit: trailing whitespace.

@@ +554,3 @@
>  
> +    /* if the user has changed a tab then we queue a persit for the future.
> +       if the user performs another action before our timeout the timestamp will be

Sentences start with a capital letter.

Use JavaDoc comment format:

/**
 * If the user ...
 */

@@ +559,5 @@
> +    private void queuePersist() {
> +        ThreadUtils.getUiHandler().postDelayed(new Runnable() {
> +                @Override
> +                public void run() {
> +                    if(lastPersistTimestamp+PERSIST_DELAY<System.currentTimeMillis() && !mPersisted) {

Space between if and (, and around arithmetic and comparison operators.

Note that this line isn't thread-safe: you're retrieving two volatile references, but the two operations are not synchronized together. It's possible for `lastPersistTimestamp` to change between checking that and checking `mPersisted`. I'm not sure whether that matters, but you should think about it.

See my earlier suggestion about perhaps merging these values.

@@ +561,5 @@
> +                @Override
> +                public void run() {
> +                    if(lastPersistTimestamp+PERSIST_DELAY<System.currentTimeMillis() && !mPersisted) {
> +                         lastPersistTimestamp = System.currentTimeMillis();
> +                         mPersisted = true;

Aren't these things that `persistAllTabs()` should do?

@@ +564,5 @@
> +                         lastPersistTimestamp = System.currentTimeMillis();
> +                         mPersisted = true;
> +                         persistAllTabs();
> +                    } else {
> +                         if(!mPersisted) {

Spacing.

@@ +565,5 @@
> +                         mPersisted = true;
> +                         persistAllTabs();
> +                    } else {
> +                         if(!mPersisted) {
> +                             Log.i(LOGTAG, "Persist pushed back");

Log.d(LOGTAG, "Delaying tab persistence.");

@@ +578,5 @@
>      public void persistAllTabs() {
>          final GeckoApp activity = getActivity();
>          final Iterable<Tab> tabs = getTabsInOrder();
> +
> +        Log.i(LOGTAG, "Persisted");

Kill this and the extra whitespace.
Comment 27 Jamie Hewland 2013-05-02 15:37:03 PDT
Hi there,

I'm also looking for a first bug. Don't mean to "crash" this one but it just caught my eye because it's Java + Android stuff which is where I'm most comfortable.

Why not do something much, much simpler? Remove the class variables mPersisted and lastPersistTimestamp and simply call queuePersist():
http://pastebin.mozilla.org/2369705

(except I meant Handler.removeCallbacks not removeRunnables...)
Comment 28 Nick Alexander :nalexander 2013-05-03 08:32:14 PDT
(In reply to Jamie Hewland from comment #27)
> Hi there,
> 
> I'm also looking for a first bug. Don't mean to "crash" this one but it just
> caught my eye because it's Java + Android stuff which is where I'm most
> comfortable.

Join the party!

> Why not do something much, much simpler? Remove the class variables
> mPersisted and lastPersistTimestamp and simply call queuePersist():
> http://pastebin.mozilla.org/2369705
> 
> (except I meant Handler.removeCallbacks not removeRunnables...)

I think this is an excellent approach, and it's even nicer that Android's |Handler| already implements the framework.

The runnable must be posted to the *background* thread, however -- we can't block UI for this database operation -- and we identify the runnable by its identity, so make it final.  Post a patch!
Comment 29 Jamie Hewland 2013-05-03 12:35:20 PDT
(In reply to Nick Alexander :nalexander from comment #28)

Great. I'll be sure to post a patch tomorrow morning (for now I must sleep).

Just one simple question about style- because I've seen this done a few different ways: whereabouts in the file should I define the Runnable? Together with the rest of the class variables? Or somewhere else?
Comment 30 Richard Newman [:rnewman] 2013-05-03 12:48:56 PDT
(In reply to Jamie Hewland from comment #29)

> Just one simple question about style- because I've seen this done a few
> different ways: whereabouts in the file should I define the Runnable?
> Together with the rest of the class variables? Or somewhere else?

If it's used only once, and it's short or basically the only thing inside its containing method, do it anonymously.

If it's long (and thus will bloat the containing method), or is reused elsewhere in the same class, do it as an inner class.

If it's really long, or is reused elsewhere in the codebase, or needs preprocessing, then define it as a standalone class in its own file.

Thanks for asking about this -- one can't necessarily use the oldest parts of the codebase as examples of good style!
Comment 31 Nick Alexander :nalexander 2013-05-03 13:47:02 PDT
(In reply to Jamie Hewland from comment #29)
> (In reply to Nick Alexander :nalexander from comment #28)
> 
> Great. I'll be sure to post a patch tomorrow morning (for now I must sleep).
> 
> Just one simple question about style- because I've seen this done a few
> different ways: whereabouts in the file should I define the Runnable?
> Together with the rest of the class variables? Or somewhere else?

Check the Handler docs, but it is most likely that the `removeRunnable` will only remove the identical Runnable.  That means the Runnable needs to persist, so put the member (mRunnable, or mPersistTabsRunnable or whatever) in with the other member variables.  You can lazy initialize it if you want.
Comment 32 Jamie Hewland 2013-05-04 03:25:18 PDT
Created attachment 745519 [details] [diff] [review]
Use the Android Handler to send a single delayed message to persist tabs
Comment 33 Jamie Hewland 2013-05-04 03:26:35 PDT
(In reply to Nick Alexander :nalexander from comment #31)
> Check the Handler docs, but it is most likely that the `removeRunnable` will
> only remove the identical Runnable.  That means the Runnable needs to
> persist, so put the member (mRunnable, or mPersistTabsRunnable or whatever)
> in with the other member variables.  You can lazy initialize it if you want.

I had a peek at the Android source and it just does an == check on the Runnables in the handler queue (which if you think about it is all it really could do). Hmm.. I've somehow never (purposefully) used lazy initialization...

A few other things... 1) I hadn't even had a look at the persistAllTabs() code when I wrote that pastebin fix so it was going runnable -> ui handler -> new runnable -> background handler. I hadn't realized. Now there is just a single Runnable that does the stuff originally in the persistAllTabs() runnable. As far as I can see, the calls to getActivity() and getTabsInOrder() from the runnable should be thread-safe.

2) I'm still not really clear about the style of declaring the Runnable. But there is a patch now so I'm sure you'll be able to clear that up.

3) I'm not really sure what the complete story is with GeckoActivity life-cycle management but it seems that we should probably remove any delayed callbacks to the activity from the handler queue when we detach from the activity.

I'm completely new to Mercurial so I hope I've done this right.

Thanks
Comment 34 Nick Alexander :nalexander 2013-05-06 11:12:11 PDT
Comment on attachment 745519 [details] [diff] [review]
Use the Android Handler to send a single delayed message to persist tabs

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

This is looking really good, Jamie -- thanks!  I've asked rnewman for his perspective on one point I'm not certain on, and made some small comments.  Back to you to address those small nits (before or after rnewman's feedback -- your option.)  Thanks again!

::: mobile/android/base/Tabs.java
@@ +57,5 @@
>      public static final int LOADURL_DELAY_LOAD = 16;
>      public static final int LOADURL_DESKTOP = 32;
>      public static final int LOADURL_BACKGROUND = 64;
>  
> +    private static final long DELAY_PERSIST_TABS = 1000 * 5;

Let's be really explicit: |PERSIST_TABS_AFTER_MILLISECONDS|

@@ +66,5 @@
>      private GeckoApp mActivity;
>      private ContentObserver mContentObserver;
>  
> +    private final Runnable mPersistTabsRunnable = new Runnable() {
> +        

nit: kill this newline.

@@ +71,5 @@
> +        @Override
> +        public void run() {
> +            boolean syncIsSetup = SyncAccounts.syncAccountsExist(getActivity());
> +            if (syncIsSetup) {
> +                TabsAccessor.persistLocalTabs(getContentResolver(), getTabsInOrder());

This has subtly changed the semantics.

The Runnable is posted to the background thread and run in the future, so I'm worried that |getActivity()| might not be valid by the time this executes.  Let's be very cautious and use |mActivity|, with a null check.  (At the end of the day, SyncAccounts only needs this to get an AccountManager instance anyway.)

A bigger change is that this will fetch the set of tabs to persist in the future, not at the time the Runnable is scheduled.  I'm again concerned that we might not be able to actually get the set of tabs in the future, but I'm not familiar enough with the Tabs lifecycle to answer this -- calling for backup: rnewman, mfinkle, wesj?

@@ +550,5 @@
>                  mInitialTabsAdded = true;
>                  break;
>  
>              // When one tab is deselected, another one is always selected, so only
>              // increment the score once. When tabs are added/closed, they are also

Update this comment, since scoring is being removed.  Maybe:

"When one tab is deselected, another one is always selected, so only queue a single persist operation.  When tabs are added/closed..."

@@ +569,5 @@
> +        ThreadUtils.postToBackgroundThread(mPersistTabsRunnable);
> +    }
> +
> +    /**
> +     * Removes any delayed requests to persist tabs and posts a new request after DELAY_PERSIST_TABS

nit: line lengths don't seem to match file's.

Try: "Queue a request to persist tabs after PERSIST_TABS_AFTER_MILLISECONDS milliseconds have elapsed.  If any existing requests are already queued, remove them."
Comment 35 Nick Alexander :nalexander 2013-05-06 11:16:02 PDT
I would like to consider rolling back https://bugzilla.mozilla.org/show_bug.cgi?id=826467 as part of this patch, too.  The PICL team will appreciate not having to hack Fennec or create a Sync account when pulling tabs from the DB.  Thoughts?
Comment 36 Mark Finkle (:mfinkle) (use needinfo?) 2013-05-06 11:27:00 PDT
(In reply to Nick Alexander :nalexander from comment #35)
> I would like to consider rolling back
> https://bugzilla.mozilla.org/show_bug.cgi?id=826467 as part of this patch,
> too.  The PICL team will appreciate not having to hack Fennec or create a
> Sync account when pulling tabs from the DB.  Thoughts?

I have become a DB h8ter on Android, so I'd need strong motivation for doing any DB code execution for no reason.
Comment 37 Richard Newman [:rnewman] 2013-05-06 13:43:50 PDT
Comment on attachment 745519 [details] [diff] [review]
Use the Android Handler to send a single delayed message to persist tabs

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

See also Nick's nits.

::: mobile/android/base/Tabs.java
@@ +71,5 @@
> +        @Override
> +        public void run() {
> +            boolean syncIsSetup = SyncAccounts.syncAccountsExist(getActivity());
> +            if (syncIsSetup) {
> +                TabsAccessor.persistLocalTabs(getContentResolver(), getTabsInOrder());

mTabs is a final COWAL, so getTabsInOrder is safe -- the Runnable will hold a reference to Tabs in its enclosing scope, so even if we stop Tabs being a magical singleton of evil, this will continue to work.

We remove tabs from mOrder before doing any other removal actions, so that's all good.

But yes, this will be a delayed snapshot.

getContentResolver is safe by fiat.

getActivity is exactly a "safe" (read: throws if null) wrapper around mActivity -- see line 336 -- so using that is actually what you suggest.
Comment 38 Nick Alexander :nalexander 2013-05-10 16:02:47 PDT
Jamie, any chance for you to get to these nits and update your patch?  I'd love to get this one landed.
Comment 39 Jamie Hewland 2013-05-14 02:27:13 PDT
Sorry, completely overloaded with university work at the moment. Will update my patch on Thursday when term ends.
Comment 40 Nick Alexander :nalexander 2013-05-14 08:26:31 PDT
(In reply to Jamie Hewland from comment #39)
> Sorry, completely overloaded with university work at the moment. Will update
> my patch on Thursday when term ends.

Hehe -- I was a student for ages until two years ago.  Just wanted to know if I should take over or re-assign this.  Good luck with exams, etc, and talk to you soon.
Comment 41 Jamie Hewland 2013-05-16 14:46:09 PDT
Created attachment 750714 [details] [diff] [review]
Use the Android Handler to send a single delayed message to persist tabs (nits fixed)

Thanks for your patience. I think I've fixed the nits you posted. Slightly rephrased your suggested comment for queuePersistAllTabs() to better match the style/tense of other javadoc-style comments.

With regards to persisting the tabs in the future... isn't that actually what we want (the up-to-date tab state)? If any major changes (remove/move/add) occur then the old runnable is aborted. 
Still, I could imagine that it could be unsafe if the tabs change state in some other way that breaks saving but that obviously depends on the tab implementation...
Comment 42 Nick Alexander :nalexander 2013-05-19 13:43:21 PDT
Comment on attachment 750714 [details] [diff] [review]
Use the Android Handler to send a single delayed message to persist tabs (nits fixed)

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

Jamie, this looks great!  nit: there are a few places with trailing whitespace.  I'll kill that when I apply your patch.  Good stuff!
Comment 43 Nick Alexander :nalexander 2013-05-19 14:21:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7fb00063dd
Comment 44 Ryan VanderMeulen [:RyanVM] 2013-05-20 13:28:39 PDT
https://hg.mozilla.org/mozilla-central/rev/cc7fb00063dd

Note You need to log in before you can comment on or make changes to this bug.