Closed Bug 1301718 Opened 3 years ago Closed 3 years ago

Update Activity Stream Highlights page icons with PageMetadata's image_url

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.28
Tracking Status
firefox57 --- fixed

People

(Reporter: sebastian, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [MobileAS] 1.26 1.27)

Attachments

(4 files, 3 obsolete files)

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?
Priority: -- → P2
Whiteboard: [MobileAS]
No longer blocks: 1301715
Depends on: 1301715
Priority: P2 → P3
Priority: P3 → P2
Priority: P2 → P3
Depends on: 1322501
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.
Priority: P3 → P2
Depends on: 1335819
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.
(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).
Blocks: 1335819
No longer depends on: 1335819
Rank: 1
Whiteboard: [MobileAS] → [MobileAS] 1.26
Iteration: --- → 1.26
Priority: P2 → P1
Iteration: 1.26 → 1.27
Whiteboard: [MobileAS] 1.26 → [MobileAS] 1.26 1.27
No longer depends on: 1386052
Assignee: nobody → michael.l.comella
Blocks: 1322501
No longer depends on: 1322501
I moved the storing to bug 1322501 - let's make this bug about updating the UI with the correct image.
Summary: Store image from website metadata → Update Activity Stream page icons with PageMetadata's image_url
Note to self: we may want to be nice to Sebastian and wait for bug 1383735 to land before landing this. :)
Attached image Screenshot post-patch (1 of 2) (obsolete) —
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 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).
Attachment #8894028 - Flags: review-
Comment on attachment 8894025 [details]
Bug 1301718: FaviconView extends AppCompatImageView.

https://reviewboard.mozilla.org/r/165124/#review172380
Attachment #8894025 - Flags: review?(liuche) → review+
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 on attachment 8894026 [details]
Bug 1301718: Add StreamPageIconLayout.

https://reviewboard.mozilla.org/r/165126/#review172966
Attachment #8894026 - Flags: review?(liuche) → review+
Comment on attachment 8894027 [details]
Bug 1301718: Use StreamPageIconLayout in Highlights.

https://reviewboard.mozilla.org/r/165128/#review172968
Attachment #8894027 - Flags: review?(liuche) → review+
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+.
Attachment #8894029 - Flags: review?(liuche) → review+
Comment on attachment 8894028 [details]
Bug 1301718: Use StreamPageIconLayout in top sites.

clearing this review flag until the top sites problem is fixed.
Attachment #8894028 - Flags: review?(liuche)
Iteration: 1.27 → 1.28
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.
Attachment #8894031 - Attachment is obsolete: true
Attachment #8894028 - Attachment is obsolete: true
Attachment #8894029 - Attachment is obsolete: true
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
Keywords: leave-open
Actually, seems cleaner to file a new bug 1390356.
Blocks: 1390356
Keywords: leave-open
Pushed by michael.l.comella@gmail.com:
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
Summary: Update Activity Stream page icons with PageMetadata's image_url → Update Activity Stream Highlights page icons with PageMetadata's image_url
You need to log in before you can comment on or make changes to this bug.