1390356 - Rename StreamPageIconLayout; use in BottomSheetContextMenu

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Bug 1390356: StreamPageIconLayout -> StreamOverridablePageIconLayout.

Now it doesn't imply it's used everywhere for page icons in activity stream.

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

Comment 2

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

Attachment: Bug 1390356: Use StreamOverridablePageIconLayout in BottomSheetContextMenu.

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

Comment 3

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

Attachment: Bug 1390356: Do not focus tap-to-dismiss BottomSheetDialog view with Talkback.

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

Comment 4

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

Attachment: Bug 1390356: Do not focus BottomSheetDialog in Talkback.

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

Comment 5

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

Attachment: Bug 1390356: Do not focus AS context menu titles in Talkback.

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

Comment 6

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

Attachment: Bug 1390356: Add long-click support to Highlights for Talkback users.

The menu button is hard to find so we decided to add support for long-click on
the whole row, which is consistent with top sites.

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

Comment 7

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

Attachment: Bug 1390356: Override AS context menu a11y title.

In theory, we could block the UI thread for the page domain and with the
current implementation, it'd be fine:
- It's async only because the first time this method is called, it reads a file
from disk. Otherwise, it's unnecessary.
- This method is guaranteed to have already been called because about:home
shows before this context menu can show

But the implementation could change and it seemed incorrect to block showing
this context menu for *all* solely for a11y text that can be estimated.

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

Comment 8

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

Comment on attachment 8897587 [details]
Bug 1390356: StreamPageIconLayout -> StreamOverridablePageIconLayout.

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

Comment 9

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

Comment on attachment 8897588 [details]
Bug 1390356: Use StreamOverridablePageIconLayout in BottomSheetContextMenu.

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

Comment 10

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

Comment on attachment 8897587 [details]
Bug 1390356: StreamPageIconLayout -> StreamOverridablePageIconLayout.

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

Comment 11

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

Comment on attachment 8897588 [details]
Bug 1390356: Use StreamOverridablePageIconLayout in BottomSheetContextMenu.

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

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8897587 [details]
Bug 1390356: StreamPageIconLayout -> StreamOverridablePageIconLayout.

https://reviewboard.mozilla.org/r/168850/#review176978

Comment 13

[Chenxia Liu [:liuche]]

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!

Comment 14

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

Attachment: Bug 1390356 - review: getMetadataSlow -> getImageUrl in Item interface.

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

Comment 15

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

Comment on attachment 8900449 [details]
Bug 1390356 - review: getMetadataSlow -> getImageUrl in Item interface.

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

r=trivial

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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 17

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

I had to back these out for android build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=125306257&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/8edd99a178f1

Comment 18

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

Attachment: Bug 1390356 - bustage: Use proper context menu constructor in tests.

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

Comment 19

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

Comment on attachment 8900480 [details]
Bug 1390356 - bustage: Use proper context menu constructor in tests.

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

r=trivial

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/63d92658ad81
https://hg.mozilla.org/mozilla-central/rev/ea2757930c48
https://hg.mozilla.org/mozilla-central/rev/7acd335e0d89
https://hg.mozilla.org/mozilla-central/rev/2df131583000