Closed Bug 1100897 Opened 10 years ago Closed 9 years ago

Duplicate RESTORED tab event on startup when "always restore tabs" is on

Categories

(Firefox for Android Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(2 files)

      No description provided.
Priority: -- → P1
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
Attachment #8524528 - Flags: feedback?(bnicholson)
Blocks: 1055604
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
Attachment #8524528 - Flags: feedback?(bnicholson) → feedback+
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.
Attachment #8529781 - Flags: review?(mhaigh)
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
Attachment #8529781 - Flags: review?(mhaigh) → review+
https://hg.mozilla.org/mozilla-central/rev/2ab7d3193c87
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.