Closed Bug 1301718 Opened 8 years ago Closed 7 years ago

Update Activity Stream Highlights page icons with PageMetadata's image_url

Categories

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

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: sebastian, Assigned: mcomella)

References

Details

(Whiteboard: [MobileAS] 1.26 1.27)

Attachments

(4 files, 3 obsolete files)

Bug 1301718: FaviconView extends AppCompatImageView.
(deleted), text/x-review-board-request
liuche
: review+
Details
Bug 1301718: Add StreamPageIconLayout.
(deleted), text/x-review-board-request
liuche
: review+
Details
Bug 1301718: Use StreamPageIconLayout in Highlights.
(deleted), text/x-review-board-request
liuche
: review+
Details
Screenshot post-patch (2 of 2)
(deleted), image/png
Details
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).
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
Assignee: nobody → michael.l.comella
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) (deleted) —
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
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
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

Creator:
Created:
Updated:
Size: