839602 - Immediately update browser toolbar after awesomescreen closes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

After entering a URL from the awesomescreen, we close start the throbber, but we still show the old page title for a while (probably before we get a locationchange event). We should just update the toolbar with the new URL immediately, since we do have it in Java as soon as the awesomescreen activity closes.

Comment 1

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #0)
> we close start the throbber

I think I meant to say something like "we close the awesomesreen and start the throbber".

Comment 2

[:Margaret Leibovic]

Attachment: WIP

I'm trying to decide if this is hacky or not. Maybe we should actually be changing the Tab title ASAP, rather than just updating the toolbar.

However, this definitely makes the awesomebar/awesomescreen feel more in sync, since whatever you typed in the awesomescreen "stays" in the awesomebar when the awesomescreen is closed.

One thing that's not quite right with this, is going to "about:home" (or "about:privatebrowsing"), since setTitle is checking tab.getURL() there instead of the title that was entered. I wonder if we can just check the title string that was passed in. I was the one who did this a long time ago, so I don't trust that there's any good reason behind it :)
http://hg.mozilla.org/mozilla-central/rev/1b9c27fad1b9

Comment 3

[:Margaret Leibovic]

Attachment: WIP

This at least fixes the about:home issue.

This feels really nice when testing it, but I'm going to do a bit more investigation to make sure we don't get into some inconsistent state if the page never loads (like if the user hits the stop button).

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 712715 [details] [diff] [review]
WIP

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

Looking good.

::: mobile/android/base/BrowserToolbar.java
@@ +962,5 @@
>  
>          // Setting a null title will ensure we just see
>          // the "Enter Search or Address" placeholder text
> +        if (tab != null && ("about:home".equals(title) ||
> +                            "about:privatebrowsing".equals(title)))

I'd probably do the title comparison in addition to the url, just in case. Also, this change probably deserves a comment explaining why title is relevant here.

Comment 5

[:Margaret Leibovic]

Attachment: alternate approach (update tab title when loading a url)

This patch feels less hacky than my previous one. Instead of just updating the toolbar title, this patch updates the Tab's title when we call Tabs.loadUrl().

I think this is a good approach, since in the case where we're loading a url in a new tab, we initialize the title as the url that was entered.

Brian, you've worked in this code a lot. What do you think?

(Also, can mSelectedTab be null here? I added a check just in case, but if it is null, I don't know how that might affect things.)

Comment 6

[:Margaret Leibovic]

Come to think of it, I'm wondering if we should also call |selectedTab.updateURL(tabUrl)| in here.

There are sometimes cases where we don't successfully load a URL (like network problems), and I wonder if this would help prevent the user from losing the URL he/she just entered. However, we'd need to make sure we're still in sync with the Gecko side here.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 714214 [details] [diff] [review]
alternate approach (update tab title when loading a url)

I think there are some problems with this approach. updateTitle() updates the database with the new title when called (http://hg.mozilla.org/mozilla-central/file/953b1db7a246/mobile/android/base/Tab.java#l249), so wouldn't this change the title of the history entry of the previous page? That is, I'm on google.com, go to the AwesomeScreen, then enter "foo" - I think this means that my google.com history entry is now stored with the title "foo".

You could try working around this by first setting the tab URL before setting the title, which should guarantee that the updated title is associated with the correct URL. But unfortunately, we don't have a real URL at this point as the user could have done a search, and bug 726080 guarantees that tab.getURL() is always an actual, valid URL (or null).

If we want to fix this, I would vote for something closer to what you had in your first approach - changing what's displayed in the browser toolbar, not the actual tab data. After all, you're doing this solely as UI tweak, so there's no reason to change any underlying data structures. But it might take some care to avoid regressing certain things; for example, after hitting the stop button, users should see the title of the page they're on, not the text they entered in the AwesomeScreen before stopping the load.

Comment 8

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #4)

> ::: mobile/android/base/BrowserToolbar.java
> @@ +962,5 @@
> >  
> >          // Setting a null title will ensure we just see
> >          // the "Enter Search or Address" placeholder text
> > +        if (tab != null && ("about:home".equals(title) ||
> > +                            "about:privatebrowsing".equals(title)))
> 
> I'd probably do the title comparison in addition to the url, just in case.
> Also, this change probably deserves a comment explaining why title is
> relevant here.

I don't think we need to check for both the url and the title because this is really just a special case check for about:home and about:privatebrowsing. This would only break if we give those two pages titles, and if we do that we'll probably be changing this placeholder logic anyway. But a comment doesn't hurt, so I can add one!

Comment 9

[:Margaret Leibovic]

Attachment: updated original patch

Based on bnicholson's concerns, I decided to go back to my original patch. I noticed that the refresh() call in the SELECTED/LOCATION_CHANGE/LOAD_ERROR handler will update the title with tab.getDisplayTitle(), but I also added a call to the STOP handler to take care of resetting the title.

I also made setTitle private because it doesn't need to be public.

(Although I should note that the first part of comment 7 isn't relevant anymore now that bug 841938 landed.)

Comment 10

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

Comment on attachment 718068 [details] [diff] [review]
updated original patch

Seems like ti should be safe, but we have a lot of edge cases around the browser toolbar and titles. Watch for regressions. Land this separately so we can regression-range easier, if needed.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 718068 [details] [diff] [review]
updated original patch

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

::: mobile/android/base/BrowserApp.java
@@ +366,5 @@
>  
>      @Override
>      protected void onActivityResult(int requestCode, int resultCode, Intent data) {
>          super.onActivityResult(requestCode, resultCode, data);
> +        String url = data.getStringExtra(AwesomeBar.URL_KEY);

I think you'll need to check the resultCode for RESULT_CANCELED; otherwise, URL_KEY will be null. I think this will happen if you just open the AwesomeScreen and hit back.

Comment 12

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #11)

> ::: mobile/android/base/BrowserApp.java
> @@ +366,5 @@
> >  
> >      @Override
> >      protected void onActivityResult(int requestCode, int resultCode, Intent data) {
> >          super.onActivityResult(requestCode, resultCode, data);
> > +        String url = data.getStringExtra(AwesomeBar.URL_KEY);
> 
> I think you'll need to check the resultCode for RESULT_CANCELED; otherwise,
> URL_KEY will be null. I think this will happen if you just open the
> AwesomeScreen and hit back.

Nice catch. I also just realized that the user can enter an empty string, which will take them nowhere, so we don't want to update the title in that case either.

Comment 13

[:Margaret Leibovic]

Attachment: patch v2

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 718550 [details] [diff] [review]
patch v2

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

One thing worth noting is that anything that triggers a refresh (e.g., an orientation change) will reset the URL bar text. This may not be a problem, but one possible fix would be to create a member variable in BrowserToolbar that's set to the search string after the AwesomeBar is closed, and this could be set with an additional method (setTitleCue?). When setTitle is called, it could set this variable to null. Then any time we call refresh(), it would first check the visual cue variable, and if null, fall back to the tab's title.

But this can be done in a follow-up (if it's worth doing at all).

::: mobile/android/base/BrowserToolbar.java
@@ +509,5 @@
>          return translation;
>      }
>  
> +    public void fromAwesomeBarSearch(String url) {
> +        // Update the title with the url that was just entered. Don't update the title if 

Nit: trailing space

@@ +510,5 @@
>      }
>  
> +    public void fromAwesomeBarSearch(String url) {
> +        // Update the title with the url that was just entered. Don't update the title if 
> +        // the AweseomeBar activity was cancelled, or if the user entered an empty string.

Nit: AweseomeBar -> AwesomeBar

Comment 15

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #14)

> One thing worth noting is that anything that triggers a refresh (e.g., an
> orientation change) will reset the URL bar text. This may not be a problem,
> but one possible fix would be to create a member variable in BrowserToolbar
> that's set to the search string after the AwesomeBar is closed, and this
> could be set with an additional method (setTitleCue?). When setTitle is
> called, it could set this variable to null. Then any time we call refresh(),
> it would first check the visual cue variable, and if null, fall back to the
> tab's title.
> 
> But this can be done in a follow-up (if it's worth doing at all).

This sounds like a good follow-up. I also noticed that if you enter a URL before gecko is loaded, we get a locationchange event for about:home before navigating to the new URL, which clears the text that you had entered. This could also be a good candidate for a follow-up, although I'm not sure exactly how we would fix that.

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dafe434d86a

Filed bug 846050 as a follow-up.

Comment 17

[Gregory Szorc [:gps]]

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