Closed Bug 1390356 Opened 2 years ago Closed 2 years ago

Rename StreamPageIconLayout; use in BottomSheetContextMenu

Categories

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

All
Android
enhancement

Tracking

()

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

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Whiteboard: [mobileAS] 1.28)

Attachments

(4 files, 5 obsolete files)

59 bytes, text/x-review-board-request
liuche
: review+
Details
59 bytes, text/x-review-board-request
liuche
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Follow-up to bug 1301718 - it'll require another review cycle so I thought it'd be easier to file a new bug.

Let's put it in the current iteration.
Assignee: nobody → michael.l.comella
Attachment #8899638 - Attachment is obsolete: true
Attachment #8899638 - Flags: review?(s.kaspari)
Attachment #8899639 - Attachment is obsolete: true
Attachment #8899639 - Flags: review?(s.kaspari)
Attachment #8899640 - Attachment is obsolete: true
Attachment #8899640 - Flags: review?(s.kaspari)
Attachment #8899641 - Attachment is obsolete: true
Attachment #8899641 - Flags: review?(s.kaspari)
Attachment #8899642 - Attachment is obsolete: true
Attachment #8899642 - Flags: review?(s.kaspari)
Comment on attachment 8897587 [details]
Bug 1390356: StreamPageIconLayout -> StreamOverridablePageIconLayout.

https://reviewboard.mozilla.org/r/168850/#review176978
Attachment #8897587 - Flags: review?(liuche) → review+
Comment on attachment 8897588 [details]
Bug 1390356: Use StreamOverridablePageIconLayout in BottomSheetContextMenu.

https://reviewboard.mozilla.org/r/168852/#review176980

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/Item.java:21
(Diff revision 3)
> +     *
> +     * This operation could be slow in some implementations (see {@link Highlight#getMetadataSlow()}), hence the name.
> +     * imo, it is better to expose this possibility in the interface for all implementations rather than hide this fact.
> +     */
> +    @NonNull
> +    Metadata getMetadataSlow();

I don't think an Item should need to expose Metadata - what you want this for is the imageurl, so you should just expose that, imo. It doesn't seem to follow that for Items in the new tab page, they *need* metadata, so I think we should keep the interface minimal.

disclaimer: I'm also modifying this code for Pocket, which has no metadata :P but I think it reinforces the idea that interfaces should be forward-compatible and minimal!
Attachment #8897588 - Flags: review?(liuche) → review+
Comment on attachment 8900449 [details]
Bug 1390356 - review: getMetadataSlow -> getImageUrl in Item interface.

https://reviewboard.mozilla.org/r/171806/#review177024

r=trivial
Attachment #8900449 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72a0d4fe9278
StreamPageIconLayout -> StreamOverridablePageIconLayout. r=liuche
https://hg.mozilla.org/integration/autoland/rev/e4d9551839b6
Use StreamOverridablePageIconLayout in BottomSheetContextMenu. r=liuche
https://hg.mozilla.org/integration/autoland/rev/864d9c092eaf
review: getMetadataSlow -> getImageUrl in Item interface. r=mcomella
Comment on attachment 8900480 [details]
Bug 1390356 - bustage: Use proper context menu constructor in tests.

https://reviewboard.mozilla.org/r/171846/#review177082

r=trivial
Attachment #8900480 - Flags: review?(michael.l.comella) → review+
Flags: needinfo?(michael.l.comella)
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/63d92658ad81
StreamPageIconLayout -> StreamOverridablePageIconLayout. r=liuche
https://hg.mozilla.org/integration/autoland/rev/ea2757930c48
Use StreamOverridablePageIconLayout in BottomSheetContextMenu. r=liuche
https://hg.mozilla.org/integration/autoland/rev/7acd335e0d89
review: getMetadataSlow -> getImageUrl in Item interface. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/2df131583000
bustage: Use proper context menu constructor in tests. r=mcomella
You need to log in before you can comment on or make changes to this bug.