Closed Bug 1382036 Opened 7 years ago Closed 7 years ago

Follow iOS algorithm for top sites title query: get provider from database

Categories

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

All
Android
enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Whiteboard: [mobileAS] 1.27)

Attachments

(1 file)

The TopSites item title in iOS looks like [1]:

        if let provider = site.metadata?.providerName {
            titleLabel.text = provider.lowercased()
        } else {
            titleLabel.text = site.tileURL.hostSLD
        }

In bug 1377287, we only implemented the latter, the hostSLD. The provider is stored in the database so, because it's a lot of extra work and I'm stumbling through it, I figured it'd be more efficient to break it out into a separate bug. Also, it may relate to bug 1369604 (highlights query is slow) because suddenly we'll have to join the page metadata table with the top sites table.

[1]: https://github.com/mozilla-mobile/firefox-ios/blob/deb9736c905cdf06822ecc4a20152df7b342925d/Client/Frontend/Home/ActivityStreamTopSitesCell.swift#L147
tracking-fennec: --- → ?
Whiteboard: [mobileAS]
I flagged this for triage, instead of fixing it this sprint (even though it's a follow-up) because it feels low priority to me, compared to the other work that we have to do. In practice, fixing this bug will make the top sites tiles' titles change like so:

google -> accounts google
mozilla -> bugzilla mozilla
rockpapershotgun -> rock paper shotgun

With favicons, this isn't too bad in most cases (e.g. bugzilla.mozilla.org would have "mozilla" as the title but the bugzilla favicon). Most providers also won't change at all:

kotaku -> kotaku
apple -> apple

There will usually only be benefits for domains with subdomains (e.g. accounts.google.com) because it gives the website the opportunity to express the subdomain in the top sites titles.

---

Though now that I've been digging through the DB, this might not be as hard as I initially thought it would be - performance may still be a concern, however and working on bug 1369604 first may give me more context.
tracking-fennec: ? → ---
Priority: -- → P2
This is borderline P2/P3 – we don't have a good sense of what the other P2 bugs are so let's compare it against them.
Iteration: --- → 1.27
Priority: P2 → P1
Whiteboard: [mobileAS] → [mobileAS] 1.27
Assignee: nobody → michael.l.comella
Not as simple as I thought I remembered that this was borderline P2/P3.
Assignee: michael.l.comella → nobody
Assignee: nobody → michael.l.comella
Comment on attachment 8893156 [details]
Bug 1382036: Use provider in top sites tile.

https://reviewboard.mozilla.org/r/164158/#review170478

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopSite.java:48
(Diff revision 1)
>          this.url = url;
>          this.title = title;
>          this.isBookmarked = isBookmarked;
>          this.isPinned = type == BrowserContract.TopSites.TYPE_PINNED;
>          this.type = type;
> +        this.metadata = metadata;

Why did you decide to pass around metadata, rather than the provider?

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopSite.java:77
(Diff revision 1)
>  
>      public Boolean isPinned() {
>          return isPinned;
>      }
>  
> +    public Metadata getMetadata() {

What kind of data is present in Metadata?

On the other hand I understand that maybe you don't want to document that here though, because whoever changes Metadata won't necessarily change a comment here, and people can just go to the declaration of Metadata.
Attachment #8893156 - Flags: review?(liuche) → review+
Comment on attachment 8893156 [details]
Bug 1382036: Use provider in top sites tile.

https://reviewboard.mozilla.org/r/164158/#review170478

> Why did you decide to pass around metadata, rather than the provider?

I guess I didn't think about it very much. :P

Thinking about it now, there can be an argument either way: 1) we have the data, why not expose it? or 2) Don't expose what you don't need. A plus for 1, in this case, is that we need to pull the data out of the DB anyway because it's a JSON string so it's more work to hide the data than to expose its data class.

I'm going to leave it the way it is (expose the data we have) because the same pattern is used on Highlights items.

> What kind of data is present in Metadata?
> 
> On the other hand I understand that maybe you don't want to document that here though, because whoever changes Metadata won't necessarily change a comment here, and people can just go to the declaration of Metadata.

Basically the content of the `PageMetadata` table: provider, imageUrl, descriptionLength.

tbh, it's ambiguously named (yeah, what the hell is metadata anyway? Why is it a separate table from BrowserDB?) and perhaps I should have done better than following existing convention.

As you mention, I don't think a comment would help here - I think it's best for devs to look at the definition of the class themselves.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a5ea44262c9b -d 845dd7955f8a: rebasing 411929:a5ea44262c9b "Bug 1382036: Use provider in top sites tile. r=liuche" (tip)
merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Highlight.java
merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Metadata.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Highlight.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Metadata.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5d6714d6cd74
Use provider in top sites tile. r=liuche
FYI: bug 1386902 is going to replace this. I won't back this out to avoid churn.
https://hg.mozilla.org/mozilla-central/rev/5d6714d6cd74
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: