Closed Bug 1036653 Opened 10 years ago Closed 10 years ago

Implement application sharing for getUserMedia

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla34

People

(Reporter: m_and_m, Assigned: m_and_m)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 22 obsolete files)

1.24 KB, patch
florian
: review+
Details | Diff | Splinter Review
10.56 KB, patch
m_and_m
: review+
Details | Diff | Splinter Review
36.93 KB, patch
m_and_m
: review+
Details | Diff | Splinter Review
23.89 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.59 KB, patch
m_and_m
: review+
Details | Diff | Splinter Review
16.41 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Implement the ability to create aMediaStream for a specific application's main window(s).  This maps to the "application" mediaSource in the getUserMedia() constraints (at this time).
https://bugzilla.mozilla.org/show_bug.cgi?id=983504#c99

Application Sharing on MacOSX is ready to review , I will put comments in this bug for application sharing.
Depends on: 983504
Implements application sharing for Mac OS X only
Matt -- Can I assign this to you?  We definitely want this in Fx 34.
Flags: needinfo?(linuxwolf)
Priority: -- → P1
Target Milestone: --- → mozilla34
Yes, you can assign this to me.

I will need some help for the parts specific to Linux or Windows, though.  Not sure I can tackle three platforms at the same time.
Flags: needinfo?(linuxwolf)
Attached patch Patch 3 - Mac-specific support (obsolete) — Splinter Review
Attachment #8466810 - Attachment description: changes to webrtcUI to support applications → Patch 1 - changes to webrtcUI to support applications
Attachment #8455046 - Attachment is obsolete: true
By re-using current WebRTC windows capture for application sharing is not workable for Application with DirectX render or customer DirectUI render,and also has NonClient Area issue since DWM(DesktopWindowManager)
I rewrite new capture  AppCapturerWin::CaptureBySample for application sharing, please refer to latest code my forked ,and verify and help re-create new patch for windows application sharing. 
https://github.com/vagouzhou/gecko-dev/commit/5d8ce8dbc7e9eb7c2b1681fd6d7e2ce4fca25bb2
I  finished one sample mode application capturer for linux platform
After this commit
https://github.com/vagouzhou/gecko-dev/commit/45e870bb709cde3e9afd9fb680e1374426366e3e.

It work well on my ubuntu 14.04 LTS now. 

Please help verify and generate code-review patches for linux platform.
also re-use current webrtc window capturer for application sharing on linux platform. 
but it don’t capture window decorations area(title-bar,border). code is ready and work well by this commit.
https://github.com/vagouzhou/gecko-dev/commit/baf1663ad2019889cd9bdf66d3b2a8a89e67788d
by default , it use sample mode capturer on linux platform.
also refactoring code to remove "\\win\\", "\\screen\\" and "\\app\\" from UniqueIdName.
Original, I want to use it to identify type. But now it is useless because there are CaptureDeviceType to identify type. 
https://github.com/vagouzhou/gecko-dev/commit/57b2f5313a2bab907c73dab561813cad3335a92b
Attachment #8466810 - Attachment is obsolete: true
Attachment #8471960 - Flags: review?(florian)
Attachment #8466812 - Attachment is obsolete: true
Attachment #8471962 - Flags: review?(rjesup)
Attachment #8471962 - Flags: review?(gpascutto)
Attachment #8466813 - Attachment is obsolete: true
Attachment #8471963 - Flags: review?(rjesup)
Attachment #8471963 - Flags: review?(gpascutto)
Attachment #8466814 - Attachment is obsolete: true
Attachment #8471966 - Flags: review?(rjesup)
Attachment #8471966 - Flags: review?(gpascutto)
Attached patch Patch 5 - X11-specific support (obsolete) — Splinter Review
Attachment #8471968 - Flags: review?(rjesup)
Attachment #8471968 - Flags: review?(gpascutto)
Comment on attachment 8471960 [details] [diff] [review]
Patch 1 v2 - changes to webrtcUI to support applications

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ -2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- LOCALIZATION NOTE : FILE This file contains the browser main menu items -->
> -<!-- LOCALIZATION NOTE : FILE Do not translate commandkeys --> 

Please avoid touching the whitespace on the whole file.

@@ +708,5 @@
>  <!ENTITY social.marklinkMenu.label "Save Link To…">
>  
>  <!ENTITY getUserMedia.selectCamera.label "Camera to share:">
>  <!ENTITY getUserMedia.selectCamera.accesskey "C">
> +<!ENTITY getUserMedia.selectWindowOrScreen.label "Window, application, or screen to share:">

Changing the meaning of a string requires changing its id to ensure all localizers notice the change.

::: browser/modules/webrtcUI.jsm
@@ +333,5 @@
>                menupopup.appendChild(chromeDoc.createElement("menuseparator"));
>                separatorNeeded = false;
>              }
> +            addDeviceToList(menupopup, devices[i].name, i,
> +                            deviceMediaSource == "application" ? "Application" : "Window");

This change will cause the button label to be updated to the string getUserMedia.shareApplication.label which needs to be added in browser.properties.

You also very likely need new strings in webrtcIndicator.properties.

@@ +439,5 @@
>      _command: function(aEvent) {
>        let type = this.getAttribute("type");
>        if (type == "Camera" || type == "Microphone")
>          type = "Devices";
> +      else if (type == "Window" || type == "Application")

Unless you also make changes near http://hg.mozilla.org/mozilla-central/annotate/ee1ad12a3939/browser/modules/webrtcUI.jsm#l577 the type will never be "Application".
Attachment #8471960 - Flags: review?(florian) → review-
Comment on attachment 8471962 [details] [diff] [review]
Patch 2 v2 - updates to basic framework for app sharing

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +211,5 @@
> +  InitializeScreenList();
> +  InitializeWindowList();
> +  InitializeApplicationList();
> +
> +  return 0;

Does this function ever return other than 0, or would it?  If not, then void makes more sense.

There are a bunch of such routines here.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h
@@ +4,5 @@
>  
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +* You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

remove duplicate
Attachment #8471962 - Flags: review?(rjesup) → review+
Comment on attachment 8471963 [details] [diff] [review]
Patch 3 v2 - Mac-specific support

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

I'm no mac expert, but r+ on what I see

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +16,5 @@
> +#include <AppKit/AppKit.h>
> +
> +#include "webrtc/modules/desktop_capture/window_capturer.h"
> +#include "webrtc/modules/desktop_capture/app_capturer.h"
> +// #include "webrtc/modules/desktop_capture/mac/CGSPrivate.h"

UNless there's a very good reason, remove

@@ +83,5 @@
> +  // Check that selected process exists
> +  NSRunningApplication *ra = [NSRunningApplication runningApplicationWithProcessIdentifier:process_id_];
> +  if (!ra) {
> +    callback_->OnCaptureCompleted(NULL);
> +    return ;

remove space before ;

@@ +121,5 @@
> +  CFRelease(windowInfos);
> +
> +  // Check that window list is not empty
> +  if (captureWindowListCount <= 0) {
> +    delete []captureWindowList;

space after [] (throughout)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

remove duplicate
Attachment #8471963 - Flags: review?(rjesup) → review+
Comment on attachment 8471968 [details] [diff] [review]
Patch 5 - X11-specific support

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

r- for error checks
Throughout: needs   webrtc::XErrorTrap error_trap(display); as appropriate to avoid errors leading to crashes/assertions, and checks for errors

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_x11.cc
@@ +42,5 @@
> +  long size;
> +  long numRects;
> +  BOX *rects;
> +  BOX extents;
> +} REGION;

Please include Xregion.h instead of cloning the definitions.

@@ +49,5 @@
> +public:
> +  WindowsCapturerProxy() : window_capturer_(WindowCapturer::Create()) {
> +    window_capturer_->Start(this);
> +  }
> +  ~WindowsCapturerProxy(){}

Style nit (throughout): space before {

@@ +115,5 @@
> +  // Sample Mode
> +  ScreenCapturerProxy screen_capturer_proxy_;
> +  Region rgn_mask_;
> +  Region rgn_visual_;
> +  Region rgn_background_;

Add 1-liner comments for each region

@@ +139,5 @@
> +    XDestroyRegion(rgn_mask_);
> +  if (rgn_visual_)
> +    XDestroyRegion(rgn_visual_);
> +  if (rgn_background_)
> +    XDestroyRegion(rgn_background_);

braces for all these ifs

@@ +191,5 @@
> +
> +  for (unsigned int i = 0; i < num_children; ++i) {
> +    ::Window app_window = window_util_x11.GetApplicationWindow(children[i]);
> +    if (!app_window)
> +      continue;

Braces around continue

@@ +194,5 @@
> +    if (!app_window)
> +      continue;
> +
> +    unsigned int processId = window_util_x11.GetWindowProcessID(app_window);
> +    if(processId != 0 && processId == selected_process_) {

space after if (throughout)

@@ +195,5 @@
> +      continue;
> +
> +    unsigned int processId = window_util_x11.GetWindowProcessID(app_window);
> +    if(processId != 0 && processId == selected_process_) {
> +      // capture

comment is superfluous; extend or remove

@@ +200,5 @@
> +      window_capturer_proxy_.SelectWindow(app_window);
> +      window_capturer_proxy_.Capture(region);
> +      DesktopFrame* frameWin = window_capturer_proxy_.GetFrame().get();
> +      if (frameWin == NULL)
> +        continue;

braces

@@ +205,5 @@
> +
> +      XRectangle  win_rect;
> +      window_util_x11.GetWindowRect(app_window,win_rect, false);
> +      if (win_rect.width <= 0 || win_rect.height <= 0)
> +        continue;

braces

@@ +216,5 @@
> +
> +      // bitblt into background frame
> +      frame->CopyPixelsFrom(*frameWin, DesktopVector(0, 0), target_rect);
> +
> +      continue;

redundant "continue"

@@ +221,5 @@
> +    }
> +  }
> +
> +  if (children)
> +    XFree(children);

braces

@@ +225,5 @@
> +    XFree(children);
> +
> +  // trigger event
> +  if (callback_)
> +    callback_->OnCaptureCompleted(frame.release());

braces

@@ +240,5 @@
> +    // fill background with black
> +    FillDesktopFrameRegionWithColor(frame, rgn_background_, 0xFF000000);
> +
> +    // fill foreground with yellow
> +    FillDesktopFrameRegionWithColor(frame, rgn_mask_, 0xFFFFFF00);

File a bug for making this configurable and maybe selectable by the application via constraints

@@ +245,5 @@
> + }
> +
> +  // trigger event
> +  if(callback_)
> +    callback_->OnCaptureCompleted(screen_capturer_proxy_.GetFrame().release());

braces (throughout)

@@ +254,5 @@
> +    return;
> +  if (XEmptyRegion(rgn))
> +    return;
> +
> +  REGION * st_rgn = (REGION *)rgn;

remove space after *

@@ +283,5 @@
> +}
> +void AppCapturerLinux::PrintWindow(::Window window) {
> +  WindowUtilX11 window_util_x11(x_display_);
> +  unsigned int processId = window_util_x11.GetWindowProcessID(window);
> +  std::string strAppName = "";

is this needed?

@@ +345,5 @@
> +      //update rgn_visual_ , rgn_mask_,
> +      unsigned int processId = window_util_x11.GetWindowProcessID(app_window);
> +      if (processId != 0 && processId == selected_process_) {
> +        XUnionRegion(rgn_visual_,win_rgn,rgn_visual_);
> +        XSubtractRegion(rgn_mask_,win_rgn,rgn_mask_);

spaces after , (throughout)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +76,5 @@
> +
> +      if (!app_window
> +          || window_util_x11.IsDesktopElement(app_window)
> +          || window_util_x11.GetWindowStatus(app_window) == WithdrawnState )
> +        continue;

braces (throughout)

@@ +96,5 @@
> +      pDesktopApplication->setProcessId(processId);
> +
> +      // process path name
> +      char szFilePathName[256]={0};
> +      pDesktopApplication->setProcessPathName(szFilePathName);

pDesktopApplication->setProcessPathName("");

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

remove duplicate

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_util.cc
@@ +9,5 @@
> + */
> +
> +#include "webrtc/modules/desktop_capture/x11/shared_x_util.h"
> +
> +namespace webrtc{

space before brace (throughout)

@@ +20,5 @@
> +  process_atom_ = XInternAtom(display(), "_NET_WM_PID", True);
> +  frame_extends_atom_ = XInternAtom(display(), "_NET_FRAME_EXTENTS", True);
> +}
> +WindowUtilX11::~WindowUtilX11(){
> +

get rid of blank line

@@ +28,5 @@
> +  XWindowProperty<uint32_t> window_state(display(), window, wm_state_atom_);
> +
> +  // WM_STATE is considered to be set to WithdrawnState when it missing.
> +  int32_t state = window_state.is_valid() ?
> +  *window_state.data() : WithdrawnState;

indent these, or combine with previous line

@@ +51,5 @@
> +  ::Window app_window = 0;
> +  for (unsigned int i = 0; i < num_children; ++i) {
> +    app_window = GetApplicationWindow(children[i]);
> +    if (app_window)
> +      break;

braces (throughout)

@@ +67,5 @@
> +  // First look for _NET_WM_WINDOW_TYPE. The standard
> +  // (http://standards.freedesktop.org/wm-spec/latest/ar01s05.html#id2760306)
> +  // says this hint *should* be present on all windows, and we use the existence
> +  // of _NET_WM_WINDOW_TYPE_NORMAL in the property to indicate a window is not
> +  // a desktop element (that is, only "normal" windows should be shareable).

Note: this means that no menus/dropdowns/etc will likely be visible.  File a followup bug to assess if we need to extend this

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_util.h
@@ +28,5 @@
> +
> +namespace webrtc {
> +
> +//===============================================================================
> +// most of code are copy from window_captuer_x11.cc to reuse common code

revise comment (clarity, spelling, moved not copied)

@@ +60,5 @@
> +  }
> +
> +  ~XWindowProperty() {
> +    if (data_)
> +      XFree(data_);

Normal style nit: braces
Attachment #8471968 - Flags: review?(rjesup) → review-
Comment on attachment 8471966 [details] [diff] [review]
Patch 4 v2 - Windows-specific support

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

Adding someone who knows Windows better for additional review

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
@@ +86,5 @@
> +  struct EnumWindowsLPARAM {
> +    ProcessId process_id;
> +    std::vector<WindowItem> vecWindows;
> +    bool bListAll;
> +  };

Are these Windows structures or ours?

@@ +118,5 @@
>      processId_(NULL) {
> +  // Sample mode
> +  hrgn_foreground_ = CreateRectRgn(0,0,0,0);
> +  hrgn_background_ = CreateRectRgn(0,0,0,0);
> +  hrgn_visual_ = CreateRectRgn(0,0,0,0);

1-liner descriptions

@@ +128,5 @@
> +    DeleteObject(hrgn_foreground_);
> +  if(hrgn_background_)
> +    DeleteObject(hrgn_background_);
> +  if(hrgn_visual_)
> +    DeleteObject(hrgn_visual_);

braces (throughout)

@@ +207,5 @@
> +  HBITMAP bmpOrigin = (HBITMAP)::SelectObject(memDcCapture, frameCapture->bitmap());
> +  BOOL bCaptureAppResult =  false;
> +  // Capture and Combine all windows into memDcCapture
> +  std::vector<WindowItem>::reverse_iterator itItem;
> +  for(itItem = lParamEnumWindows.vecWindows.rbegin(); itItem != lParamEnumWindows.vecWindows.rend(); itItem++) {

space after for (throughout

@@ +210,5 @@
> +  std::vector<WindowItem>::reverse_iterator itItem;
> +  for(itItem = lParamEnumWindows.vecWindows.rbegin(); itItem != lParamEnumWindows.vecWindows.rend(); itItem++) {
> +    WindowItem window_item = *itItem;
> +    HWND hWndCapturer = window_item.hWnd;
> +    if(!IsWindow(hWndCapturer) || !IsWindowVisible(hWndCapturer) || IsIconic(hWndCapturer))

space after if...

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +52,5 @@
> +  //List all running applications exclude background process.
> +  HWND hWnd;
> +  for (hWnd = GetWindow(GetDesktopWindow(), GW_CHILD); hWnd; hWnd = GetWindow(hWnd, GW_HWNDNEXT)) {
> +    if (!IsWindowVisible(hWnd))
> +      continue;

braces (throughout)

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

remove dup

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/win_shared.cc
@@ +10,5 @@
> +std::string Utf16ToUtf8(const WCHAR* str) {
> +    int len_utf8 = WideCharToMultiByte(CP_UTF8, 0, str, -1,
> +        NULL, 0, NULL, NULL);
> +    if (len_utf8 <= 0)
> +        return std::string();

braces
Attachment #8471966 - Flags: review?(rjesup)
Attachment #8471966 - Flags: review?(bas)
Attachment #8471966 - Flags: review+
(In reply to Randell Jesup [:jesup] from comment #22)
> Comment on attachment 8471966 [details] [diff] [review]
> Patch 4 v2 - Windows-specific support
> 
> Review of attachment 8471966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Adding someone who knows Windows better for additional review
> 
> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
> @@ +86,5 @@
> > +  struct EnumWindowsLPARAM {
> > +    ProcessId process_id;
> > +    std::vector<WindowItem> vecWindows;
> > +    bool bListAll;
> > +  };
> 
> Are these Windows structures or ours?
> 

These are not structures defined by Windows.  I can change the names to make that clearer.
(In reply to Randell Jesup [:jesup] from comment #20)
> Comment on attachment 8471963 [details] [diff] [review]
> Patch 3 v2 - Mac-specific support
> 
> Review of attachment 8471963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm no mac expert, but r+ on what I see
> 
> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
> @@ +16,5 @@
> > +#include <AppKit/AppKit.h>
> > +
> > +#include "webrtc/modules/desktop_capture/window_capturer.h"
> > +#include "webrtc/modules/desktop_capture/app_capturer.h"
> > +// #include "webrtc/modules/desktop_capture/mac/CGSPrivate.h"
> 
> UNless there's a very good reason, remove
> 

I completely spaced that.  At one point this code used private APIs which require this header to compile, but it isn't now.
Comment on attachment 8471962 [details] [diff] [review]
Patch 2 v2 - updates to basic framework for app sharing

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

Just some minor remarks.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +211,5 @@
> +  InitializeScreenList();
> +  InitializeWindowList();
> +  InitializeApplicationList();
> +
> +  return 0;

It's implementing the interface so I'm not sure he has a choice. That said for example InitializeWindowList can fail e.g. if (!pWinCap) but the error isn't relayed back by the current code.

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc
@@ +398,3 @@
>      pWindowCapturer->SelectWindow(winId);
>  
>      MouseCursorMonitor * pMouseCursorMonitor = MouseCursorMonitor::CreateForWindow(webrtc::DesktopCaptureOptions::CreateDefault(), winId);

You don't need to create another DesktopCaptureOptions here, you have "options" already, just pass that down.
Attachment #8471962 - Flags: review?(gpascutto) → review+
Comment on attachment 8471963 [details] [diff] [review]
Patch 3 v2 - Mac-specific support

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm
@@ +142,5 @@
> +  delete []captureWindowList;
> +
> +  // Wrap raw data into DesktopFrame
> +  if (!app_image) {
> +      CFRelease(app_image);

nit: inconsistent indentation

::: media/webrtc/trunk/webrtc/modules/desktop_capture/mac/desktop_device_info_mac.mm
@@ +62,5 @@
> +      continue;
> +    if (pid == getpid())
> +      continue;
> +
> +    DesktopApplication *pDesktopApplication = new DesktopApplication;

Check that this doesn't leak. (It looks like it leaks to me, but I don't know what .mm files are)
Attachment #8471963 - Flags: review?(gpascutto) → review+
Comment on attachment 8471966 [details] [diff] [review]
Patch 4 v2 - Windows-specific support

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
@@ +160,5 @@
> +}
> +
> +BOOL CALLBACK AppCapturerWin::EnumWindowsProc(HWND handle, LPARAM lParam) {
> +  EnumWindowsLPARAM *pEnumWindowsLPARAM = (EnumWindowsLPARAM *)lParam;
> +  if(pEnumWindowsLPARAM==NULL)

nit: spaces

@@ +185,5 @@
> +  //List Windows of selected application
> +  EnumWindowsLPARAM lParamEnumWindows;
> +  lParamEnumWindows.process_id = processId_;
> +  lParamEnumWindows.bListAll = false;
> +  EnumWindows(EnumWindowsProc,(LPARAM)&lParamEnumWindows);

nit: missing space

@@ +216,5 @@
> +
> +    HDC memDcWin = NULL;
> +    HBITMAP bmpOriginWin = NULL;
> +    HBITMAP hBitmapFrame = NULL;
> +    HDC dcWin =  NULL;

nit: Take this extra space and put it before the = on the next line ;-)

@@ +260,5 @@
> +    if (memDcWin) {
> +      SelectObject(memDcWin, bmpOriginWin);
> +      DeleteDC(memDcWin);
> +    }
> +    if (dcWin)

nit: braces for readability

@@ +320,5 @@
> +    SetDCBrushColor(memDcCapture, RGB(0xff,0xff,0));
> +    FillRect(memDcCapture, &rcScreen, (HBRUSH)GetStockObject(DC_BRUSH));
> +
> +    if (dcScreen)
> +      ReleaseDC(NULL, dcScreen);

nit: braces for readability again
Attachment #8471966 - Flags: review?(gpascutto) → review+
>Check that this doesn't leak. (It looks like it leaks to me, but I don't know what .mm files are)

Disregard, it's fine.
Blocks: 1054503
Comment on attachment 8471968 [details] [diff] [review]
Patch 5 - X11-specific support

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.cc
@@ +99,5 @@
> +      char szFilePathName[256]={0};
> +      pDesktopApplication->setProcessPathName(szFilePathName);
> +
> +      // application name
> +      std::string strAppName="";

Don't need the =""; part

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/desktop_device_info_x11.h
@@ +23,3 @@
>    //DesktopDeviceInfo Interfaces
> +  virtual int32_t InitializeApplicationList();
> +  virtual int32_t InitializeScreenList();

Add OVERRIDE for consistency with the rest of the code.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_util.cc
@@ +86,5 @@
> +  }
> +
> +  if (strcmp("gnome-panel", class_hint.res_name) == 0 ||
> +      strcmp("desktop_window", class_hint.res_name) == 0) {
> +    result = true;

This is a bit strange. Why is the GNOME Panel considered a "desktop element"?

@@ +194,5 @@
> +  *window_state.data() : -1;
> +  return state;
> +}
> +
> +bool WindowUtilX11::GetWindowFrameExtends(::Window window,

Extends or extents?

@@ +205,5 @@
> +  int actual_format;
> +  unsigned long nitems;
> +  unsigned long bytes_remaining;
> +  unsigned char *data;
> +  long *data_as_long;

Move to point of use.

@@ +275,5 @@
> +    win_info.height = nScreenCY - absy;
> +  }
> +
> +  // data setting
> +  rcWindow.x =absx;

spaces
Attachment #8471968 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #29)
> Comment on attachment 8471968 [details] [diff] [review]
> Patch 5 - X11-specific support
> 
> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/x11/shared_x_util.cc
> @@ +86,5 @@
> > +  }
> > +
> > +  if (strcmp("gnome-panel", class_hint.res_name) == 0 ||
> > +      strcmp("desktop_window", class_hint.res_name) == 0) {
> > +    result = true;
> 
> This is a bit strange. Why is the GNOME Panel considered a "desktop element"?

[ NOTE: this is code originally in window_capturer_x11.cc, moved here to be shared with app_capturer_x11.cc ]

IIRC, gnome-panel was similar to Windows' Taskbar, which would be considered a part of the desktop not the app itself.

> 
> @@ +194,5 @@
> > +  *window_state.data() : -1;
> > +  return state;
> > +}
> > +
> > +bool WindowUtilX11::GetWindowFrameExtends(::Window window,
> 
> Extends or extents?

That should be extents; will change.
This is the absolute minimum necessary change to allow the user to choose an application to share.  It treats application exactly the same as window, and is likely not adequate enough.

However, I'm not sure a more explicit distinction is really worth the amount of change needed, nor am I sure I fully understand where and how and what all the changes really are.
Attachment #8471960 - Attachment is obsolete: true
Attachment #8475371 - Flags: review?(florian)
Interdiff over attachment 8471962 [details] [diff] [review] to address review comments from Jesup and Gian-Carlo

Also includes absolute minimum changes to determine capture state.
Interdiff over attachment 8471963 [details] [diff] [review] to address review comments from Jesup and Gian-Carlo.
Interdiff over attachment 8471966 [details] [diff] [review] to address review comments from Jesup and Gian-Carlo.
Interdiff over attachment 8471968 [details] [diff] [review] to address review comments from Jesup and Gian-Carlo.
Attachment #8475419 - Flags: review?(rjesup)
Attachment #8475409 - Flags: review?(rjesup)
Attachment #8475410 - Flags: review?(rjesup)
Attachment #8475416 - Flags: review?(rjesup)
Comment on attachment 8475371 [details] [diff] [review]
Patch 1 v3 - changes to webrtcUI to support applications

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

Looks OK as a temporary solution to unblock platform work. We can do the rest of the UI work in a different bug.

(In reply to Matthew Miller [:linuxwolf] from comment #31)

> However, I'm not sure a more explicit distinction is really worth the amount
> of change needed, nor am I sure I fully understand where and how and what
> all the changes really are.

The UI changes for app sharing are being designed in bug 1053221, you are welcome to give feedback there if you have thoughts about the UI.
Attachment #8475371 - Flags: review?(florian) → review+
Assignee: nobody → linuxwolf
Attachment #8475409 - Flags: review?(rjesup) → review+
Attachment #8475410 - Flags: review?(rjesup) → review+
Attachment #8475416 - Flags: review?(rjesup) → review+
Comment on attachment 8475419 [details] [diff] [review]
Interdiff for review comments on X11-specific support

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_x11.cc
@@ +225,1 @@
>      callback_->OnCaptureCompleted(frame.release());

Do we want to callback if the ErrorTrap stuff caught an error?

@@ +249,1 @@
>      callback_->OnCaptureCompleted(screen_capturer_proxy_.GetFrame().release());

ditto
Attachment #8475419 - Flags: review?(rjesup) → review+
Attachment #8471966 - Flags: review?(bas) → review?(jmathies)
(In reply to Randell Jesup [:jesup] from comment #37)
> Comment on attachment 8475419 [details] [diff] [review]
> Interdiff for review comments on X11-specific support
> 
> Review of attachment 8475419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_x11.cc
> @@ +225,1 @@
> >      callback_->OnCaptureCompleted(frame.release());
> 
> Do we want to callback if the ErrorTrap stuff caught an error?
> 
> @@ +249,1 @@
> >      callback_->OnCaptureCompleted(screen_capturer_proxy_.GetFrame().release());
> 
> ditto

We probably do want to callback, but with NULL instead of the frame.
Interdiff over attachment 8475419 [details] [diff] [review] to address review comments from Jesup.

This sends back a NULL frame if the XErrorTrap gets a status other than 0.
Attachment #8475419 - Attachment is obsolete: true
Attachment #8476246 - Flags: review?(rjesup)
Attachment #8476246 - Flags: review?(rjesup) → review+
Collapsing original + interdiffs into a single patch.

Carrying forward r+=jesup,gcp
Attachment #8471962 - Attachment is obsolete: true
Attachment #8475409 - Attachment is obsolete: true
Collapsing original + interdiff into a single patch

Carrying forward r+=jesup,gcp
Attachment #8471963 - Attachment is obsolete: true
Attachment #8475410 - Attachment is obsolete: true
Collapsing original + interdiffs into a single patch.

Carrying forward r+=jesup,gcp
Attachment #8471968 - Attachment is obsolete: true
Attachment #8476246 - Attachment is obsolete: true
Comment on attachment 8476655 [details] [diff] [review]
coelescing win patches for review

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

General comments -

1) This code doesn't conform to our coding standards at all. Since it's down in webrtc, maybe that's ok? I'm not sure.
2) uses of NULL in here could probably be converted to nullptr.

Jesup, I assume this code runs in the chrome process? (if not sandboxing is going to break it.)

Generally looks good, just a lot of nits. r- for now to see the final patch.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc
@@ +25,5 @@
>  namespace webrtc {
>  
>  namespace {
>  
> +class WindowsCapturerProxy : DesktopCapturer::Callback {

What do these proxy classes do? Please add comment headers explaining use.

@@ +165,5 @@
> +  CaptureBySample(region);
> +}
> +
> +BOOL CALLBACK AppCapturerWin::EnumWindowsProc(HWND handle, LPARAM lParam) {
> +  EnumWindowsCtx *pEnumWindowsCtx = (EnumWindowsCtx *)lParam;

nit - static_cast

@@ +166,5 @@
> +}
> +
> +BOOL CALLBACK AppCapturerWin::EnumWindowsProc(HWND handle, LPARAM lParam) {
> +  EnumWindowsCtx *pEnumWindowsCtx = (EnumWindowsCtx *)lParam;
> +  if (pEnumWindowsCtx == NULL) {

nit - if (!pEnumWindowsCtx) {

there are various other cases of this down below, please try to update them.

@@ +171,5 @@
> +    return FALSE;
> +  }
> +
> +  DWORD procId = -1;
> +  GetWindowThreadProcessId( handle, &procId );

nit - (handle, &procId);

@@ +176,5 @@
> +  if (procId == pEnumWindowsCtx->process_id || pEnumWindowsCtx->list_all) {
> +    WindowItem window_item;
> +    window_item.handle = handle;
> +
> +    if (!::IsWindowVisible(handle) || ::IsIconic(handle)) {

Is there some reason why you're explicitly specifying the global scope here? You don't do this in other calls.

@@ +200,5 @@
> +  DesktopRect rcDesktop(DesktopRect::MakeXYWH(
> +      GetSystemMetrics(SM_XVIRTUALSCREEN),
> +      GetSystemMetrics(SM_YVIRTUALSCREEN),
> +      GetSystemMetrics(SM_CXVIRTUALSCREEN),
> +      GetSystemMetrics(SM_CYVIRTUALSCREEN)

This is the virtual screen, which includes all monitors, was this what you wanted?

@@ +206,5 @@
> +
> +  HDC dcScreen = GetDC(NULL);
> +  HDC memDcCapture = CreateCompatibleDC(dcScreen);
> +  if (dcScreen) {
> +    ReleaseDC(NULL,dcScreen);

nit - (NULL, dcScreen)

@@ +212,5 @@
> +
> +  scoped_ptr<DesktopFrameWin> frameCapture(DesktopFrameWin::Create(
> +      DesktopSize(rcDesktop.width(), rcDesktop.height()),
> +      NULL, memDcCapture));
> +  HBITMAP bmpOrigin = (HBITMAP)::SelectObject(memDcCapture, frameCapture->bitmap());

nit - another random use of '::', plus static_cast.

@@ +257,5 @@
> +      }
> +      if (hBitmapFrame == NULL) {
> +        break;
> +      }
> +      bmpOriginWin = (HBITMAP)SelectObject(memDcWin, hBitmapFrame);

nit - static_cast

@@ +263,5 @@
> +
> +    // bitblt to capture memDcCapture
> +    if (bmpOriginWin) {
> +      BitBlt(memDcCapture,
> +          rcWin.left, rcWin.top, rcWin.right - rcWin.left,rcWin.bottom - rcWin.top,

nit - missing a space between rcWin.left & rcWin.bottom

@@ +347,5 @@
> +    // trigger event
> +    if (callback_) {
> +      callback_->OnCaptureCompleted(screen_capturer_proxy_.GetFrame().release());
> +    }
> +    return;

Looks like this can just fall through, and you can remove the first if (callback_) block.

@@ +378,5 @@
> +  for (itItem = lParamEnumWindows.windows.rbegin(); itItem != lParamEnumWindows.windows.rend(); itItem++) {
> +    WindowItem window_item = *itItem;
> +    SetRectRgn(hrgn_window, 0, 0, 0, 0);
> +    if (GetWindowRgn(window_item.handle, hrgn_window) == ERROR) {
> +      SetRectRgn(hrgn_window,window_item.bounds.left,

nit - space

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/desktop_device_info_win.cc
@@ +62,5 @@
> +    if (dwProcessId == GetCurrentProcessId())
> +      continue;
> +    // filter out any already processed
> +    if(desktop_application_list_.find(dwProcessId) != desktop_application_list_.end())
> +      continue;

nit - combine these into a single if block that checks all three conditions.

@@ +67,2 @@
>  
> +    //Add one application

nit - space after //, there are more of these below.

@@ +74,5 @@
> +    pDesktopApplication->setProcessId(dwProcessId);
> +
> +    // process path name
> +    WCHAR szFilePathName[MAX_PATH]={0};
> +    QueryFullProcessImageNameProc lpfnQueryFullProcessImageNameProc = (QueryFullProcessImageNameProc)GetProcAddress(GetModuleHandle(TEXT("kernel32.dll")), "QueryFullProcessImageNameW");

I think you can get rid of your typedefs and just use decltype for these.

@@ +104,5 @@
> +    pDesktopApplication->setProcessPathName(Utf16ToUtf8(szFilePathName).c_str());
> +
> +    // application name
> +    WCHAR szWndTitle[_MAX_PATH]={0};
> +    GetWindowText(hWnd,szWndTitle,MAX_PATH);

nit - spacing

@@ +108,5 @@
> +    GetWindowText(hWnd,szWndTitle,MAX_PATH);
> +    if (lstrlen(szWndTitle) <= 0)
> +      pDesktopApplication->setProcessAppName(Utf16ToUtf8(szFilePathName).c_str());
> +    else
> +      pDesktopApplication->setProcessAppName(Utf16ToUtf8(szWndTitle).c_str());

nit - please add appropriate paren here.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/win/win_shared.cc
@@ +8,5 @@
> +namespace webrtc {
> +
> +std::string Utf16ToUtf8(const WCHAR* str) {
> +    int len_utf8 = WideCharToMultiByte(CP_UTF8, 0, str, -1,
> +        NULL, 0, NULL, NULL);

nit -  align spacing with '('

@@ +10,5 @@
> +std::string Utf16ToUtf8(const WCHAR* str) {
> +    int len_utf8 = WideCharToMultiByte(CP_UTF8, 0, str, -1,
> +        NULL, 0, NULL, NULL);
> +    if (len_utf8 <= 0)
> +        return std::string();

nit - paren

@@ +15,5 @@
> +    std::string result(len_utf8, '\0');
> +    int rv = WideCharToMultiByte(CP_UTF8, 0, str, -1,
> +        &*(result.begin()), len_utf8, NULL, NULL);
> +    if (rv != len_utf8)
> +        assert(false);

nit - paren
Attachment #8476655 - Flags: review-
As for (1) and (2), this is upstream code that we collaborate on so indeed it has it's own coding standards (which seem to include using NULL and not nullptr).
gcp is correct: coding style follows google/webrtc.org style for media/webrtc/trunk.  Where there are unspecified bits, we extend theirs (like enforce use of {}, though they mostly do to on any new code).  NULL, not nullptr.  (Note that webrtc.org is independently buildable separate from Chrome/blink/chromium).

> Jesup, I assume this code runs in the chrome process? (if not sandboxing is going to break it.)

None of WebRTC other than UI runs in Chrome currently; this may well break in e10s.  The webrtc networking stuff does work with e10s mostly (and works with B2G); note that b2G getUserMedia camera/etc capture bypasses the "normal" code in the imported webrtc.org stuff we use to capture with (talks to b2g camera directly).

That'll be a whole other metabug...

>> +  EnumWindowsCtx *pEnumWindowsCtx = (EnumWindowsCtx *)lParam;
>> +  if (pEnumWindowsCtx == NULL) {
>
>nit - if (!pEnumWindowsCtx) {
>
>there are various other cases of this down below, please try to update them.

with some google types (scoped_ptr?) you can't use "if (foo)" constructs.  Not sure if this is one of them.  With those, you have to compare to NULL or use "if (!foo.get())" IIRC
(In reply to Randell Jesup [:jesup] from comment #46)
> gcp is correct: coding style follows google/webrtc.org style for
> media/webrtc/trunk.  Where there are unspecified bits, we extend theirs
> (like enforce use of {}, though they mostly do to on any new code).  NULL,
> not nullptr.  (Note that webrtc.org is independently buildable separate from
> Chrome/blink/chromium).
> 
> > Jesup, I assume this code runs in the chrome process? (if not sandboxing is going to break it.)
> 
> None of WebRTC other than UI runs in Chrome currently; this may well break
> in e10s.  The webrtc networking stuff does work with e10s mostly (and works
> with B2G); note that b2G getUserMedia camera/etc capture bypasses the
> "normal" code in the imported webrtc.org stuff we use to capture with (talks
> to b2g camera directly).
> 
> That'll be a whole other metabug...
> 
> >> +  EnumWindowsCtx *pEnumWindowsCtx = (EnumWindowsCtx *)lParam;
> >> +  if (pEnumWindowsCtx == NULL) {
> >
> >nit - if (!pEnumWindowsCtx) {
> >
> >there are various other cases of this down below, please try to update them.
> 
> with some google types (scoped_ptr?) you can't use "if (foo)" constructs. 
> Not sure if this is one of them.  With those, you have to compare to NULL or
> use "if (!foo.get())" IIRC

Ok, on coding style / feature use I'll default to you.

If this is running in content, the window/desktop sharing is going to break when we turn sandboxing on which will lower the integrity of the content process, which restricts its access to windowing related resources. We expect to turn this on in nightly this fall. Just an FYI!
Note, if you want, you can test all this today with sandboxing turned on, just add

ac_add_options --enable-content-sandbox

to your build, and flip 'browser.tabs.remote.autostart'.

https://wiki.mozilla.org/Security/Sandbox
There's no point in testing, there's almost nothing WebRTC related that works with sandboxing. At the very least permissions are broken in e10s already, audio output doesn't work, etc etc etc. It's also IMHO totally offtopic for this bug.
(In reply to Jim Mathies [:jimm] from comment #44)
> @@ +176,5 @@
> > +  if (procId == pEnumWindowsCtx->process_id || pEnumWindowsCtx->list_all) {
> > +    WindowItem window_item;
> > +    window_item.handle = handle;
> > +
> > +    if (!::IsWindowVisible(handle) || ::IsIconic(handle)) {
> 
> Is there some reason why you're explicitly specifying the global scope here?
> You don't do this in other calls.
> 

There isn't; will remove.

> @@ +200,5 @@
> > +  DesktopRect rcDesktop(DesktopRect::MakeXYWH(
> > +      GetSystemMetrics(SM_XVIRTUALSCREEN),
> > +      GetSystemMetrics(SM_YVIRTUALSCREEN),
> > +      GetSystemMetrics(SM_CXVIRTUALSCREEN),
> > +      GetSystemMetrics(SM_CYVIRTUALSCREEN)
> 
> This is the virtual screen, which includes all monitors, was this what you
> wanted?
> 

It is for now.  Better multi-monitor support is reserved for Bug 1037797; will add a comment referencing that bug.
Interdiff over attachment 8476655 [details] [diff] [review] to address review comments from Jim Mathies.
Attachment #8475416 - Attachment is obsolete: true
Interdiff over attachment 8476655 [details] [diff] [review] to address review comments from Jim Mathies.

The full patch is attachment 8476799 [details] [diff] [review].
Comment on attachment 8476799 [details] [diff] [review]
Patch 4 v3 - Windows-specific support

Complete patch of window-specific changes.

Interdiff over previous patches is in attachment 8476802 [details] [diff] [review].
Attachment #8476799 - Attachment description: Interdiff for review comments on Windows-specific support → Patch 4 v3 - Windows-specific support
Attachment #8476799 - Flags: review?(jmathies)
Attachment #8471966 - Attachment is obsolete: true
Attachment #8471966 - Flags: review?(jmathies)
Attachment #8476655 - Attachment is obsolete: true
Comment on attachment 8476799 [details] [diff] [review]
Patch 4 v3 - Windows-specific support

Thanks!
Attachment #8476799 - Flags: review?(jmathies) → review+
Blocks: 1057006
Comment on attachment 8476802 [details] [diff] [review]
Interdiff for review comments on Windows-specific support

changes already in attachment 8476799 [details] [diff] [review]
Attachment #8476802 - Attachment is obsolete: true
Attachment #8477026 - Flags: review?(rjesup) → review+
Comment on attachment 8476297 [details] [diff] [review]
Patch 2 v3 - updates to basic framework for app sharing

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

This patch bitrotted with the landing of bug 1050802.

::: dom/media/MediaManager.h
@@ +130,5 @@
>    }
> +  bool CapturingApplication()
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +    return mVideoSource && !mStopped &&

This likely needs a " !mVideoSource->IsAvailable() &&" like we added in bug 1050802 attachment 8476843 [details] [diff] [review].
Fix the bitrot I mentioned in comment 57.
Attachment #8476297 - Attachment is obsolete: true
I've been fighting build issues on Linux (32-bit) Debug.  Once I included a missing header to fix undefined symbols, I now get even stranger errors.  This is exacerbated by the fact that 64-bit Linux seems to build fine.  I also don't have a 32-bit linux handy, but am working on that. 

Latest failure log: < https://tbpl.mozilla.org/php/getParsedLog.php?id=46596441&tree=Try&full=1 >
Patch queue (stop at "try-linux"): < http://hg.mozilla.org/users/linuxwolf_outer-planes.net/appshare-patches/ >

Could someone take a look and see what my real problem is?
Flags: needinfo?(rjesup)
Flags: needinfo?(gpascutto)
Looks like an include-order issue with X11 loving to make macros for common strings like "Data()"

Try with include order changed:
https://tbpl.mozilla.org/?tree=Try&rev=7a076cceb1f0
Flags: needinfo?(rjesup)
New try:  (shared_x11_utils.cc was missing a bunch)
 https://tbpl.mozilla.org/?tree=Try&rev=6eca29c8b05d

These failures are partly due to it working when it shouldn't, due to unified builds (which can semi-randomly include other things for you)

Locally, you can disable all the unified building with:
ac_add_options --disable-unified-compilation
in .mozconfig.
Try is green now (using the try-linux params)
Comment on attachment 8477369 [details] [diff] [review]
Patch 2 v4 - updates to basic framework for app sharing

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

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc
@@ +251,2 @@
>  #else
> +      snprintf(idStr, sizeof(idStr), "%ld", pWinDevice->getScreenId());

There's a small touch of bitrot here caused by bug 1057174.
Flags: needinfo?(gpascutto)
Update to address bitrot from Bug 1057174 that Florian uncovered.

Carry forward r+=jesup,gcp
Attachment #8477369 - Attachment is obsolete: true
Update to fix all missing includes.

Carry forward r+=jesup
Attachment #8477026 - Attachment is obsolete: true
Interdiff over attachment 8478503 [details] [diff] [review] to address bitrot fixes.
Full patch for basic framework changes, correctly addressing bitrot.  Interdiff in attachment 8479105 [details] [diff] [review].
Attachment #8478503 - Attachment is obsolete: true
Attachment #8479108 - Flags: review?(rjesup)
Blocks: 1058766
Attachment #8479108 - Flags: review?(rjesup) → review+
Comment on attachment 8476298 [details] [diff] [review]
Patch 3 v3 - Mac-specific support for app sharing

Carry forward r+=jesup,gcp
Attachment #8476298 - Flags: review+
Comment on attachment 8476299 [details] [diff] [review]
Patch 5 v2 - X11-specific support for app sharing

Carry forward r+=rjesup,gcp
Attachment #8476299 - Flags: review+
Comment on attachment 8478509 [details] [diff] [review]
Patch 6 v2 - fix missing includes in shared_x_util

Carry forward r+=rjesup
Attachment #8478509 - Flags: review+
Comment on attachment 8479105 [details] [diff] [review]
Interdiff over basic framework to address bitrot

Changes already present in the full patch (attachment 8479108 [details] [diff] [review])
Attachment #8479105 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 1058944
sorry had to backout this change since this might have caused a non unified build bustage on windows like https://tbpl.mozilla.org/php/getParsedLog.php?id=46833334&tree=Mozilla-Inbound
Depends on: 1059295
Flags: qe-verify+
Depends on: 1102253
Depends on: 1102865
Depends on: 1102871
Depends on: 1102886
We performed some exploratory testing on this implementation using Firefox 34 beta 11 across platforms, found a few issues and logged bugs for those to track them separately. Will go ahead and close this bug as verified fixed.
More information regarding our testing can be found in the etherpad [1] we tracked our work.

*[1] https://etherpad.mozilla.org/getUserMedia-Applications
Status: RESOLVED → VERIFIED
Blocks: 1050930
Depends on: 1466742
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: