Comment on attachment 8524528 [details] [diff] [review] Avoid duplicate RESTORED tab event on startup (r=bnicholson) We unconditionally trigger TabEvents.RESTORED in GeckoApp's initialization code[1] after creating the stub tabs on startup. I don't really follow why the extra event from Gecko is necessary. Am I missing something? [1] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?from=geckoapp.java#1524

Comment on attachment 8524528 [details] [diff] [review] Avoid duplicate RESTORED tab event on startup (r=bnicholson) Review of attachment 8524528 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Lucas Rocha (:lucasr) from comment #2) > We unconditionally trigger TabEvents.RESTORED in GeckoApp's initialization > code[1] after creating the stub tabs on startup. I don't really follow why > the extra event from Gecko is necessary. Am I missing something? I believe this one is there solely to handle the animation for external tabs at startup. Consider the case where Fennec has died in the background, and the user has clicked a link to load in Fennec. Since the OOM kill/restore should be transparent to the user, we would still want to show the new tab animation after restoring their existing tabs. Waiting until all startup tabs have been created means we've also created the external tab, meaning we won't show that animation. That said, it's wrong to notify listeners of RESTORED twice. What do you think of adding a check around [1] to return early for RESTORED events where mInitialTabsAdded == true? [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#603

Thinking about this a bit more, I don't think SessionStore.js should even be driving Session:RestoreEnd anymore since we have stubs -- the Java front-end owns the restore. We can probably just remove the Session:RestoreEnd message/listeners altogether, then tweak GeckoApp to notify RESTORE appropriately. Perhaps something like this: // External URLs should always be loaded regardless of whether Gecko is // already running. if (isExternalURL) { Tabs.getInstance().notifyListeners(null, Tabs.TabEvents.RESTORED); loadStartupTab(passedUri); } else if (!mIsRestoringActivity) { loadStartupTab(null); Tabs.getInstance().notifyListeners(null, Tabs.TabEvents.RESTORED); }

Comment on attachment 8529781 [details] [diff] [review] Avoid duplicate RESTORED tab event on startup (r=mhaigh) This is doing exactly what bnicholson suggested in comment #4. Redirecting the review request to mhaigh as bnicholson is on holiday today.

Comment on attachment 8529781 [details] [diff] [review] Avoid duplicate RESTORED tab event on startup (r=mhaigh) Review of attachment 8529781 [details] [diff] [review]: ----------------------------------------------------------------- Looks good