742771 - Browser shows old URL on startup

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

When launching the browser from an application (such as Twitter) after it has been closed from a previous session, the URL bar shows the title of the last page. Once Gecko has loaded, the URL bar changes, but due to the change, the loading time feels much longer.

Comment 1

[JP Rosevear [:jpr]]

Related to bug 741712?

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: patch

In general, we aren't handling mLastTitle correctly. We're only setting it when we first launch Fennec, so an old title gets saved when saving mLastTitle to the bundle. Also, we're calling mBrowserToolbar.setTitle() before we determine whether we're loading an external URL (in which case we should discard mLastTitle).

Rather than save mLastTitle to the bundle, we should be able to just the title directly from the selected tab.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 612690 [details] [diff] [review]
patch

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

What is mLastTitle used for after this patch? Looks like it can just be removed.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 612690 [details] [diff] [review]
patch

r- to see a new patch without mLastTitle or hear why its still needed

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Brad Lassey [:blassey] from comment #3)
> Comment on attachment 612690 [details] [diff] [review]
> patch
> 
> Review of attachment 612690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is mLastTitle used for after this patch? Looks like it can just be
> removed.

mLastTitle is used in initialize() when we call `mBrowserToolbar.setTitle(mLastTitle);`. We get the bundle parameter in onCreate(), but it's not used until initialize(), so I'm using a class variable that's accessible in both places.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

initialize() is triggered by a message posted from onResume(). Logically, this should be triggered from onCreate() where we get the last title from the bundle and not onResume() where we need to check mInitialized every time.

But to make this less of a change, just "mBrowserToolbar.setTitle(mLastTitle)" initialize to onCreate(), where already have mLastTitle (rename it obviously) from the bundle and drop mLastTitle entirely

Comment 7

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

(In reply to Brad Lassey [:blassey] from comment #6)
> initialize() is triggered by a message posted from onResume(). Logically,
> this should be triggered from onCreate() where we get the last title from
> the bundle and not onResume() where we need to check mInitialized every time.

We trigger initialize in onResume to speed up showing the main UI. That's the reason we moved the code in intialize out of onCreate.

> But to make this less of a change, just
> "mBrowserToolbar.setTitle(mLastTitle)" initialize to onCreate(), where
> already have mLastTitle (rename it obviously) from the bundle and drop
> mLastTitle entirely

Setting the URL when the main UI shows up seems like a good idea. We have had some reports saying that the external URL is not displayed for a moment or two after the main UI appears. This change should address that issue.

Comment 8

[Brian Nicholson (:bnicholson)]

A problem here is that we don't necessarily always want to set the last title. When launching URLs externally - from shortcuts or links, for example - we instead want to show the URL of the page we're going to.

The intent that tells us we're launching an external URL isn't available until onNewIntent(). onCreate() is called before onNewIntent(), so we don't know in onCreate() whether we're going to be showing the last page or launching an external URL.

We could set it onCreate(), then change it if we find that we're opening a different page, but this would cause the old URL to appear first (which is what we're trying to fix in this bug).

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> The intent that tells us we're launching an external URL isn't available
> until onNewIntent(). onCreate() is called before onNewIntent(), so we don't
> know in onCreate() whether we're going to be showing the last page or
> launching an external URL.
it is available from getIntent() in onCreate()

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Brad Lassey [:blassey] from comment #9)
> (In reply to Brian Nicholson (:bnicholson) from comment #8)
> > The intent that tells us we're launching an external URL isn't available
> > until onNewIntent(). onCreate() is called before onNewIntent(), so we don't
> > know in onCreate() whether we're going to be showing the last page or
> > launching an external URL.
> it is available from getIntent() in onCreate()

It may be the old intent; that's why we had to explicitly set the intent in bug 731333.

Comment 11

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

Let's get a patch that correctly stops setting the old URL, and leave setting the new external URL in onNewIntent. That's the core of this bug. We can explorer setting the new external URL sooner in a different bug.

Comment 12

[Brian Nicholson (:bnicholson)]

Attachment: enable setting toolbar title before toolbar exists in layout

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: delay creation of BrowserToolbar until initialize()

This patch delays adding the BrowserToolbar to the layout until initialize(). We still create the toolbar in onCreate(), and with the previous patch, we can also set the title in onCreate() without having to carry around an mLastTitle. By delaying adding the BrowserToolbar until we know the title, we don't have to worry about the title being empty. Finally, this may slightly boost our startup performance since we aren't immediately adding the toolbar with the rest of the UI.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: delay creation of BrowserToolbar until initialize()

forgot to completely remove mLastTitle

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 614517 [details] [diff] [review]
delay creation of BrowserToolbar until initialize()

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

::: mobile/android/base/GeckoApp.java
@@ +1623,1 @@
>  

at this point you should be able to set the title on the browser toolbar without the previous patch, right? Just keep the title as a variable in this function (onCreate()) and set it here. Drop the previous patch.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 614510 [details] [diff] [review]
enable setting toolbar title before toolbar exists in layout

this change appears to be unnecessary

Comment 17

[Brian Nicholson (:bnicholson)]

Attachment: patch

This patch sets the last title immediately in onCreate(), then changes the title, if necessary, when we receive the new intent in onNewIntent().

Comment 18

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0cac35aa7296

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0cac35aa7296

Comment 20

[Andreea Pod]

When opening a link from other application with Nightly I don't see the URL of the previous loaded page. 

Verified fixed on build: Firefox 14.0a1 (2012-04-18)
Device: LG Optimus 2X (Android 2.2.2) and Motorola Droid Pro (Android 2.3.4)