1393579 - AS top sites: what happens when you have a page with a path but don't have a page title?

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, enhancement, P1)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: URLs in top sites (yahoo search results)

In bug 1390372, we determined, for top sites, if you have a page with a path, you should show a page title instead of "subdomain.domain", which is the standard for other top sites.

However, we didn't have specifications for when there is no page title so I opted to use the URL and it doesn't look great.

Bryan, Aaron, what do you think we should do? Should we just fall back to "subdomain.domain" if there's no page title?

btw, ignore the debug lines in the screenshots please! :)

Comment 1

[bbell]

> Bryan, Aaron, what do you think we should do? Should we just fall back to
> "subdomain.domain" if there's no page title?

Going back to "subdomain.domain" seems like the most reasonable thing to do for these odd ball pages with no title.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This bug is simple: update http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java#131 to check if the title is empty before entering the first branch, else enter the second. And update the comment so it doesn't mirror the if statement (just explains why we want the results we do).

However, bug 1391413 modifies this statement so we should wait for it to land first.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Easy fix: let's rank it up.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1393579: Show subdomain.domain for pages without titles in top sites.

Review commit: https://reviewboard.mozilla.org/r/178958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178958/

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8907282 [details]
Bug 1393579: Show subdomain.domain for pages without titles in top sites.

https://reviewboard.mozilla.org/r/178958/#review184642

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:135
(Diff revision 1)
>          // site titles were written for the old top sites, where we had more room to display titles: we want
>          // to provide them with more lines. However, it's complex to distinguish a distribution intended for
>          // the old top sites and the new one so for code simplicity, we allow all distributions more lines for titles.
>          title.setMaxLines(isSiteSuggestedFromDistribution ? 2 : 1);
>  
> -        // At a high level, the logic is: if the path non-empty or the site is suggested by a distribution, use the page
> +        // From a UX perspective, we use titles because people refer to domains by their name ("it's on wikipedia")

This comment is useful, but I think it could be better if it was prefaced with something like "this is our preferred ordering of what is shown" and followed by an ordered list or something along those lines, so it's more explicit.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java:145
(Diff revision 1)
> -        // be shown. If we ever want better top site titles, we could create a heuristic to pull the title from parts
> -        // of the URL, page title, etc.
> -        if (wasException ||
> +        //
> +        // We use page titles with distributions because that's what those distributions expect to be shown.
> +        final String pageTitle = topSite.getTitle();
> +        if (isInvalidURI ||
>                  isSiteSuggestedFromDistribution ||
> -                !URIUtils.isPathEmpty(topSiteURI)) {
> +                (!URIUtils.isPathEmpty(topSiteURI) && !TextUtils.isEmpty(pageTitle))) {

I would break this link into a separate "else if" with an explanation because it's a little confusing that the pageTitle emptiness is being checked again - I'd sacrifice a tiny bit of efficiency for a more readable case statement.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8907282 [details]
Bug 1393579: Show subdomain.domain for pages without titles in top sites.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178958/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72a9bca22654
Show subdomain.domain for pages without titles in top sites. r=liuche

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72a9bca22654