800199 - Stub initial tab before Gecko starts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

This is similar to bug 722661, but we can also stub the initial tab (about:home or the external URL being loaded) even if we aren't doing a session restore. 

This would also have the side effect of fixing bug 799977. Bug 799977 happens when clicking a "tabs from last time" entry before Gecko has loaded, and it fails when calling Tabs.getSelectedTab() (since there are no tabs at the time). Creating an about:home stub guarantees that a tab will exist here.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Stub initial tab before Gecko starts

This patch removes some of the special case code we had for handling the initial tab at startup. Here's some explanations since some of these changes aren't obvious:

> -                passedUri = "about:empty";

> -    if (url == "about:empty")
> -      loadParams.flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY;

These changes revert bugs 743415 and 735838. We're triggering these tab loads directly from Java, so we shouldn't need to worry about the "about:home or null" weirdness described in https://bugzilla.mozilla.org/show_bug.cgi?id=735838#c5.


> -    if (!browser)
> -      return;

If we're calling Tab:Load to create the initial tab, we can't have this check at the top of the observer since we won't have a selected browser yet. I don't think this check is necessary since we grey out all menu items that require a tab if we haven't received a Gecko:Ready yet.

I've verified that this fixes bug 799977.

Comment 2

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

Comment on attachment 670197 [details] [diff] [review]
Stub initial tab before Gecko starts

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+        // Tabs for external URLs are created in BrowserApp's initializeChrome().
>+        // Create them here if this is a webapp.
>+        if (action != null && action.startsWith(GeckoApp.ACTION_WEBAPP_PREFIX)) {
>+            Tabs.getInstance().loadUrl(uri, Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_PINNED);
>+        }

Couldn't we move this to WebApp ? If so, file a followup.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    if (url == "about:empty")
>-      loadParams.flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_HISTORY;

This makes me nervous. Let's really make sure we don't regress bug 743415

Overall I think the cleanup is good and hopefully the UI acts more responsive during startup

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Stub initial tab before Gecko starts, v2

> >+        // Tabs for external URLs are created in BrowserApp's initializeChrome().
> >+        // Create them here if this is a webapp.
> >+        if (action != null && action.startsWith(GeckoApp.ACTION_WEBAPP_PREFIX)) {
> >+            Tabs.getInstance().loadUrl(uri, Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_PINNED);
> >+        }
> 
> Couldn't we move this to WebApp ? If so, file a followup.

Oops, thanks. It's not a big change, so I'll just do that now.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Stub initial tab before Gecko starts, v3

Added missing call to super method.

Comment 5

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

Comment on attachment 670858 [details] [diff] [review]
Stub initial tab before Gecko starts, v3

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>-    void initializeChrome(String uri, Boolean isExternalURL) {
>+    protected void initializeChrome(String uri, Boolean isExternalURL) {
>         mDoorHangerPopup = new DoorHangerPopup(this, null);
>     }

Not your problem, but the way we initialize mDoorHangerPopup here and in BrowserApp.initializeChrome, but not in WebApp.initializeChrome is weird and error-prone. We should file a bug to clean it up.

Comment 6

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 670858 [details] [diff] [review]
> Stub initial tab before Gecko starts, v3
> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >-    void initializeChrome(String uri, Boolean isExternalURL) {
> >+    protected void initializeChrome(String uri, Boolean isExternalURL) {
> >         mDoorHangerPopup = new DoorHangerPopup(this, null);
> >     }
> 
> Not your problem, but the way we initialize mDoorHangerPopup here and in
> BrowserApp.initializeChrome, but not in WebApp.initializeChrome is weird and
> error-prone. We should file a bug to clean it up.

We don't override initializeChrome in WebApp.java, so GeckoApp takes care of it for us. We just override it in BrowserApp because we want to pass in an extra parameter for the anchor.

Comment 7

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

(In reply to Margaret Leibovic [:margaret] from comment #6)
> (In reply to Mark Finkle (:mfinkle) from comment #5)
> > Comment on attachment 670858 [details] [diff] [review]
> > Stub initial tab before Gecko starts, v3
> > 
> > >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> > 
> > >-    void initializeChrome(String uri, Boolean isExternalURL) {
> > >+    protected void initializeChrome(String uri, Boolean isExternalURL) {
> > >         mDoorHangerPopup = new DoorHangerPopup(this, null);
> > >     }
> > 
> > Not your problem, but the way we initialize mDoorHangerPopup here and in
> > BrowserApp.initializeChrome, but not in WebApp.initializeChrome is weird and
> > error-prone. We should file a bug to clean it up.
> 
> We don't override initializeChrome in WebApp.java, so GeckoApp takes care of
> it for us. We just override it in BrowserApp because we want to pass in an
> extra parameter for the anchor.

Exactly. That is too fragile. I think we should consider making GeckoApp be able to handle a null mDoorHangerPopup and move all door hanger popup initialization to subclasses (BrowserApp and WebApp) which makes things seem more sane.

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e7208eeb05da

Comment 9

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Margaret Leibovic [:margaret] from comment #6)
> > (In reply to Mark Finkle (:mfinkle) from comment #5)
> > > Comment on attachment 670858 [details] [diff] [review]
> > > Stub initial tab before Gecko starts, v3
> > > 
> > > >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> > > 
> > > >-    void initializeChrome(String uri, Boolean isExternalURL) {
> > > >+    protected void initializeChrome(String uri, Boolean isExternalURL) {
> > > >         mDoorHangerPopup = new DoorHangerPopup(this, null);
> > > >     }
> > > 
> > > Not your problem, but the way we initialize mDoorHangerPopup here and in
> > > BrowserApp.initializeChrome, but not in WebApp.initializeChrome is weird and
> > > error-prone. We should file a bug to clean it up.
> > 
> > We don't override initializeChrome in WebApp.java, so GeckoApp takes care of
> > it for us. We just override it in BrowserApp because we want to pass in an
> > extra parameter for the anchor.
> 
> Exactly. That is too fragile. I think we should consider making GeckoApp be
> able to handle a null mDoorHangerPopup and move all door hanger popup
> initialization to subclasses (BrowserApp and WebApp) which makes things seem
> more sane.

Looks like we already do null checks whenever mDoorHangerPopup is used. I filed bug 801739.

Comment 10

[Brian Nicholson (:bnicholson)]

Backed out for Android failures:

http://hg.mozilla.org/integration/mozilla-inbound/rev/3ff5410852cc

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Stub initial tab before Gecko starts, v4

This failed on TBPL because we create our telemetry prompt - which requires a tab - in startup(). This patch creates the initial tab immediately after startup(), so we need to delay showing the telemetry prompt until then.

I also addressed comment 5 with this patch by calling mDoorHangerPopup.setAnchor() in the overridden initializeChrome(), allowing it to call the super method.

Comment 12

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5013505b3930

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5013505b3930