909375 - Regression: Use website URL when title not available for about:home item context menu header

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: New | Old - Screenshot

Currently listed sites without available titles to use do not expose a menu item header in the about:home context menu. In the old about:home, the URL was just used.

See screenshot

URL I used for testing; http://account.live.com which does a redirect and the entry gets added to our places database

--
Nightly (08/26) |  LG Nexus 4 (Android 4.3)

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I stole some piece of code from shortcut title creation. This removes "http(s)://www." from the urls when showing as title -- same as shortcut title.

Comment 2

[:Margaret Leibovic]

Comment on attachment 795747 [details] [diff] [review]
Patch

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

This gets the job done, but I think it can be better :)

::: mobile/android/base/home/HomeFragment.java
@@ +85,5 @@
>  
>          MenuInflater inflater = new MenuInflater(view.getContext());
>          inflater.inflate(R.menu.home_contextmenu, menu);
>  
> +        final String headerTitle = TextUtils.isEmpty(info.title) ? info.url.replaceAll(REGEX_URL_TO_TITLE, "") : info.title;

This is exactly the same as the line to make shortcutTitle, and I think it's complex enough that it makes sense to pull it out into a helper method, maybe something like getDisplayTitle().

Or should we consider setting the title like this in the HomeContextMenuInfo constructor? It looks like this empty title problem might also be an issue for launching a share intent:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#128

Unless you foresee any problems with that, I think that would actually be the best solution.

Comment 3

[Sriram Ramasubramanian [:sriram]]

This is not for share title. I thought of moving it to a helper method. But then its a bit extra work for a one-liner. So went ahead and added that one line here. Do you really want a helper method for a one-liner?

Comment 4

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> This is not for share title. I thought of moving it to a helper method. But
> then its a bit extra work for a one-liner. So went ahead and added that one
> line here. Do you really want a helper method for a one-liner?

I know this fix isn't for the share title, but I was wondering if that is another bug that we could address with a more general fix here. We currently only use info.title for the shortcut title, this header title, and the share intent title, so if we want them to all do the same thing, I see an opportunity for us to just fix them all in one place (the HomeContextMenuInfo constructor).

In fact, I think we should do this, since this is what we're already doing for the main menu share item:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#826

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This is so clean - Hope
I had already seen!

Comment 6

[:Margaret Leibovic]

Comment on attachment 796299 [details] [diff] [review]
Patch

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

Nice, this is what I had in mind.

Comment 7

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/fx-team/rev/e80e45a7f211
https://hg.mozilla.org/integration/fx-team/rev/51cfe3cb90c6
Wrong bug number :( Still can't figure out where I got that bug number from! :O

https://hg.mozilla.org/integration/fx-team/rev/1e421d1e1f3b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e421d1e1f3b