1187538 - *_tabs_item_cell does not use gecko:state_recording

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

And thus does not highlight tab thumbnails when they're being streamed.

We use @drawable/tabs_thumbnail to set the background colors [1].

In new_tablet_tabs_item_cell, we wrap the thumbnail in a RelativeLayout while in tabs_item_row (which is used on phone), we wrap it in TabThumbnailWrapper, which supports the gecko:state_recording state.

Finkle, is this something to be concerned about? I assume streaming is tab casting and that it's unclear how much this feature is used.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=%40drawable%2Ftab_thumbnail&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

The rabbit hole was not too deep here:

1. TabThumnailWrapper.setRecording() is set by Tab.isRecording()
2. Tab.setRecording() is set via "Tab:StreamStart" and "Tab:StreamStop" messages from TabSource.js

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#579

This means that state_recording seems to be driven by Tab Mirroring. I was wondering if it was also related to WebRTC in general, but it seems to be only set via Tab Mirroring.

Should we fix this? Yes.
Will we someday remove it? Maybe, but that day is not today.

We also have bug 1018504 to support showing state_audio (or whatever) so we should make sure the UI and code are able to display different states.

Comment 2

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

afaict, this also affects tabs_item_cell.xml.

Comment 3

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

It looks like the simplest solution would be to change the containing RelativeLayout to a TabsThumbnailWrapper and change TabsThumbnailWrapper to extend ThemedRelativeLayout. There would be a slight perf hit during measure (I think) in the tabs_item_row layout since we'd be moving from a FrameLayout, but it's likely negligible.

Comment 4

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

Attachment: MozReview Request: Bug 1187538 - Wrap tab tray thumbnails in TabThumbnailWrapper and add id. r=mhaigh

Bug 1187538 - Wrap tab tray thumbnails in TabThumbnailWrapper and add id. r=mhaigh

I don't have a device to screen cast to test this makes state_recording work in
*_tabs_item_cell, but following the code, the bits to turn state_recording on
or off are in place in TabsListLayout and presumably working for tabs_item_row,
so it should automatically work for tabs_item_cell, and the same bits are in
TabsGridLayout so this should presumably work for new_tablet_tabs_item_cell.

I verified the changes do not cause crashing or a difference in appearance.

Comment 5

[Martyn Haigh (:mhaigh)]

Comment on attachment 8640031 [details]
MozReview Request: Bug 1187538 - Wrap tab tray thumbnails in TabThumbnailWrapper and add id. r=mhaigh

https://reviewboard.mozilla.org/r/14279/#review13055

This seems sane, I don't think the perf hit is something we have to worry about.

Comment 6

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

url:        https://hg.mozilla.org/integration/fx-team/rev/95fa7f919da334c292d9181ee4b2130e2baab023
changeset:  95fa7f919da334c292d9181ee4b2130e2baab023
user:       Michael Comella <[e-mail]>
date:       Tue Jul 28 11:58:25 2015 -0700
description:
Bug 1187538 - Wrap tab tray thumbnails in TabThumbnailWrapper and add id. r=mhaigh

I don't have a device to screen cast to test this makes state_recording work in
*_tabs_item_cell, but following the code, the bits to turn state_recording on
or off are in place in TabsListLayout and presumably working for tabs_item_row,
so it should automatically work for tabs_item_cell, and the same bits are in
TabsGridLayout so this should presumably work for new_tablet_tabs_item_cell.

I verified the changes do not cause crashing or a difference in appearance.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95fa7f919da3