1301718 - Update Activity Stream Highlights page icons with PageMetadata's image_url

Status: RESOLVED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

As part of the metadata extraction (bug 1301715) we get the URL of an image. We want to later use this image in the UI (e.g. for highlights). This bug is about actually getting and storing this image.

We extract (bug 1301715) and store (bug 1301715) the metadata while browsing. In this situation we often already loaded the image. Can we get the image from Gecko without loading it in "Java world" again? Can we piggyback on Gecko's cache or do we need to keep our own cache (duplicate)? And how do we want to keep the cache small?

Comment 1

[Richard Newman [:rnewman]]

Quick driveby: AIUI desktop is planning on storing this in a separate SQLite DB, using ATTACH to join it with Places data. I don't disagree with that approach. Marco Bonardo can fill in the details if you're interested.

Comment 2

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

To add more context here:
- Page metadata is accessible in Java via a ContentProvider (implemented in bug 1301717)
- The metadata is stored in the DB as a JSON String and that JSON contains a field image_url for the image url.
- bug 1322501 blocks this and is about getting the image from Gecko -> Java (whereas this bug is strictly about storing that transferred image)
- bug 1335819 also blocks this but it's considered a "nicetohave" and I'm unclear what it entails. At the very least we'll need to read the image metadata to get the size in that bug.

Comment 3

[:Grisha Kruglov]

(In reply to Michael Comella (:mcomella) from comment #2)
> - bug 1335819 also blocks this but it's considered a "nicetohave" and I'm
> unclear what it entails. At the very least we'll need to read the image
> metadata to get the size in that bug.

That dependency is either backwards, or not needed at all (depending on how we end-up determining image size).

Comment 4

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

I moved the storing to bug 1322501 - let's make this bug about updating the UI with the correct image.

Comment 5

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

Attachment: Bug 1301718: FaviconView extends AppCompatImageView.

This change is unrelated to this bug.

According to my IDE warning:

```
This custom view should extend android.support.v7.widget.AppCompatImageView
instead.

In order to support features such as tinting, the appcompat library will
automatically load special appcompat replacements for the builtin widgets.
However, this does not work for your custom views.

Instead of extending the android.widget classes directly, you should instead
extend one of the delegate classes in android.support.v7.widget.AppCompat.
```

Seems reasonable and I don't see any documentation telling me not to. :)

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

Comment 6

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

Attachment: Bug 1301718: Add StreamPageIconLayout.

This will be used by highlights, top sites, and the AS bottom sheet to set
their icons.

I chose to use centerCrop, over centerInside, because the mocks look like they
chose to do a center crop scale pattern:
https://mozilla.invisionapp.com/share/5JCHCGXPG#/screens/224085516

To my reviewer: some of this code comes from the next commit, where it is moved
from the Highlights code to this file.

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

Comment 7

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

Attachment: Bug 1301718: Use StreamPageIconLayout in Highlights.

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

Comment 8

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

Attachment: Bug 1301718: Use StreamPageIconLayout in top sites.

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

Comment 9

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

Attachment: Bug 1301718: Use StreamPageIconLayout in bottom sheet context menu.

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

Comment 10

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

Note to self: we may want to be nice to Sebastian and wait for bug 1383735 to land before landing this. :)

Comment 11

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

Attachment: Screenshot post-patch (1 of 2)

fwiw, this didn't come out super great:
- We still don't have images for some sites (e.g. rockpapershotgun.com) so their tiny favicons look really bad next to the fullsize favicons
- Some pages give a cover photo that's wide (like the top of an article): when we center crop, these can look bad (e.g. gizmodo)

We should revisit our image_url strategy based on how our cover photos are supposed to look.

Comment 12

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

Attachment: Screenshot post-patch (2 of 2)

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894028 [details]
Bug 1301718: Use StreamPageIconLayout in top sites.

https://reviewboard.mozilla.org/r/165130/#review170692

From what I've seen I do not think this is correct: An image (if available) should be used in the highlights section but not for top sites (-> Full size icon + color only).

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8894025 [details]
Bug 1301718: FaviconView extends AppCompatImageView.

https://reviewboard.mozilla.org/r/165124/#review172380

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8894026 [details]
Bug 1301718: Add StreamPageIconLayout.

https://reviewboard.mozilla.org/r/165126/#review172964

lgtm

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamPageIconLayout.java:64
(Diff revision 1)
> +        cancelPendingRequests();
> +
> +        if (!TextUtils.isEmpty(overrideImageURL)) {
> +            setUIMode(UIMode.NONFAVICON_IMAGE);
> +
> +            // TODO (bug 1322501): Optimization: since we've already navigated to these pages, there's a chance

Good call, let's get this working and then focus on hitting the cache later.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8894026 [details]
Bug 1301718: Add StreamPageIconLayout.

https://reviewboard.mozilla.org/r/165126/#review172966

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8894027 [details]
Bug 1301718: Use StreamPageIconLayout in Highlights.

https://reviewboard.mozilla.org/r/165128/#review172968

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8894029 [details]
Bug 1301718: Use StreamPageIconLayout in bottom sheet context menu.

https://reviewboard.mozilla.org/r/165132/#review172972

Just updating calls to new class, r+.

Comment 19

[Chenxia Liu [:liuche]]

Comment on attachment 8894028 [details]
Bug 1301718: Use StreamPageIconLayout in top sites.

clearing this review flag until the top sites problem is fixed.

Comment 20

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

Comment on attachment 8894031 [details]
Screenshot post-patch (1 of 2)

I'm going to drop the top sites commit. To avoid confusion, let's take out the screenshot with top sites.

Comment 21

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

Comment on attachment 8894025 [details]
Bug 1301718: FaviconView extends AppCompatImageView.

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

Comment 22

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

Comment on attachment 8894026 [details]
Bug 1301718: Add StreamPageIconLayout.

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

Comment 23

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

Comment on attachment 8894027 [details]
Bug 1301718: Use StreamPageIconLayout in Highlights.

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

Comment 24

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

I'm going to push the first three commits because I'll need additional review in order to:
- Rename StreamPageIconLayout (it's not for all stream icons now)
- Correct how we use ^ in our BottomSheetContextMenu

Comment 25

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

Actually, seems cleaner to file a new bug 1390356.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9cf0fc9bfc3
FaviconView extends AppCompatImageView. r=liuche
https://hg.mozilla.org/integration/autoland/rev/accde04cd0a5
Add StreamPageIconLayout. r=liuche
https://hg.mozilla.org/integration/autoland/rev/d662a65a2034
Use StreamPageIconLayout in Highlights. r=liuche

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9cf0fc9bfc3
https://hg.mozilla.org/mozilla-central/rev/accde04cd0a5
https://hg.mozilla.org/mozilla-central/rev/d662a65a2034