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

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Priority: -- → P1
(Assignee)

Comment 1

4 years ago
Created attachment 8524528 [details] [diff] [review]
Avoid duplicate RESTORED tab event on startup (r=bnicholson)
(Assignee)

Comment 2

4 years ago
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)
(Assignee)

Updated

4 years ago
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);
        }
(Assignee)

Comment 5

4 years ago
Created attachment 8529781 [details] [diff] [review]
Avoid duplicate RESTORED tab event on startup (r=mhaigh)
(Assignee)

Comment 6

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.