928096 - UI for Tab streaming

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_sharing_fennec.patch

bug 742832 implements tab streaming, but leaves the UI for the frontend to implement. I've attached an example implementation that implements the service that bug 742832 looks for, but simply returns the first open tab rather than presenting any UI to the user.

need-info to Ian for a UX spec

Comment 1

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

Ian - What UI would you like to see here?

Jesup - What are the security issues we would need to deal with?

Comment 2

[Ian Barlow (:ibarlow)]

This is a sketch of what Brad and I talked about the other day -- basically the same interface as the microphone/video one, but with a spinner menu that contains a list of your open tabs to choose from. http://cl.ly/image/1B3W1y1U220t

If only one tab is open, then we can use the same interface, just without the spinner box, and adapt the string to read "Would you like to share this tab with [page]?

--

One thing I'm not clear on here though is whether this would be a choice on its own like I've sketched above, or if there would actually be cases where a user would be presented with an option to share video, audio, and/or an open tab all at once -- http://cl.ly/image/2F3a0E0F150S -- which might feel a little heavy?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #2)
> This is a sketch of what Brad and I talked about the other day -- basically
> the same interface as the microphone/video one, but with a spinner menu that
> contains a list of your open tabs to choose from.
> http://cl.ly/image/1B3W1y1U220t
> 
> If only one tab is open, then we can use the same interface, just without
> the spinner box, and adapt the string to read "Would you like to share this
> tab with [page]?
> 
> --
> 
> One thing I'm not clear on here though is whether this would be a choice on
> its own like I've sketched above, or if there would actually be cases where
> a user would be presented with an option to share video, audio, and/or an
> open tab all at once -- http://cl.ly/image/2F3a0E0F150S -- which might feel
> a little heavy?

Neither of those exactly. The tab sharing is essentially an additional camera. So it should be something like this:


What video do you want to share?
    |________________________________ 
                                     | Front Camera
                                     | Back Camera
                                     | Share Tab > ____________
                                     | None                    | google.com
                                                               | slideshare.com
What audio do you want to share?
    |________________________________
                                     | Internal Microphone
                                     | None

Comment 4

[Ian Bicking (:ianbicking)]

These proposed interfaces are very peculiar to me.  It seems to me that what this interface needs to do is:

1. Explain what the site is asking for
2. Get the user to accurately select something
3. Make sure the user doesn't forget what they've enabled
4. Give the user a way to terminate the sharing
5. Notify the user if the sharing terminates on its own

This is complicated by the fact that the consumer of the tab sharing is in one tab, and the tab producing content is in another tab, and the user needs to be switching between the two of them if they are going to do anything interesting.

More thoughts on some of these items:

1a. One way to explain what they are asking for is to show what is being asked for.  For instance, the tab selector could show thumbnails.  The user *isn't* just sharing a URL.
1b. The user isn't sharing a site, they are sharing a tab.  That's stickier than a site, and longer lived than any one page.
1c. To what end is the user sharing the tab?  Here we hit up against the unwillingness to put any site-supplied text in browser dialog, so I don't know if there's a solution.  But "tab sharing" isn't what a person is going to be thinking going into this.  The actual end goal of a site would be something like recording a video of an interaction, or sharing a stream of a tab with another person, or submitting a bug with an accompanying video.  When that dialog pops up how is the user going to understand how the dialog relates to what the site is doing?
2a. There might be multiple tabs with the same title or domain.  And again they are sharing the tab, not the site, so any title or domain is an inaccurate representation of what is being shared.
2b. Can the user literally select a tab?
2c. Can we show the user what they have selected, with a confirmation?  Like, focus the selected tab and throw up a dialog and video camera icon overlay and get explicit confirmation about sharing this tab.

Comment 5

[Ian Barlow (:ibarlow)]

Thanks for the feedback, ianb and Brad. I'm hearing a couple of different things here, so I'd like to call a quick timeout and ask if specific UX requirements for this feature have been called out anywhere (other than this conversation). 

Because Brad's notes make sense to me, and ianb's comments do indeed add up to a better overall experience, but they are not the same as what Brad and I discussed a couple of weeks ago. 

Any additional notes/wikis/etc would be appreciated, so I can make sure I'm designing the right thing.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

+ ekr

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Ian - What UI would you like to see here?
> 
> Jesup - What are the security issues we would need to deal with?

I'd start by reading ekr's IETF document on WebRTC/rtcweb security.
http://tools.ietf.org/html/draft-ietf-rtcweb-security-06#section-4.1.1

The main security risks are moot when we control/trust the capture application and know the destination is not an attacker.

If we control the capture but don't know the destination is safe, then the exposure I can think of would be that if the destination site also controlled an iframe/etc in the page being shared and had some way to know it was being shared, it could flip up sensitive data.  An obvious additional risk is that (of course) the destination could mirror the data (in info about it, screenshots, etc) to a third-party/attacker while appearing to show it normally to the user at the destination side.  (This isn't surprising.)

Note that a site could embed info in the page that seems innocuous but could identify if it's being shared - simple form would just be a text GUID or 2d barcode.  The destination would then key on that and inform the attack code in the shared page to flip an iframe of bank account data on screen (for example).  Normally cross-origin blocking would stop attack code from getting access to the data in the iframe, but now it's being encoded as video and sent.  For extra slime points, it could wait until it doesn't see a face in an accompanying video stream (user looked away).

If we don't control the capture code, it gets more fun. Many of the threats are similar, but the attacker has more levers to use.

Comment 7

[Ian Barlow (:ibarlow)]

Attachment: ffxandroid-wbrtc-tabstreaming-140123_02.png

Here's an updated set of sketches, based on a conversation with Brad and Deb. Couple things of note:

* Since a streamed tab is being considered a video source like a camera, I changed the wording around permissions prompts from "Camera" to "Video source" (and "Audio source" for microphones, for symmetry).
* I've also added some sketches for how a streaming tab could be represented on a page, in the notification bar, and in the tab tray using a coloured border. I'm still on the fence between using red (record) or green (on), but that's something we can iterate on later.

Comment 8

[Lucas Rocha (:lucasr)]

Re-nom'ing for discussion,

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_sharing_fennec.patch

Comment 10

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

Comment on attachment 8390976 [details] [diff] [review]
tab_sharing_fennec.patch

>diff --git a/content/media/webrtc/MediaEngineTabVideoSource.cpp b/content/media/webrtc/MediaEngineTabVideoSource.cpp

This looked fine to me. Simple changes, but I am not really a peer here.

>diff --git a/content/media/webrtc/MediaEngineTabVideoSource.h b/content/media/webrtc/MediaEngineTabVideoSource.h

This looked fine to me. Simple changes, but I am not really a peer here.

>diff --git a/content/media/webrtc/nsITabSource.idl b/content/media/webrtc/nsITabSource.idl

>+  void notifyTabStreaming(in nsIDOMWindow win);
>+  void notifyTabNotStreaming(in nsIDOMWindow win);

Not a fan of the naming. We seem to use a mix of notifyXxx and onXxx style methods in IDL. Let's assume we want the notifyXxx style.

I like having consistent "pairing" of names, especially for starting/stopping type events. I also like simpler names. Let's use:
notifyStreamStart and notifyStreamStop

Lastly, let's use | window | since it won't conflict with any active DOM window variable since this is a component.

>diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
>+                Log.i(LOGTAG, "view is null");
>+                Log.i(LOGTAG, "setRecording(true)");
>+                Log.i(LOGTAG, "setRecording(false)");

Seems like debugging and should be removed

>diff --git a/mobile/android/base/resources/drawable/tab_thumbnail.xml b/mobile/android/base/resources/drawable/tab_thumbnail.xml
Some strange indents in here

>diff --git a/mobile/android/base/resources/layout/tabs_item_cell.xml b/mobile/android/base/resources/layout/tabs_item_cell.xml

Some strange indents in here

>diff --git a/mobile/android/base/resources/layout/tabs_item_row.xml b/mobile/android/base/resources/layout/tabs_item_row.xml

Some strange indents in here

>diff --git a/mobile/android/base/widget/ThumbnailView.java b/mobile/android/base/widget/ThumbnailView.java

> import android.graphics.drawable.Drawable;
> import android.util.AttributeSet;
> import android.widget.ImageView;
>+import org.mozilla.gecko.R;
>+import android.util.Log;

We care about the ordering of imports

>+    public int[] onCreateDrawableState(int extraSpace) {

>+        if (mRecording)
>+            mergeDrawableStates(drawableState, STATE_RECORDING);

Add braces please

>+    public void setRecording(boolean recording) {

>+            Log.i(LOGTAG, "Set recording to: " + recording);

Remove please

>diff --git a/mobile/android/base/widget/TwoWayView.java b/mobile/android/base/widget/TwoWayView.java

>-        Drawable d = a.getDrawable(R.styleable.TwoWayView_android_listSelector);
>+        Drawable d = new PaintDrawable(Color.GREEN); //a.getDrawable(R.styleable.TwoWayView_android_listSelector));

Leftover cruft

>     private void useDefaultSelector() {
>-        setSelector(getResources().getDrawable(
>-                android.R.drawable.list_selector_background));
>+        setSelector(new PaintDrawable(Color.RED));
>+					       //getResources().getDrawable(android.R.drawable.list_selector_background));

Same

>diff --git a/mobile/android/components/TabSource.js b/mobile/android/components/TabSource.js

>+XPCOMUtils.defineLazyModuleGetter(this, "sendMessageToJava",
>+                                  "resource://gre/modules/Messaging.jsm");

No wrapping please

>+TabSource.prototype = {

>+  classDescription: "Fennec Tab Source",
I don't like using "Fennec" in the code. How about "TabSource for Streaming"

>+  QueryInterface: function(aIID)
>+  {
>+    if (!aIID.equals(Ci.nsITabSource) &&    
>+        !aIID.equals(Ci.nsISupports)) {
>+      throw Components.results.NS_ERROR_NO_INTERFACE;
>+    }
>+    return this;
>+  },

We should be able to use the XPCOMUtils version of this impl

>+  getTabToStream: function() {

>+    // First, let's decide on which feed to subscribe

Comment needs to be fixed. "// Choose the tab we want to stream"

>+    while (result == null)
>+      thread.processNextEvent(true);

Add braces

>+    if (result == -1)
>+      return null;

Add braces

>+  },
>+  notifyTabStreaming: function(win) {

Add a blank line between methods

>+    for (var i in tabs) {

var -> let

>+      if (tabs[i].browser.contentWindow == win) {
>+	sendMessageToJava({ type: "Tab:Streaming", tabID: tabs[i].id });

Use spaces for indent and let's use "Tab:StreamStart"

>+  },
>+  notifyTabNotStreaming: function(win) {

Add a blank line between methods

>+    let app = Services.wm.getMostRecentWindow("navigator:browser").BrowserApp;
>+    let tabs = app.tabs;
>+    for (var i in tabs) {

var -> let

>+      if (tabs[i].browser.contentWindow == win) {
>+	sendMessageToJava({ type: "Tab:NotStreaming", tabID: tabs[i].id });

Use spaces for indent and let's use "Tab:StreamStop"


With this patch, do we still show the "share tab" text in the WebRTC permission doorhanger? If we do, I think we need a better string there too.

Since I know the least about TabsTray and TwoWayView, I am sending those parts to Lucas for review.

r- for the cleanup and to figure out the "share tab" string issue.

Comment 11

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

Another question about the WebRTC doorhanger: Should the "share tab" item be first or last in the list? I think it shows up first now, but I think it should be last, since it's not what I would think of as a good default choice.

NI'ing Ian for any input on this comment and a better string for "share tab" in the list.

Comment 12

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Another question about the WebRTC doorhanger: Should the "share tab" item be
> first or last in the list? I think it shows up first now, but I think it
> should be last, since it's not what I would think of as a good default
> choice.
> 
> NI'ing Ian for any input on this comment and a better string for "share tab"
> in the list.

not sure what you mean here -- in the designs, we actually have this in the "video source" spinner menu, like so: http://cl.ly/image/26432Y441S3l. Is this different from what is being built?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Sorry, I did hg qref -X mobile/android/base to remove the WIP selection indicator, then immediately "hg qref" which undid that. Lots of extra stuff in that patch that shouldn't have been there.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_sharing_fennec.patch

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8391450 [details] [diff] [review]
tab_sharing_fennec.patch

Review of attachment 8391450 [details] [diff] [review]:
-----------------------------------------------------------------

Can't actually say much sensible about this.

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +22,4 @@
>  
>  NS_IMPL_ISUPPORTS1(MediaEngineTabVideoSource, MediaEngineVideoSource)
>  
> +MediaEngineTabVideoSource::MediaEngineTabVideoSource() 

nit: spurious whitespace

::: content/media/webrtc/MediaEngineTabVideoSource.h
@@ +19,5 @@
>      MediaEngineTabVideoSource();
> +    ~MediaEngineTabVideoSource() { if (mData)
> +	{
> +	  free(mData);
> +	} 

Indentation/Formatting, spurious whitespace.

::: mobile/android/components/TabSource.js
@@ +33,5 @@
> +
> +    let bundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +    let title = bundle.GetStringFromName("tabshare.title")
> +
> +    // First, let's decide on which feed to subscribe

Outdated comment? Copy paste error?

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +303,5 @@
>  getUserMedia.videoDevice.front = Front facing
>  getUserMedia.videoDevice.back = Back facing
>  getUserMedia.videoDevice.none = No Video
> +getUserMedia.videoDevice.tabShare = Choose a tab to stream
> +getUserMedia.videoDevice.prompt = Video source

Won't this change the dialog into:
"Video source"
"Front facing"

If you remove the "camera" from the prompt you'll need to add it back to the camera strings themselves.

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 8391450 [details] [diff] [review]
tab_sharing_fennec.patch

Review of attachment 8391450 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has no TabsTray changes as far as I can see. Clearing review flag.

::: mobile/android/components/TabSource.js
@@ +15,5 @@
> +  classDescription: "Fennec Tab Source",
> +  contractID: "@mozilla.org/tab-source-service;1",
> +  QueryInterface: function(aIID)
> +  {
> +    if (!aIID.equals(Ci.nsITabSource) &&    

nit: remove trailing spaces here.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #15)

> Won't this change the dialog into:
> "Video source"
> "Front facing"
> 
> If you remove the "camera" from the prompt you'll need to add it back to the
> camera strings themselves.

Ian, further string changes?

Comment 18

[Ian Barlow (:ibarlow)]

In the designs (http://cl.ly/image/26432Y441S3l), the 'video source' choices read as: 

Front facing camera
Rear facing camera
Choose a tab to stream

So yes, we should make sure the strings are updated to avoid the problem that gcp pointed out.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: tab_sharing_fennec.patch

Comment 20

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

Comment on attachment 8392513 [details] [diff] [review]
tab_sharing_fennec.patch

>diff --git a/content/media/webrtc/MediaEngineTabVideoSource.cpp b/content/media/webrtc/MediaEngineTabVideoSource.cpp

>+MediaEngineTabVideoSource::~MediaEngineTabVideoSource()
>+{
>+  if (mData)
>+    {
>+      free(mData);
>+    }
>+}

This file seems to also allow "no brace" style. Also checking to see if the indent on these braces is right. If it's right, then ignore me.

>diff --git a/mobile/android/chrome/content/WebrtcUI.js b/mobile/android/chrome/content/WebrtcUI.js

>+        if (device.name.startsWith("&") && device.name.endsWith(";"))
>+	  return Strings.browser.GetStringFromName(device.name.substring(1, device.name.length -1));

TAB in the indent
I don't know if this scheme is the best way to handle the string, but it works well enough for now.

>diff --git a/mobile/android/components/TabSource.js b/mobile/android/components/TabSource.js

>+  QueryInterface: function(aIID)
>+  {
>+    if (!aIID.equals(Ci.nsITabSource) &&
>+        !aIID.equals(Ci.nsISupports)) {
>+      throw Components.results.NS_ERROR_NO_INTERFACE;
>+    }
>+    return this;
>+  },

Try using our normal QueryInterface impl:

    QueryInterface: XPCOMUtils.generateQI([Ci.nsITabSource]),

It should work, and would be more consistent. If this doesn't work for some reason, just let me know and use the current impl.

>+    while (result == null)
>+      thread.processNextEvent(true);
>+
>+    if (result == -1)
>+      return null;

Would you mind throwing {} around these two statements

>+  },
>+  notifyStreamStart: function(window) {

Add a blank line between the methods

>+    for (var i in tabs) {

Use "let"

>+  },
>+  notifyStreamStop: function(window) {

Blank line

>+    for (var i in tabs) {

"let"

r+ with the nits

I think Gian-Carlo added some WebRTC tests for the UI. Make sure this doesn't break those tests. Maybe see if we could file a followup to add a test for the Tab Share mode.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: recording_indicator.patch

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/af15bd900a2a

Comment 23

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

Comment on attachment 8393176 [details] [diff] [review]
recording_indicator.patch

>diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java

>+        GeckoAppShell.getEventDispatcher().registerEventListener("Tab:Streaming", this);
>+        GeckoAppShell.getEventDispatcher().registerEventListener("Tab:NotStreaming", this);

Didn't we change these to: Tab:StreamStart and Tab:StreamStop ?

(Looking at patch that landed, you did not make that change. See comment 10)

Let's make that change now.

>diff --git a/mobile/android/base/widget/ThumbnailWrapper.java b/mobile/android/base/widget/ThumbnailWrapper.java

>+    private static String LOGTAG = "GeckoWrapper";


Too generic. GeckoThumbnailWrapper ?

>+    public int[] onCreateDrawableState(int extraSpace) {

>+        if (mRecording)
>+            mergeDrawableStates(drawableState, STATE_RECORDING);

braces please

Could you also remove this file too:
https://hg.mozilla.org/mozilla-central/diff/71734f89f0a1/mobile/android/base/resources/layout/.%23tabs_item_row.xml

It was part of a different patch that must have got mixed in by accident.

Overall, this looks fine to me. I'd still like Lucas to take a look for anything I missed.

Comment 24

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

I really do want the Tab:StreamStart and Tab:StreamStop changes :)

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 8393176 [details] [diff] [review]
recording_indicator.patch

Review of attachment 8393176 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/TabsTray.java
@@ +90,5 @@
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        try {
> +            int tabId = message.getInt("tabID");
> +            View view = TabsTray.this.getChildAt(mTabsAdapter.getPositionForTab(Tabs.getInstance().getTab(tabId)) - TabsTray.this.getFirstVisiblePosition());

Since views are recycled in these lists, this seems prone to failure (the fact we do it for titles in here is scary too...)

You should probably just set an attribute on the tab itself, then in the TabsAdapter (in this same file), set the wrapper data in getView. If you get Tab:StreamStart/Stop messages while this TabsTray is showing, just call adapter.notifyDataSetChanged();

Or maybe there's some reason you can't do that that I'm not seeing.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: menu_icon.patch

Comment 27

[Théo Chevalier]

http://hg.mozilla.org/mozilla-central/rev/af15bd900a2a#l10.37

>tabshare.title = "Choose a tab to stream"

Not sure where this string is being used, but are the quotes wanted?

Comment 28

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

(In reply to Théo Chevalier [:tchevalier] from comment #27)
> http://hg.mozilla.org/mozilla-central/rev/af15bd900a2a#l10.37
> 
> >tabshare.title = "Choose a tab to stream"
> 
> Not sure where this string is being used, but are the quotes wanted?

The quotes are not needed

Comment 29

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

Comment on attachment 8395222 [details] [diff] [review]
menu_icon.patch

The approach looks fine to me. Do we think exposing "thumbnail:" creates any privacy concern? I can't think of any, but wanted others to thin about it too.

Wes knows the Prompt code better than I and should give this a better look.

Brad - What does a screenshot of this prompt look like?

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #23)
> Comment on attachment 8393176 [details] [diff] [review]
> recording_indicator.patch
> 
> >diff --git a/mobile/android/base/TabsTray.java b/mobile/android/base/TabsTray.java
> 
> >+        GeckoAppShell.getEventDispatcher().registerEventListener("Tab:Streaming", this);
> >+        GeckoAppShell.getEventDispatcher().registerEventListener("Tab:NotStreaming", this);
> 
> Didn't we change these to: Tab:StreamStart and Tab:StreamStop ?
> 
> (Looking at patch that landed, you did not make that change. See comment 10)
> 
> Let's make that change now.
You asked for the function names to change in nsITabSource, and those did change.


> >+    public int[] onCreateDrawableState(int extraSpace) {
> 
> >+        if (mRecording)
> >+            mergeDrawableStates(drawableState, STATE_RECORDING);
> 
> braces please
> 
> Could you also remove this file too:
> https://hg.mozilla.org/mozilla-central/diff/71734f89f0a1/mobile/android/base/
> resources/layout/.%23tabs_item_row.xml
> 
> It was part of a different patch that must have got mixed in by accident.

I removed that the day after I pushed it.
note: https://hg.mozilla.org/mozilla-central/diff/tip/mobile/android/base/resources/layout/.%23tabs_item_row.xml

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: choose_tab_dialog.png

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 8393176 [details] [diff] [review]
recording_indicator.patch

Review of attachment 8393176 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/TabsTray.java
@@ +90,5 @@
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        try {
> +            int tabId = message.getInt("tabID");
> +            View view = TabsTray.this.getChildAt(mTabsAdapter.getPositionForTab(Tabs.getInstance().getTab(tabId)) - TabsTray.this.getFirstVisiblePosition());

What wesj said above :-) The streaming state should be in the Tab instance, which serves as a model for each TabsTray item. The views will be recycled and should hold this kind of state. If you want to know more about how TabsTray, ListView, etc work, have a look at:

http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/

::: mobile/android/base/resources/drawable/tab_thumbnail.xml
@@ +26,5 @@
> +
> +    <item gecko:state_recording="true">
> +
> +        <shape android:shape="rectangle">
> +            <solid android:color="#FFFF0000"/>

So the tab being recorded is just going to get a different border?

::: mobile/android/base/widget/ThumbnailWrapper.java
@@ +5,5 @@
> +import android.util.AttributeSet;
> +import org.mozilla.gecko.R;
> +
> +
> +public class ThumbnailWrapper extends LinearLayout {

This view name looks too generic for what it actually does. Maybe it should be called RecordingThumbnailWrapper or something. While you're at it, this should actually be a FrameLayout.

But I think the right thing to do here is to add a ThumbnailView subclass (called TabThumbnailView) and add the recording state to it instead of adding a custom wrapper.

@@ +29,5 @@
> +        final int[] drawableState = super.onCreateDrawableState(extraSpace + 1);
> +
> +        if (mRecording)
> +            mergeDrawableStates(drawableState, STATE_RECORDING);
> +     

nit: remove trailing spaces.

Comment 33

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #32)
 > But I think the right thing to do here is to add a ThumbnailView subclass
> (called TabThumbnailView) and add the recording state to it instead of
> adding a custom wrapper.

Even though now I see that the recording state applies to the container of the thumbnail, not the thumbnail itself. In this case, maybe just rename your wrapper to TabThumbnailWrapper for clarity.

Comment 34

[Martin Thomson [:mt:]]

I don't think that Jesup was clear enough about the security implications here.  This allows for trivial circumvention of web sandbox invariants.  Obtaining user consent is not sufficient in this case because the implications are not immediately obvious.

We should not be landing any code for tab or screen sharing without also providing a way to opt out of sharing for security-sensitive content.  Ideally, that means standards for notification and opt-out mechanisms (X-Frame-Options, for instance).

Chrome pulled their screen sharing capabilities for these reasons; they now require an "app install" to get the feature.  That is perhaps a little pessimistic, but it shows just how serious this issue is.

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Martin Thomson [:mt] from comment #34)
> I don't think that Jesup was clear enough about the security implications
> here.  This allows for trivial circumvention of web sandbox invariants. 
> Obtaining user consent is not sufficient in this case because the
> implications are not immediately obvious.
I disagree that the implications are not obvious (or at least obvious enough)
> 
> We should not be landing any code for tab or screen sharing without also
> providing a way to opt out of sharing for security-sensitive content. 
> Ideally, that means standards for notification and opt-out mechanisms
> (X-Frame-Options, for instance).
This sounds all fine and good, modulo the "we should not be landing any code.." bit. File bugs, let's debate the merits there.
> 
> Chrome pulled their screen sharing capabilities for these reasons; they now
> require an "app install" to get the feature.  That is perhaps a little
> pessimistic, but it shows just how serious this issue is.
I think limiting web features to installed web apps is a bad precedent. Let's figure out how to get the security and privacy right and not bandaid it by restricting to apps.

Comment 36

[Martin Thomson [:mt:]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #35)
> (In reply to Martin Thomson [:mt] from comment #34)
> > I don't think that Jesup was clear enough about the security implications
> > here.  This allows for trivial circumvention of web sandbox invariants. 
> > Obtaining user consent is not sufficient in this case because the
> > implications are not immediately obvious.
> I disagree that the implications are not obvious (or at least obvious enough)

I've seen no evidence that the security implications are understood universally by engineers building applications, let alone the general public.  In fact, I can provide several examples of clear demonstrations of clear misunderstanding.

There's a marked difference between understanding that the app can see what you can see, and understanding what it means when the app can also control what is shown at the same time.

> > We should not be landing any code for tab or screen sharing without also
> > providing a way to opt out of sharing for security-sensitive content. 
> > Ideally, that means standards for notification and opt-out mechanisms
> > (X-Frame-Options, for instance).
> This sounds all fine and good, modulo the "we should not be landing any
> code.." bit. File bugs, let's debate the merits there.

I will do that.  I don't care about landing as much as I care that there is something that can be used - and exploited - in released software.

> > Chrome pulled their screen sharing capabilities for these reasons; they now
> > require an "app install" to get the feature.  That is perhaps a little
> > pessimistic, but it shows just how serious this issue is.
> I think limiting web features to installed web apps is a bad precedent.
> Let's figure out how to get the security and privacy right and not bandaid
> it by restricting to apps.

Agree, just including the data point for emphasis.

Comment 37

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt] from comment #36)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #35)
> > (In reply to Martin Thomson [:mt] from comment #34)
> > > I don't think that Jesup was clear enough about the security implications
> > > here.  This allows for trivial circumvention of web sandbox invariants. 
> > > Obtaining user consent is not sufficient in this case because the
> > > implications are not immediately obvious.
> > I disagree that the implications are not obvious (or at least obvious enough)
> 
> I've seen no evidence that the security implications are understood
> universally by engineers building applications, let alone the general
> public.  In fact, I can provide several examples of clear demonstrations of
> clear misunderstanding.

Indeed, as far as I can tell they were not understood by the WebRTC
community until Adam Barth and I pointed it out in email to the mailin glist.


> There's a marked difference between understanding that the app can see what
> you can see, and understanding what it means when the app can also control
> what is shown at the same time.
> 
> > > We should not be landing any code for tab or screen sharing without also
> > > providing a way to opt out of sharing for security-sensitive content. 
> > > Ideally, that means standards for notification and opt-out mechanisms
> > > (X-Frame-Options, for instance).

This doesn't seem acceptable to me. It needs to be secure by default
not vice versa.


> > This sounds all fine and good, modulo the "we should not be landing any
> > code.." bit. File bugs, let's debate the merits there.
> 
> I will do that.  I don't care about landing as much as I care that there is
> something that can be used - and exploited - in released software.

Right, but until we resolve this issue, this should be preffed
off.


> > > Chrome pulled their screen sharing capabilities for these reasons; they now
> > > require an "app install" to get the feature.  That is perhaps a little
> > > pessimistic, but it shows just how serious this issue is.
> > I think limiting web features to installed web apps is a bad precedent.
> > Let's figure out how to get the security and privacy right and not bandaid
> > it by restricting to apps.
> 
> Agree, just including the data point for emphasis.

Comment 38

[Eric Rescorla (:ekr)]

Adding security team members.

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: recording_indicator.patch

Comment 40

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #31)
> Created attachment 8395792 [details]
> choose_tab_dialog.png

Looking nice. We might want to chat with Wesj about the size of the thumbnails (bigger might be better). I see the " in the dialog title! Looks like we really do need to remove those from the string.

Comment 41

[Théo Chevalier]

(In reply to Mark Finkle (:mfinkle) from comment #40)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #31)
> > Created attachment 8395792 [details]
> > choose_tab_dialog.png
> 
> Looking nice. We might want to chat with Wesj about the size of the
> thumbnails (bigger might be better). I see the " in the dialog title! Looks
> like we really do need to remove those from the string.

Yes, and maybe file bugs for locales that added them too. http://transvision.mozfr.org/string/?entity=mobile/android/chrome/browser.properties:tabshare.title&repo=central

 CC'ing ru and es localizers.

Comment 42

[Wesley Johnston (:wesj)]

Comment on attachment 8395222 [details] [diff] [review]
menu_icon.patch

Review of attachment 8395222 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I'd prefer to use favicons tbh. If we're going to put thumbnails in, I think we probably need some design. Maybe something like the color picker where they look stacked up? That probably requires a special View type in PromptListItem i.e. we'd set a Type in there to some enum value, then query it in PromptListAdapter, and inflate a different row type (with a full width image) if its set. The Color picker is a special PromptInput type. We could do that too...

If we leave it like this, you might try cropping out the top left corner a little bit as well (and fixing the aspect ratio to be square). We resize there here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/prompts/PromptListAdapter.java#121 You could try relaxing that if you want.

r+, but I think UX needs to sign off here as well.... maybe they did and I missed it.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +86,5 @@
> +                        }
> +                    }
> +                });
> +            ThumbnailHelper.getInstance().getAndProcessThumbnailFor(tab);
> +            return;

This function is big enough, it would probably be nice to pull this stuff into its own function

::: mobile/android/base/prompts/PromptListItem.java
@@ +56,5 @@
> +		    @Override
> +		    public void onBitmapFound(Drawable d) {
> +			mIcon = d;
> +		    }
> +		});

Careful with indentation here...

::: mobile/android/components/TabSource.js
@@ +37,5 @@
>        title: title,
>        window: null
>      }).setSingleChoiceItems(tabs.map(function(tab) {
> +      return { label: tab.browser.contentTitle || tab.browser.contentURI.spec,
> +               icon: "thumbnail:" + tab.id }

Ideally this might take a url. We could generate thumbnails on demand then, or reuse an existing tab, or a disk cache... Since its all internal, we can probably do that dance later if we want.

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 8396796 [details] [diff] [review]
recording_indicator.patch

Review of attachment 8396796 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/TabsTray.java
@@ +95,5 @@
> +            if (event.equals("Tab:StreamStart")) {
> +                tab.setRecording(true);
> +            } else if (event.equals("Tab:StreamStop")) {
> +                tab.setRecording(false);
> +            }

These messages should not depend on tabs tray being live. For instance, the tabs tray is stubbed and not created immediately on startup. So, I suggest:

1. Move the Tab:StreamStart and Tab:StreamStop handling to Tabs.java and call tab.setRecording(true/false) accordingly there.
2. Add a new TabsEvents.RECORDING_CHANGED (or simply TabEvents.RECORDING?) and call notifyListeners(tab, TabEvents.RECORDING_CHANGED) after updating the tab recording state.
3. Handle TabsEvents.RECORDING_CHANGED in TabsTray.

::: mobile/android/base/resources/layout/tabs_item_row.xml
@@ +20,3 @@
>                    android:layout_height="wrap_content"
>                    android:padding="4dip"
> +		  gecko:state_recording="true"

Leftover from local tests?

@@ +23,5 @@
>                    android:background="@drawable/tab_thumbnail"
>                    android:duplicateParentState="true">
>  
>          <org.mozilla.gecko.widget.ThumbnailView android:id="@+id/thumbnail"
> +						gecko:state_recording="true"

Ditto.

::: mobile/android/base/resources/values/attrs.xml
@@ +74,4 @@
>          <attr name="state_more" format="boolean"/>
>      </declare-styleable>
>  
> +    <declare-styleable name="ThumbnailView">

This should actually be TabThumbnailWrapper as it only applies to the view with the same name.

::: mobile/android/base/widget/TabThumbnailWrapper.java
@@ +5,5 @@
> +import android.util.AttributeSet;
> +import org.mozilla.gecko.R;
> +
> +
> +public class TabThumbnailWrapper extends LinearLayout {

Use FrameLayout instead.

::: mobile/android/components/TabSource.js
@@ +37,5 @@
> +      else if (tab.browser.contentURI && tab.browser.contentURI.spec)
> +        label = tab.browser.contentURI.spec;
> +      else
> +        label = tab.originalURI;
> +      return { label: label }

I'd prefer mfinkle/wesj to review the js bits.

Comment 44

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #42)
> Comment on attachment 8395222 [details] [diff] [review]
> menu_icon.patch
> 
> Review of attachment 8395222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I'd prefer to use favicons tbh. If we're going to put thumbnails in, I
> think we probably need some design. 

I'd prefer thumbnails if possible, as per the original design -- https://bug928096.bugzilla.mozilla.org/attachment.cgi?id=8364612

Comment 45

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #43)

> ::: mobile/android/base/widget/TabThumbnailWrapper.java
> @@ +5,5 @@
> > +import android.util.AttributeSet;
> > +import org.mozilla.gecko.R;
> > +
> > +
> > +public class TabThumbnailWrapper extends LinearLayout {
> 
> Use FrameLayout instead.
This is a LinearLayout now, why do you want to change it?
> 
> ::: mobile/android/components/TabSource.js
> @@ +37,5 @@
> > +      else if (tab.browser.contentURI && tab.browser.contentURI.spec)
> > +        label = tab.browser.contentURI.spec;
> > +      else
> > +        label = tab.originalURI;
> > +      return { label: label }
> 
> I'd prefer mfinkle/wesj to review the js bits.

mfinkle did

Comment 46

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #45)
> (In reply to Lucas Rocha (:lucasr) from comment #43)
> 
> > ::: mobile/android/base/widget/TabThumbnailWrapper.java
> > @@ +5,5 @@
> > > +import android.util.AttributeSet;
> > > +import org.mozilla.gecko.R;
> > > +
> > > +
> > > +public class TabThumbnailWrapper extends LinearLayout {
> > 
> > Use FrameLayout instead.
> This is a LinearLayout now, why do you want to change it?

This is container with only one child. No need to be a LinearLayout. Not a big deal.

Comment 47

[Daniel Veditz [:dveditz]]

> Flags: sec-review?

Sec review is covered by bug 942805

Comment 48

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

Comment on attachment 8396796 [details] [diff] [review]
recording_indicator.patch

r+ on the JS part

fyi: you editor is messing with your indents in tabs_item_row.xml

Comment 49

[RickieES aka Ricardo Palomares]

(In reply to Théo Chevalier [:tchevalier] from comment #41)
> (In reply to Mark Finkle (:mfinkle) from comment #40)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #31)
> > > Created attachment 8395792 [details]
> > > choose_tab_dialog.png
> > 
> > Looking nice. We might want to chat with Wesj about the size of the
> > thumbnails (bigger might be better). I see the " in the dialog title! Looks
> > like we really do need to remove those from the string.
> 
> Yes, and maybe file bugs for locales that added them too.
> http://transvision.mozfr.org/string/?entity=mobile/android/chrome/browser.
> properties:tabshare.title&repo=central
> 
>  CC'ing ru and es localizers.

I'll catch it anyway for es-ES because my toolchain warns me of changes in strings even if the entity/key name does not change itself.

Comment 50

[Wesley Johnston (:wesj)]

(In reply to Ian Barlow (:ibarlow) from comment #44)
> I'd prefer thumbnails if possible, as per the original design --
> https://bug928096.bugzilla.mozilla.org/attachment.cgi?id=8364612

I missed this thanks. This will require bigger changes underneith our Prompt service stuff. I played around yesterday with it, but I think we'll want to let you specify a view-type for a list (or maybe for individual rows in a list...). Lets do that in a separate bug.

Comment 51

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: recording_indicator.patch

Comment 52

[Lucas Rocha (:lucasr)]

Comment on attachment 8399821 [details] [diff] [review]
recording_indicator.patch

Review of attachment 8399821 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great with the suggested changes.

::: mobile/android/base/Tab.java
@@ +68,4 @@
>      private ErrorType mErrorType = ErrorType.NONE;
>      private static final int MAX_HISTORY_LIST_SIZE = 50;
>      private volatile int mLoadProgress;
> +    private int mRecordingCount = 0;

Make mRecordingCount volatile as you set/get it from different threads.

::: mobile/android/base/TabsTray.java
@@ +8,4 @@
>  import java.util.ArrayList;
>  import java.util.List;
>  
> +import org.json.JSONObject;

Unused?

@@ +12,5 @@
>  import org.mozilla.gecko.animation.PropertyAnimator;
>  import org.mozilla.gecko.animation.PropertyAnimator.Property;
>  import org.mozilla.gecko.animation.ViewHelper;
>  import org.mozilla.gecko.widget.TwoWayView;
> +import org.mozilla.gecko.widget.ThumbnailView;

Unused?

@@ +32,4 @@
>  import android.widget.ImageButton;
>  import android.widget.ImageView;
>  import android.widget.TextView;
> +import android.util.Log;

Unused?

@@ +36,3 @@
>  
>  public class TabsTray extends TwoWayView
> +    implements TabsPanel.PanelView {

Unrelated change?

@@ +132,5 @@
>              info = (ViewGroup) view;
>              title = (TextView) view.findViewById(R.id.title);
>              thumbnail = (ImageView) view.findViewById(R.id.thumbnail);
>              close = (ImageButton) view.findViewById(R.id.close);
> +            wrapper = (TabThumbnailWrapper) view.findViewById(R.id.wrapper);

wrapper -> thumbnailWrapper for clarity.

::: mobile/android/base/resources/layout/tabs_item_row.xml
@@ +4,4 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <org.mozilla.gecko.widget.TabRow xmlns:android="http://schemas.android.com/apk/res/android"
> +				 xmlns:gecko="http://schemas.android.com/apk/res-auto"

Unused?

@@ +15,4 @@
>                                   android:paddingBottom="6dip"
>                                   android:background="@drawable/tab_row">
>  
> +    <org.mozilla.gecko.widget.TabThumbnailWrapper android:layout_width="wrap_content"

nit: break line after TabThumbnailWrapper. Move layout_width after id.

::: mobile/android/base/resources/values/attrs.xml
@@ +74,4 @@
>          <attr name="state_more" format="boolean"/>
>      </declare-styleable>
>  
> +    <declare-styleable name="TabThumbnailView">

This should be TabThumbnailWrapper, no?

::: mobile/android/base/widget/TabThumbnailWrapper.java
@@ +6,5 @@
> +import org.mozilla.gecko.R;
> +
> +
> +public class TabThumbnailWrapper extends FrameLayout {
> +    private boolean mRecording = false;

nit: move mRecording after LOGTAG

@@ +7,5 @@
> +
> +
> +public class TabThumbnailWrapper extends FrameLayout {
> +    private boolean mRecording = false;
> +    private static String LOGTAG = "GeckoThumbnailWrapper";

Unused?

Comment 53

[Martin Thomson [:mt:]]

(In reply to Lucas Rocha (:lucasr) from comment #52)
> Comment on attachment 8399821 [details] [diff] [review]
> recording_indicator.patch
> 
> Review of attachment 8399821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great with the suggested changes.
> 
> ::: mobile/android/base/Tab.java
> @@ +68,4 @@
> >      private ErrorType mErrorType = ErrorType.NONE;
> >      private static final int MAX_HISTORY_LIST_SIZE = 50;
> >      private volatile int mLoadProgress;
> > +    private int mRecordingCount = 0;
> 
> Make mRecordingCount volatile as you set/get it from different threads.

That is not sufficient.  Because you use ++ and --, you will instead need to use Atomic<int> (I'm not up on what facilities are present in Android).  Using volatile only guarantees that reads and writes are independently safe, it does not prevent loss of counts for ++/--.  ++ and -- are not atomic.

I'm thinking that mLoadProgress is probably the same, but I didn't look.

Comment 54

[Lucas Rocha (:lucasr)]

(In reply to Martin Thomson [:mt] from comment #53)
> (In reply to Lucas Rocha (:lucasr) from comment #52)
> > Comment on attachment 8399821 [details] [diff] [review]
> > recording_indicator.patch
> > 
> > Review of attachment 8399821 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks great with the suggested changes.
> > 
> > ::: mobile/android/base/Tab.java
> > @@ +68,4 @@
> > >      private ErrorType mErrorType = ErrorType.NONE;
> > >      private static final int MAX_HISTORY_LIST_SIZE = 50;
> > >      private volatile int mLoadProgress;
> > > +    private int mRecordingCount = 0;
> > 
> > Make mRecordingCount volatile as you set/get it from different threads.
> 
> That is not sufficient.  Because you use ++ and --, you will instead need to
> use Atomic<int> (I'm not up on what facilities are present in Android). 
> Using volatile only guarantees that reads and writes are independently safe,
> it does not prevent loss of counts for ++/--.  ++ and -- are not atomic.
> 
> I'm thinking that mLoadProgress is probably the same, but I didn't look.

Ah, you're right. AtomicInteger should work fine here:

http://developer.android.com/reference/java/util/concurrent/atomic/AtomicInteger.html

Comment 55

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #54)
> (In reply to Martin Thomson [:mt] from comment #53)
> > (In reply to Lucas Rocha (:lucasr) from comment #52)
> > > Comment on attachment 8399821 [details] [diff] [review]
> > > recording_indicator.patch
> > > 
> > > Review of attachment 8399821 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Looks great with the suggested changes.
> > > 
> > > ::: mobile/android/base/Tab.java
> > > @@ +68,4 @@
> > > >      private ErrorType mErrorType = ErrorType.NONE;
> > > >      private static final int MAX_HISTORY_LIST_SIZE = 50;
> > > >      private volatile int mLoadProgress;
> > > > +    private int mRecordingCount = 0;
> > > 
> > > Make mRecordingCount volatile as you set/get it from different threads.
> > 
> > That is not sufficient.  Because you use ++ and --, you will instead need to
> > use Atomic<int> (I'm not up on what facilities are present in Android). 
> > Using volatile only guarantees that reads and writes are independently safe,
> > it does not prevent loss of counts for ++/--.  ++ and -- are not atomic.
> > 
> > I'm thinking that mLoadProgress is probably the same, but I didn't look.
> 
> Ah, you're right. AtomicInteger should work fine here:
> 
> http://developer.android.com/reference/java/util/concurrent/atomic/
> AtomicInteger.html

It is incremented and decremented on the same thread. That said, no harm in future-proofing.

Comment 56

[Martin Thomson [:mt:]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #55)
> It is incremented and decremented on the same thread. That said, no harm in
> future-proofing.

Thread confinement is a perfectly valid approach.  No need to provide redundant protection in that case.

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f963d175e165
https://hg.mozilla.org/mozilla-central/rev/299e19d9b209
https://hg.mozilla.org/mozilla-central/rev/e47f044bd09d

Comment 58

[Brad Lassey [:blassey] (use needinfo?)]

I'm goint to close out this bug. Please file follow up bugs for any follow up work.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

Erin, could you suggest a description here? "Streaming of a tab" or is it too vague?

Comment 60

[Wesley Johnston (:wesj)]

It sounds like we're planning to ship this enabled on 33. Can we track there?

Comment 61

[Benjamin Kerensa [:bkerensa]]

(In reply to Wesley Johnston (:wesj) from comment #60)
> It sounds like we're planning to ship this enabled on 33. Can we track there?

Tracking for now

Comment 62

[Brad Lassey [:blassey] (use needinfo?)]

Not planning on 33 for now, we need to satisify the concerns brought up in sec review.

Wes, I assume you're thinking of tab casting, which is different.

Comment 63

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.