Closed Bug 742771 Opened 8 years ago Closed 8 years ago

Browser shows old URL on startup

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: cwiiis, Assigned: bnicholson)

Details

(Whiteboard: [has reviewed patch])

Attachments

(1 file, 4 obsolete files)

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.
blocking-fennec1.0: --- → ?
Related to bug 741712?
Assignee: nobody → bnicholson
blocking-fennec1.0: ? → beta+
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #612690 - Flags: review?(blassey.bugs)
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 on attachment 612690 [details] [diff] [review]
patch

r- to see a new patch without mLastTitle or hear why its still needed
Attachment #612690 - Flags: review?(blassey.bugs) → review-
(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.
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
(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.
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).
(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()
(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.
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.
Attachment #614510 - Flags: review?(blassey.bugs)
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.
Attachment #612690 - Attachment is obsolete: true
Attachment #614515 - Flags: review?(blassey.bugs)
forgot to completely remove mLastTitle
Attachment #614517 - Flags: review?(blassey.bugs)
Attachment #614515 - Attachment is obsolete: true
Attachment #614515 - Flags: review?(blassey.bugs)
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.
Attachment #614517 - Flags: review?(blassey.bugs) → review-
Comment on attachment 614510 [details] [diff] [review]
enable setting toolbar title before toolbar exists in layout

this change appears to be unnecessary
Attachment #614510 - Flags: review?(blassey.bugs) → review-
Attached patch patchSplinter Review
This patch sets the last title immediately in onCreate(), then changes the title, if necessary, when we receive the new intent in onNewIntent().
Attachment #614510 - Attachment is obsolete: true
Attachment #614517 - Attachment is obsolete: true
Attachment #614902 - Flags: review?(blassey.bugs)
Attachment #614902 - Flags: review?(blassey.bugs) → review+
Whiteboard: [has reviewed patch]
https://hg.mozilla.org/mozilla-central/rev/0cac35aa7296
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.