1325380 - Use 'provider name' metadata for AS panel

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1311434 introduced the rule for collecting the "provider name" from the website's metadata. Now let's use it for the AS panel.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1325380 - Use provider name for AS highlights.

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

Comment 2

[:Grisha Kruglov]

Comment on attachment 8821188 [details]
Bug 1325380 - Use provider name for AS highlights.

https://reviewboard.mozilla.org/r/100530/#review101110

Looks good. I've added some thoughts on how we might evolve this pattern over time, which might be a good follow-up.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1201
(Diff revision 1)
> +                DBUtils.qualifyColumn(PageMetadata.TABLE_NAME, PageMetadata.JSON) + " AS " + Highlights.METADATA + " " +
>                  "FROM " + Bookmarks.TABLE_NAME + " " +
>                  "LEFT JOIN " + History.TABLE_NAME + " ON " +
>                      DBUtils.qualifyColumn(Bookmarks.TABLE_NAME, Bookmarks.URL) + " = " +
>                      DBUtils.qualifyColumn(History.TABLE_NAME, History.URL) + " " +
> +                "LEFT JOIN " + PageMetadata.TABLE_NAME + " ON " +

It might be worth noting in a comment that the way we currently insert page metadata is via INSERT OR REPLACE, ensuring that it's a 1x1 relationship between history and pagemetadata.

That is why a simple left join without a group by works here.

::: mobile/android/base/java/org/mozilla/gecko/home/activitystream/StreamItem.java:292
(Diff revision 1)
> +                if (metadata != null && metadata.has("provider")) {
> +                    vPageView.setText(metadata.getString("provider"));
> +                    return;
> +                }
> +            } catch (JSONException e) {
> +                // Broken JSON? Continue with fallback.

I wonder how clumsy dealing with json metadata will get once we start doing more of this.

It might be helpful to think of this in terms of a model-view relationship, where we have a "highlight model", which is instantiated with various highlight data from the cursor, and, for example, provides a "displayName" method which encapsulates various ways to obtain a suitable display name: try provider, fallback to X, fallback to Y...

We'll be doing this sort of a thing for highlight images very soon, and I would imagine for more and more bits of data as time goes on.

This will be nicely testable and should help keep view logic free from data stuff.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, I agree. In addition to that the whole StreamItem file gets more and more convoluted. It's time to break it up - I'll look at this in a follow-up bug.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8821188 [details]
Bug 1325380 - Use provider name for AS highlights.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8702f15c7d2e
Use provider name for AS highlights. r=Grisha

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/8702f15c7d2e