1208417 - [Presentation WebAPI] Integrate multi-screen over HDMI into Presentation API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tommy Kuo (away forever...)]

We want to integrate multi-screen feature into presentation API as the 1-UA scenario.

Comment 1

[Tommy Kuo (away forever...)]

I think only the display which have the top-level window can be a presentation device. So, I'll add the display to the presentation device list after the top-level window opened.

The control flow is following:

display connected --> emit "display changed" event --> check mozSettings --> open top-level window --> add to presentation device list

Comment 2

[Tommy Kuo (away forever...)]

I noticed that there should be only one HDMI display can be handled in nsScreenManagerGonk. The ID is as same as the display type. So, all HDMI display would have the same ID.

According to this limit, we don't need to maintain a HDMI display list. I decide to keep a HDMIDisplayDevice object in HDMIDipslayProvider. We can create it when the provider is initialized and keep it until the provider is killed. I think it would be more efficient than creating a new HDMIDisplayDevice object on display connected every time.

Comment 3

[Tommy Kuo (away forever...)]

Now, we decide to open the top level window when we need it. So, the display is in mirroring mode on the display connected by default. But I noticed that we can not open a top-level window when display is in mirroring mode. I saw some errors in log:

> E/libEGL  ( 1608): EGLNativeWindowType 0xa651dc08 already connected to another API
> E/libEGL  ( 1608): eglCreateWindowSurface:414 error 3003 (EGL_BAD_ALLOC)

I think we still need some mechanism to toggle screen-mirroring dynamically. Maybe we can reopen Bug 1210708 to do that.

Comment 4

[Sotaro Ikeda [:sotaro]]

I do not think Bug 1210708 is necessary to address this problem. CompositorOGL::CreateContext() already try to get GLContext if widget already has GLContext. 
   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp#101

But gonk's nsWindow::GetNativeData() does not handle it.
  https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#534

Comment 5

[Sotaro Ikeda [:sotaro]]

:KuoE0, can you check if attachment 8686999 [details] [diff] [review] work?

Comment 6

[Tommy Kuo (away forever...)]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> :KuoE0, can you check if attachment 8686999 [details] [diff] [review] work?

It works!! Thanks a lot. So which bug is the attachment belong to? Will it land to m-c?

Comment 7

[Sotaro Ikeda [:sotaro]]

The patch is in Bug 1224445. I set it as to block this bug. I am going to ask review.

Comment 8

[Tommy Kuo (away forever...)]

Attachment: [WIP] Bug-1208417-hg.patch

Hi Kyle. I create a top-level window (nsIDOMWindow) with WindowWatcher::OpenWindow() in HDMIDisplayProvider::HDMIDisplayDevice::OpenTopLevelWindow(). And I need to close the window (nsIDOMWindow) when turn off multi-screen mode. So I call nsIDOMWindow::Close() to do that. But I found this method no longer exists after Bug 1216401 fixed.

What is the proper way to close it?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

QI to nsPIDOMWindow and call Close on that.

Comment 10

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Add disconnect interface to nsIPresentationDevice

I add disconnect interface into nsIPresentationDevice.idl. In multi-screen, device need to close the top level window when Presentation API is disconnected.

Comment 11

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

We'll use Presentation API as the only one way to start multi-screen mode. So, we don't need MultiscreenHandler.jsm ever. All functionality of multi-screen is moving into HDMIDisplayProvider::HDMIDisplayDevice.

Comment 12

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

For now, we leverage the existed architecture with TCP socket to establish the presentation session transport. We establish the socket by connecting to localhost in 1-UA use case. But we hope to create the LocalPresentationSessionTransport without launch a TCP server in the future (see Bug 1195221).

Comment 13

[Tommy Kuo (away forever...)]

Hi smaug, can you review the part 1 & part 2?

For part 3, I think it is just a workaround for now. We don't need to launch a TCP server for communication between local sessions.

Demo video for multi-screen: https://youtu.be/nYSu_dA942I

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(Sorry about delay. Review is coming, but I've had just quite a bit other stuff to review too lately.)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8702214 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

>+NS_IMETHODIMP
>+HDMIDisplayProvider::HDMIDisplayDevice::GetId(nsACString& aId)
>+{
>+  // add "hdmi:" as prefix
>+  aId = Move(nsCString("hdmi:"));
Move doesn't really help anything here.
aID.AppendLiteral("hdmi:"); 
should work just fine and is easier to read.


>+HDMIDisplayProvider::HDMIDisplayDevice::OpenTopLevelWindow()
>+{
>+  if (isTopLevelWindowOpened) {
>+    return NS_OK;
>+  }
>+
>+  nsresult rv;
>+  char* prefValue = nullptr;
>+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+
>+  rv = pref->GetCharPref("toolkit.defaultChromeFeatures", &prefValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCString flags(prefValue);
>+  flags.Append(",mozDisplayId=");
I think AppendLiteral is tiny bit more efficient.

>+  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
>+  rv = ww->OpenWindow(nullptr, remoteShellURL.Data(), windowName.Data(),
I'd like to see what kind of shell we actually end up loading here. Something with chrome privileges and then it has some remote
<xul:browser> or <html:iframe mozbrowser> ?

>+HDMIDisplayProvider::Init()
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  obs->AddObserver(this, DISPLAY_CHANGED_EVENT, false);
>+  // initial HDMIDisplayDevice
Ok, so HDMIDisplayProvider is a service and doesn't need to be removed from observer service, right?
Except that Init() can be called multiple times (based on that we have explicit Uninit), which means we end up calling AddObserver multiple times.
Though, I don't understand the need for mInitialized. It isn't used.
Please explain or fix.


>+    nsCOMPtr<nsIDOMWindow> mWindow;
>+    HDMIDisplayProvider* mProvider;
So I don't see mProvider to be ever set to null. What guarantees it is never access after the
provider object has been deleted? Please explicitly set it to null somewhere.


Mostly small things , but could take a look at a new patch.
And would like to see that shell thingie this is about to use.

Really sorry about review delays!

Comment 16

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

Comment 17

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

Comment 18

[Tommy Kuo (away forever...)]

Hi smuag, could you review the part 2 and part 3?

Part 2:
I updated with using mInitialized duplicate initialization. And I think the shell is <html:iframe mozbrowser>.

Part 3:
We decide to leverage the current TCP architecture to set up the communication. So please review it directly.

Comment 19

[Tommy Kuo (away forever...)]

Sorry, there is some typo in last comment.

Part 2:
I updated with using mInitialized to prevent duplicate initialization. And I think the shell is <html:iframe mozbrowser>.

Part 3:
We decide to leverage the current TCP architecture to set up the communication. So please review it directly.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8727257 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

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

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +20,5 @@
> +GetHDMIProviderLog()
> +{
> +  static PRLogModuleInfo* log = PR_NewLogModule("HDMIDisplayProvider");
> +  return log;
> +}

use LazyLogModule, see example in https://dxr.mozilla.org/mozilla-central/source/docshell/shistory/nsSHistory.cpp#62

@@ +100,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString remoteShellURL(prefValue);
> +  remoteShellURL.Append('#');
> +  remoteShellURL.Append(mId);

use nsSimpleURI instead of string concatenation?

::: dom/presentation/provider/PresentationDeviceProviderModule.cpp
@@ +41,5 @@
>  
>  static const mozilla::Module::CategoryEntry kPresentationDeviceProviderCategories[] = {
>  #if defined(MOZ_WIDGET_COCOA) || defined(MOZ_WIDGET_ANDROID) || (defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 16)
>    { PRESENTATION_DEVICE_PROVIDER_CATEGORY, "MulticastDNSDeviceProvider", MULTICAST_DNS_PROVIDER_CONTRACT_ID },
> +  { PRESENTATION_DEVICE_PROVIDER_CATEGORY, "HDMIDisplayProvider", HDMI_DISPLAY_PROVIDER_CONTRACT_ID },

HDMI provider is only available on Gonk, right?

Comment 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8727258 [details] [diff] [review]
[Part 3] Leverage socket-based session transport to establish presentation connection

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

steal r? from @smaug because I know better about device provider.

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +50,5 @@
> +  nsresult SetListener(HDMIDisplayProvider* aListener)
> +  {
> +    mListener = aListener;
> +    return NS_OK;
> +  }

You'll need to call "SetListener(nullptr)" when provider is destructing, otherwise mListener will be a dangling pointer.

@@ +209,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsresult rv;
> +  Unused << mPresentationServer->SetId(Move(nsCString("local.tcp.server")));

simply use NS_LITERAL_CSTRING("local.tcp.server").

@@ +219,5 @@
> +
> +  /**
> +    * If |servicePort| is non-zero, it means PresentationServer is running.
> +    * Otherwise, we should make it start serving.
> +    */

'/*' is enough.

@@ +331,5 @@
> +                                      const nsAString& aUrl,
> +                                      const nsAString& aPresentationId,
> +                                      nsIPresentationControlChannel* aControlChannel)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

MOZ_ASSERT check for aDeviceInfo and aControlChannel.

@@ +334,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsAutoCString address;
> +  Unused << aDeviceInfo->GetAddress(address);

unused local variable

@@ +381,5 @@
>                                      const nsAString& aPresentationId,
>                                      nsIPresentationControlChannel** aControlChannel)
>  {
> +  MOZ_ASSERT(aDevice);
> +  MOZ_ASSERT(mPresentationServer);

add |*aControlChannel = nullptr| to ensure the sanity of out param.

@@ +387,5 @@
> +  RefPtr<TCPDeviceInfo> deviceInfo = new TCPDeviceInfo(aDevice->Id(),
> +                                                       aDevice->Address(),
> +                                                       mPort);
> +
> +  return mPresentationServer->RequestSession(deviceInfo, aUrl, aPresentationId, aControlChannel);

nit: break into multiple line.

::: dom/presentation/provider/HDMIDisplayProvider.h
@@ +33,5 @@
>  };
>  
> +class HDMIDisplayWrappedListener;
> +
> +

nit: remove one extra new line

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8727257 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

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

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +87,5 @@
> +  nsresult rv;
> +  char* prefValue = nullptr;
> +  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +
> +  rv = pref->GetCharPref("toolkit.defaultChromeFeatures", &prefValue);

use nsCString for reading char pref, see example in https://dxr.mozilla.org/mozilla-central/source/browser/components/dirprovider/DirectoryProvider.cpp#137

@@ +91,5 @@
> +  rv = pref->GetCharPref("toolkit.defaultChromeFeatures", &prefValue);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Due to the limitation of nsWinodw, screen ID should be an integer.
> +  nsCString flags(prefValue);

use nsAutoCString

@@ +92,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Due to the limitation of nsWinodw, screen ID should be an integer.
> +  nsCString flags(prefValue);
> +  flags.Append(",mozDisplayId=");

use AppendLiteral

@@ +107,5 @@
> +  windowName.Append("TopWindow");
> +
> +  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
> +  rv = ww->OpenWindow(nullptr, remoteShellURL.Data(), windowName.Data(),
> +                      flags.Data(), nullptr, getter_AddRefs(mWindow));

Use PromiseFlatCString to ensure you get valid memory of char*.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Raw_Character_Pointers

@@ +123,5 @@
> +  }
> +
> +  auto* piWindow = nsPIDOMWindowOuter::From(mWindow);
> +  piWindow->Close();
> +  piWindow = nullptr;

mWindow = nullptr?

BTW, why we need "isTopLevelWindowOpened" if we can check the "mWindow" directly?

@@ +140,5 @@
> +
> +nsresult
> +HDMIDisplayProvider::Init()
> +{
> +  nsresult rv;

unused local variable

@@ +154,5 @@
> +
> +nsresult
> +HDMIDisplayProvider::Uninit()
> +{
> +  mInitialized = false;

unused member variable.

@@ +169,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIPresentationDevice> device(do_QueryObject(mDevice));

do_QueryInterface should be enough.

@@ +188,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIPresentationDevice> device(do_QueryObject(mDevice));

ditto.

@@ +226,5 @@
> +  } else {
> +    if (NS_WARN_IF(NS_FAILED(rv = Uninit()))) {
> +      return rv;
> +    }
> +  }

nsresult rv = mDeviceListener ? Init() : Uninit();
if (NS_WARN_IF(NS_FAILED(rv)) {
  return rv;
}

Does this look simpler?

@@ +247,5 @@
> +  if (!strcmp(aTopic, DISPLAY_CHANGED_EVENT)) {
> +    nsCOMPtr<nsIDisplayInfo> displayInfo = do_QueryInterface(aSubject);
> +    if (!displayInfo) {
> +      return NS_ERROR_NO_INTERFACE;
> +    }

simply do MOZ_ASSERT here?

@@ +252,5 @@
> +
> +    int32_t type;
> +    bool isConnected;
> +    displayInfo->GetConnected(&isConnected);
> +    displayInfo->GetId(&type);

This code is based one the implicit conversion between display Id and display type introduced in [1]. We should either fix it or add the comment for it.

[1] https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsScreenManagerGonk.cpp#928

@@ +254,5 @@
> +    bool isConnected;
> +    displayInfo->GetConnected(&isConnected);
> +    displayInfo->GetId(&type);
> +
> +    if (type == DisplayType::DISPLAY_EXTERNAL) {

if (DisplayType::DISPLAY_EXTERNAL != type) {
  return NS_OK;
}

It can reduce the level of indentation.

::: dom/presentation/provider/HDMIDisplayProvider.h
@@ +57,5 @@
> +
> +    nsresult OpenTopLevelWindow();
> +    nsresult CloseTopLevelWindow();
> +
> +    const nsCString Id() const { return mId; }

return nsCString& here?

@@ +68,5 @@
> +    nsCString mType;
> +    nsCString mId;
> +
> +    nsCOMPtr<mozIDOMWindowProxy> mWindow;
> +    HDMIDisplayProvider* mProvider;

add comment "// weak reference" for people to know we explicitly use the raw pointer.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Patch part 2 and 3 are for device provider, which I can help to review.

Comment 24

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #22)
> @@ +68,5 @@
> > +    nsCString mType;
> > +    nsCString mId;
> > +
> > +    nsCOMPtr<mozIDOMWindowProxy> mWindow;
> > +    HDMIDisplayProvider* mProvider;
> 
> add comment "// weak reference" for people to know we explicitly use the raw
> pointer.

You could use MOZ_NON_OWNING_REF.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

And comment why using raw ref is safe there, like what guarantees it never points to a deleted object.
(sorry I'm late with this bug, will look at this properly a bit later, I guess when the patches are updated.)

Comment 26

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

Hi S.C., I already updated my patch. Could review it again?

> @@ +107,5 @@
> > +  windowName.Append("TopWindow");
> > +
> > +  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
> > +  rv = ww->OpenWindow(nullptr, remoteShellURL.Data(), windowName.Data(),
> > +                      flags.Data(), nullptr, getter_AddRefs(mWindow));
> 
> Use PromiseFlatCString to ensure you get valid memory of char*.

I use nsAutoCString here. I think PromiseFlatCString is only for nsACString.

Comment 27

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

I also updated this part. Could you review it again? Thanks.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8733216 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

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

Getting closed!

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +18,5 @@
> +#include "nsThreadUtils.h"
> +
> +static mozilla::LazyLogModule gSHistoryLog("HDMIDisplayProvider");
> +
> +#define LOG(format) MOZ_LOG(gSHistoryLog, mozilla::LogLevel::Debug, format)

rename gSHitoryLog to something else.

@@ +85,5 @@
> +  nsresult rv;
> +  nsAutoCString prefValue;
> +  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +
> +  rv = pref->GetCharPref("toolkit.defaultChromeFeatures",

Maybe make these pref name strings as a #define?

@@ +124,5 @@
> +
> +  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);
> +  rv = ww->OpenWindow(nullptr,
> +                      remoteShellURLString.get(),
> +                      windowName.get(),

We seem to have a convention using "_blank" as the window name, you can follow it.
https://dxr.mozilla.org/mozilla-central/search?q=%3EOpenWindow(&redirect=true&case=false

@@ +182,5 @@
> +{
> +  // Provider must be deleted only once.
> +  if (!mInitialized) {
> +    return NS_OK;
> +  }

remove observer while deinitialization.

@@ +288,5 @@
> +    // See Bug 1138287 and nsScreenManagerGonk::AddScreen() for more detail.
> +    displayInfo->GetId(&type);
> +
> +    if (type == DisplayType::DISPLAY_EXTERNAL) {
> +      isConnected ? AddScreen() : RemoveScreen();

missing nsresult rv = ...? Or, you shouldn't use conditional operator to replace if-else statement.

::: dom/presentation/provider/HDMIDisplayProvider.h
@@ +32,5 @@
> +    DISPLAY_VIRTUAL,
> +    NUM_DISPLAY_TYPES
> +};
> +
> +class HDMIDisplayProvider final

This device provider is for all types of display (e.g. HDMI / Wifi Display), right? We might call it DisplayDeviceProvider.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8733219 [details] [diff] [review]
[Part 3] Leverage socket-based session transport to establish presentation connection

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

r+ with my comment addressed.

::: dom/presentation/provider/HDMIDisplayProvider.cpp
@@ +243,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsresult rv;
> +  rv =  mPresentationServer->SetId(NS_LITERAL_CSTRING("presentation.1ua.hdmi"));

simply use class name as server Id.

::: dom/presentation/provider/HDMIDisplayProvider.h
@@ +66,5 @@
>      const nsCString& Id() const { return mId; }
>  
>    private:
>      ~HDMIDisplayDevice() {
>        mProvider = nullptr;

RefPtr will naturally release the reference during destruction. You can simply write |virtual ~HDMIDisplayDevice() = default;

Comment 30

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Add disconnect interface to nsIPresentationDevice

Add reviewer's name. Carry r+.

Comment 31

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

> > +#include "nsThreadUtils.h"
> > +
> > +static mozilla::LazyLogModule gSHistoryLog("HDMIDisplayProvider");
> > +
> > +#define LOG(format) MOZ_LOG(gSHistoryLog, mozilla::LogLevel::Debug, format)
>
> rename gSHitoryLog to something else.

Sorry for the stupid mistake...

I updated the patch with your suggestions. Please review it again, thanks!

Comment 32

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

Hi smaug, I think this part is already completed to review. Could you review the patch? Thanks!

Comment 33

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8733710 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

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

::: dom/presentation/provider/DisplayDeviceProvider.cpp
@@ +85,5 @@
> +  MOZ_ASSERT(!mWindow);
> +
> +  nsresult rv;
> +  nsAutoCString prefValue;
> +  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);

MOZ_ASSERT(pref); ?

@@ +118,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID);

MOZ_ASSERT(ww); ?

@@ +143,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  piWindow = nullptr;

nsCOMPtr will be delete automatically, do we need to manually dereference it here?

@@ +166,5 @@
> +  if (mInitialized) {
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();

MOZ_ASSERT(obs);

@@ +181,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  obs->RemoveObserver(this, DISPLAY_CHANGED_EVENT);

if (obs) {
  obs->RemoveObserver(...);
}

Uninit might happened at xpcom-shutdown phase.

@@ +229,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  device->Disconnect();

add |mDevice = nullptr;|?

::: dom/presentation/provider/DisplayDeviceProvider.h
@@ +98,5 @@
> +  // weak pointer
> +  // HDMIDisplayDevice (mDevice) hold strong pointer to DisplayDeviceProvider.
> +  // Use nsWeakPtr to avoid reference cycle.
> +  // Now support HDMI display only and there should be only one HDMI display.
> +  nsWeakPtr mDevice = nullptr;

Provider should hold strong reference to device because it controls the lifecycle of device.
Make device hold weak reference to provider.

Comment 34

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

Comment 35

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

Comment 36

[Tommy Kuo (away forever...)]

Changes:
- Let the provider hold a strong pointer to the device.
- Let the device hold a weak pointer to the provider.
- Remove the device from PresentationDeviceManager when the provider is destroying.

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8734310 [details] [diff] [review]
[Part 2] Add HDMIDisplayProvider to Presentation API

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

r+ with following comment addressed.

::: dom/presentation/provider/DisplayDeviceProvider.cpp
@@ +12,5 @@
> +#include "nsIObserverService.h"
> +#include "nsIServiceManager.h"
> +#include "nsIWindowWatcher.h"
> +#include "nsPIDOMWindow.h"
> +#include "nsQueryObject.h"

this header file is not necessary.

Comment 38

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add HDMIDisplayProvider to Presentation API

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #37)
> Comment on attachment 8734310 [details] [diff] [review]
> [Part 2] Add HDMIDisplayProvider to Presentation API
> 
> Review of attachment 8734310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with following comment addressed.
> 
> ::: dom/presentation/provider/DisplayDeviceProvider.cpp
> @@ +12,5 @@
> > +#include "nsIObserverService.h"
> > +#include "nsIServiceManager.h"
> > +#include "nsIWindowWatcher.h"
> > +#include "nsPIDOMWindow.h"
> > +#include "nsQueryObject.h"
> 
> this header file is not necessary.

I removed some unnecessary header files. Bug without the following header files, I can't build successfully. I still keep them.

> +#include "nsIObserverService.h" 
> +#include "nsIServiceManager.h"
> +#include "nsPIDOMWindow.h"

Comment 39

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

Some changes on header files according the part 2.

Comment 40

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add DisplayDeviceProvider to Presentation API

Comment 41

[Tommy Kuo (away forever...)]

Attachment: [Part 3] Leverage socket-based session transport to establish presentation connection

Comment 42

[Tommy Kuo (away forever...)]

Attachment: [Part 1] Update interface of nsIPresentationDevice, r=smaug

Comment 43

[Tommy Kuo (away forever...)]

Attachment: [Part 2] Add DisplayDeviceProvider to Presentation API

Comment 44

[Tommy Kuo (away forever...)]

Attachment: Bug-1208417-Part-3-hg.patch

Comment 45

[Tommy Kuo (away forever...)]

Hi S.C., I add an attribute "windowId" for 1-UA device. I think all 1-UA devices need an window to render something on it. And we can use "windowId" to specify which window to communicate, like Bug 1235123.

Comment 46

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8735390 [details] [diff] [review]
[Part 1] Update interface of nsIPresentationDevice, r=smaug

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

I'll suggest you create an nsIPresentation1UADevice interface inherited from nsIPresentationDevice, and put the new member in it. Then, you can use do_QueryInterface to know when should perform 1UA specific control.

Comment 47

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Tommy Kuo [:KuoE0] from comment #38)
> Created attachment 8734683 [details] [diff] [review]
> [Part 2] Add HDMIDisplayProvider to Presentation API
> 
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #37)
> > Comment on attachment 8734310 [details] [diff] [review]
> > [Part 2] Add HDMIDisplayProvider to Presentation API
> > 
> > Review of attachment 8734310 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r+ with following comment addressed.
> > 
> > ::: dom/presentation/provider/DisplayDeviceProvider.cpp
> > @@ +12,5 @@
> > > +#include "nsIObserverService.h"
> > > +#include "nsIServiceManager.h"
> > > +#include "nsIWindowWatcher.h"
> > > +#include "nsPIDOMWindow.h"
> > > +#include "nsQueryObject.h"
> > 
> > this header file is not necessary.
> 
> I removed some unnecessary header files. Bug without the following header
> files, I can't build successfully. I still keep them.
> 
> > +#include "nsIObserverService.h" 
> > +#include "nsIServiceManager.h"
> > +#include "nsPIDOMWindow.h"

nsQueryObject.h is the only header file you need to remove.

Comment 48

[Tommy Kuo (away forever...)]

Attachment: [Part 1] (v2) Update interface of nsIPresentationDevice, r=smaug

Add a new interface nsIPresentationLocalDevice inherited nsIPresentationDevice.

Comment 49

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v2) Add DisplayDeviceProvider to Presentation API

Changes based on Part 1 (Use nsIPresentationLocalDevice).

Comment 50

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v2) Leverage socket-based session transport to establish presentation connection

Changes based on Part 1 and Part 2.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8736247 [details] [diff] [review]
[Part 1] (v2) Update interface of nsIPresentationDevice, r=smaug

Very sorry about delays here. Whenever I'm being slow, please ping me or send email or something.

>+++ b/dom/presentation/PresentationSessionInfo.cpp
>@@ -721,16 +721,17 @@ void
> PresentationPresentingInfo::Shutdown(nsresult aReason)
> {
>   PresentationSessionInfo::Shutdown(aReason);
> 
>   if (mTimer) {
>     mTimer->Cancel();
>   }
> 
>+  mDevice->Disconnect();
Is it guaranteed that mDevice is non-null here?
I think I'd prefer a null check here, since the setup is complicated and it isn't clear to me we guarantee Shuwdown isn't
ever called when SetDevice hasn't been called.

Should mDevice set to null after Disconnect() ?
If not, why not? At least add some comment.


>+interface nsIPresentationLocalDevice : nsIPresentationDevice
So nothing implements this yet, but I assume latter patches add the implementation,

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8736248 [details] [diff] [review]
[Part 2] (v2) Add DisplayDeviceProvider to Presentation API

Again, hard to understand/remember all the object hierarchy in presentation API implementation.
Would be really great to have some picture/diagram explaining it all.

>+static mozilla::LazyLogModule gDisplayDeviceProviderLog("DisplayDeviceProvider");
>+
>+#define LOG(format) MOZ_LOG(gDisplayDeviceProviderLog, mozilla::LogLevel::Debug, format)
>+
>+#define DISPLAY_CHANGED_EVENT   "display-changed"
Since it isn't any event but notification, perhaps call this DISPLAY_CHANGED_NOTIFICATION

>+#define DEFAULT_CHROME_FEATURES "toolkit.defaultChromeFeatures"
s/DEFAULT_CHROME_FEATURES/DEFAULT_CHROME_FEATURES_PREF/

>+#define CHROME_REMOTE_URL       "b2g.multiscreen.chrome_remote_url"
s/CHROME_REMOTE_URL/CHROME_REMOTE_URL_PREF/

...since those are about the pref name, not about features or url.


>+DisplayDeviceProvider::HDMIDisplayDevice
>+                     ::EstablishControlChannel(const nsAString& aUrl,
>+                                               const nsAString& aPresentationId,
>+                                               nsIPresentationControlChannel** aControlChannel)
>+{
>+  nsresult rv = OpenTopLevelWindow();
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
>+
>+  if (NS_WARN_IF(!mProvider)) {
>+    return NS_ERROR_FAILURE;
>+  }
Why do we check !mProvider after OpenTopLevelWindow?

>+  return mProvider->RequestSession(this, aUrl, aPresentationId, aControlChannel);
I'd prefer if you had strong reference to the provider before calling any methods on it to guarantee it stays alive during the whole
method call.


>+DisplayDeviceProvider::HDMIDisplayDevice::OpenTopLevelWindow()
>+{
>+  MOZ_ASSERT(!mWindow);
>+
>+  nsresult rv;
>+  nsAutoCString prefValue;
>+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  MOZ_ASSERT(pref);
>+
>+  rv = pref->GetCharPref(DEFAULT_CHROME_FEATURES, getter_Copies(prefValue));
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
Couldn't you use 
#include "mozilla/Preferences.h" and
Preferences::GetString here?


>+
>+  nsAutoCString flags(prefValue);
>+  flags.AppendLiteral(",mozDisplayId=");
>+  flags.AppendInt(mScreenId);
>+
>+  rv = pref->GetCharPref(CHROME_REMOTE_URL, getter_Copies(prefValue));
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
and for this too.

>+
>+  nsCOMPtr<nsIURI> remoteShellURL = new nsSimpleURI();
>+  rv = remoteShellURL->SetSpec(prefValue);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
>+
>+  rv = remoteShellURL->SetRef(mWindowId);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }

Please use NS_NewURI here.




>+DisplayDeviceProvider::~DisplayDeviceProvider()
>+{
>+  Uninit();
>+}
So DisplayDeviceProvider is added as a strong observer to observer service, so observer service keeps it alive.
This setup does work if it is clear that having a listener on the object means keeping the object alive until shutdown.
Please document that.
And test that we don't leak in debug builds.
export XPCOM_MEM_LEAK_LOG=1
environment variable can be useful for that.

>+DisplayDeviceProvider::Observe(nsISupports* aSubject,
>+                             const char* aTopic,
>+                             const char16_t* aData)
nit, align parameters

>+DisplayDeviceProvider::RequestSession(HDMIDisplayDevice* aDevice,
>+                                    const nsAString& aUrl,
>+                                    const nsAString& aPresentationId,
>+                                    nsIPresentationControlChannel** aControlChannel)
ditto



>+class DisplayDeviceProvider final : public nsIObserver
>+                                  , public nsIPresentationDeviceProvider
>+                                  , public SupportsWeakPtr<DisplayDeviceProvider>

It is still not clear to me who actually creates DisplayDeviceProvider. Perhaps that is in the next patch.


>+  // Now support HDMI display only and there should be only one HDMI display.
>+  nsCOMPtr<nsIPresentationLocalDevice> mDevice = nullptr;
No need to initialize nsCOMPtr to null

>+  // weak pointer
>+  // PresentationDeviceManager (mDeviceListener) hold strong pointer to
>+  // DisplayDeviceProvider. Use nsWeakPtr to avoid reference cycle.
>+  nsWeakPtr mDeviceListener = nullptr;
similar here.

>+#define DISPLAY_DEVICE_PROVIDER_CONTRACT_ID "@mozilla.org/presentation-device/displaydevice-provider;1"
I wonder why we talk about DisplayDeviceProvider everywhere, and why not HDMIDeviceProvider? Wouldn't the latter be more clear?
Up to you.



Fix all those, and r+ (or feel free to re-ask review)

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8736249 [details] [diff] [review]
[Part 3] (v2) Leverage socket-based session transport to establish presentation connection

Hopefully I'm not missing anything here.
This moves some code and mPresentationServer is handled similarly-ish to 
MulticastDNSDeviceProvider.

I really really wish we had some documentation for all this.


>+  nsresult rv;
>+
>   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>   MOZ_ASSERT(obs);
> 
>   obs->AddObserver(this, DISPLAY_CHANGED_EVENT, false);
> 
>   mDevice = new HDMIDisplayDevice(this);
> 
>+  mWrappedListener = new DisplayDeviceProviderWrappedListener();
>+  rv = mWrappedListener->SetListener(this);
since you don't use rv before this, you should define it here.

>+DisplayDeviceProvider::StartTCPService()
it would be nice if some of this code could be shared with MulticastDNSDeviceProvider but it isn't clear to me how.

>+DisplayDeviceProvider::OnSessionRequest(nsITCPDeviceInfo* aDeviceInfo,
>+                                      const nsAString& aUrl,
>+                                      const nsAString& aPresentationId,
>+                                      nsIPresentationControlChannel* aControlChannel)
nit, align params? (or is the line then too long?)

Comment 54

[Tommy Kuo (away forever...)]

(In reply to Olli Pettay [:smaug] from comment #52)
> >+DisplayDeviceProvider::HDMIDisplayDevice
> >+                     ::EstablishControlChannel(const nsAString& aUrl,
> >+                                               const nsAString& aPresentationId,
> >+                                               nsIPresentationControlChannel** aControlChannel)
> >+{
> >+  nsresult rv = OpenTopLevelWindow();
> >+  if (NS_WARN_IF(NS_FAILED(rv))) {
> >+    return rv;
> >+  }
> >+
> >+  if (NS_WARN_IF(!mProvider)) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> Why do we check !mProvider after OpenTopLevelWindow?
>
> >+  return mProvider->RequestSession(this, aUrl, aPresentationId, aControlChannel);
> I'd prefer if you had strong reference to the provider before calling any
> methods on it to guarantee it stays alive during the whole
> method call.

After openTopLevelWindow need to be called when this device is chosen. And then it need to get a control channel from mProvider's service.

And DisplayDeviceProvider hold a strong pointer to HDMIDisplayDevice. To avoid reference cycle, I use a weak pointer to DisplayDeviceProvider in HDMIDisplayDevice. When DisplayDeviceProvider is killed, HDMIDisplayDevice is useless even it is not killed. That's why I add a !mProvider before using it.

> >+#define DISPLAY_DEVICE_PROVIDER_CONTRACT_ID "@mozilla.org/presentation-device/displaydevice-provider;1"
> I wonder why we talk about DisplayDeviceProvider everywhere, and why not
> HDMIDeviceProvider? Wouldn't the latter be more clear?
> Up to you.

In comment #28, S.C. mentioned that we can support all types of displays (e.g. HDMI display / WiFi display).

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tommy Kuo [:KuoE0] from comment #54)

> And DisplayDeviceProvider hold a strong pointer to HDMIDisplayDevice. To
> avoid reference cycle, I use a weak pointer to DisplayDeviceProvider in
> HDMIDisplayDevice. When DisplayDeviceProvider is killed, HDMIDisplayDevice
> is useless even it is not killed. That's why I add a !mProvider before using
> it.
Sure, but I mean a local nsRefPtr<'put the right type here'> provider = mProvider;
provider->CallAMethod(); to ensure the object stays alive long enough.
What else is guaranteeing the object stays alive long enough?
(I could overworried here. But this is similar to cases where we in Gecko have kungfuDeathGrip to keep objects alive long enough)


> In comment #28, S.C. mentioned that we can support all types of displays
> (e.g. HDMI display / WiFi display).
ok , thanks

Comment 56

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8736247 [details] [diff] [review]
[Part 1] (v2) Update interface of nsIPresentationDevice, r=smaug

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

The nsIPresentationLocalDevice interface is also used in bug 1235123. You'll have to figure out the check-in order.

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +31,5 @@
>     */
>    nsIPresentationControlChannel establishControlChannel(in DOMString url,
>                                                          in DOMString presentationId);
> +  // Disconnect this device.
> +  void disconnect();

Need better comment here.

::: dom/presentation/interfaces/nsIPresentationLocalDevice.idl
@@ +4,5 @@
> +
> +#include "nsIPresentationDevice.idl"
> +
> +/*
> + * Local device.

Need better comment here.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +1054,5 @@
>  
> +NS_IMETHODIMP
> +MulticastDNSDeviceProvider::Device::Disconnect()
> +{
> +  return NS_OK;

add comment about why we do nothing here

Comment 57

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8736248 [details] [diff] [review]
[Part 2] (v2) Add DisplayDeviceProvider to Presentation API

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

f+ with @smaug's comment addressed.

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +34,5 @@
>    // Disconnect this device.
>    void disconnect();
>  };
> +
> +

remove this unnecessary modification.

Comment 58

[Tommy Kuo (away forever...)]

(In reply to Olli Pettay [:smaug] from comment #52)
> Comment on attachment 8736248 [details] [diff] [review]
> [Part 2] (v2) Add DisplayDeviceProvider to Presentation API
> >+
> >+  nsCOMPtr<nsIURI> remoteShellURL = new nsSimpleURI();
> >+  rv = remoteShellURL->SetSpec(prefValue);
> >+  if (NS_WARN_IF(NS_FAILED(rv))) {
> >+    return rv;
> >+  }
> >+
> >+  rv = remoteShellURL->SetRef(mWindowId);
> >+  if (NS_WARN_IF(NS_FAILED(rv))) {
> >+    return rv;
> >+  }
> 
> Please use NS_NewURI here.

Hi smaug, because this URI is a chrome URI. If I use NS_NewURI to create nsIURI object, it will call nsChromeProtocolHandler::NewURI to do that. And it create the nsStandardURI object. The nsStandardURI is always immutable, even if I call nsStadardURI::SetMutable(true). That let me cannot call nsStandardURI::SetRef(mWindowId).

See: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#2975-2982

I think I should use the origin way to create nsSImpleURI to do that. Or, do you have any way to make nsStandardURI be mutable?

Thanks!

Comment 59

[Tommy Kuo (away forever...)]

Attachment: [Part 1] (v3) Update interface of nsIPresentationDevice, r=smaug

All fixed address in comment 51, comment #56 and comment #57. Carry r+ from comment #51.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Could you create the uri with the ref, and then not use SetRef explicitly?

Comment 61

[Tommy Kuo (away forever...)]

(In reply to Olli Pettay [:smaug] from comment #60)
> Could you create the uri with the ref, and then not use SetRef explicitly?

So, do you mean that do string concat with ref and call NS_NewURI with the result of string concat? Is the following steps?

1. Concat string URI and ref
2. Create nsIURI object by NS_NewURI
3. Call nsIURI.GetSpec() to get the CString used to pass to WindowWatcher.OpenWindow

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Yeah, that.

(Curious, have you seen use of nsSimpleURI for chrome stuff elsewhere in codebase?)

Comment 63

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v3) Add DisplayDeviceProvider to Presentation API

(In reply to Olli Pettay [:smaug] from comment #62)
> Yeah, that.
> 
> (Curious, have you seen use of nsSimpleURI for chrome stuff elsewhere in
> codebase?)

Hmm..., I think all chrome URIs are handled by nsChromeProtocolHandler, so they always are nsStandardURI.

---

I think this patch is almost done! But for the caution, could you review again? Thanks for your help!

Comment 64

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Olli Pettay [:smaug] from comment #62)
> Yeah, that.
> 
> (Curious, have you seen use of nsSimpleURI for chrome stuff elsewhere in
> codebase?)

Mostly nsSimpleURI is used as a helper class for other protocol handler. The only one place found using nsSimpleURI directly is https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1575

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8742017 [details] [diff] [review]
[Part 2] (v3) Add DisplayDeviceProvider to Presentation API

Sorry about the delay.

Btw, an interdiff in this kind of case would have been quite nice, it would have shown how small the changes comparing to the previous patch are.

Comment 66

[Carsten Book [:Tomcat]]

has problems to apply (also with bug 1234492 applied)

adding 1208417 to series file
renamed 1208417 -> Bug-1208417-Part-1-hg.patch
applying Bug-1208417-Part-1-hg.patch
patching file dom/presentation/PresentationSessionInfo.cpp
Hunk #1 FAILED at 720
1 out of 1 hunks FAILED -- saving rejects to file dom/presentation/PresentationSessionInfo.cpp.rej
patching file dom/presentation/interfaces/moz.build
Hunk #1 FAILED at 5
1 out of 1 hunks FAILED -- saving rejects to file dom/presentation/interfaces/moz.build.rej
patch failed, unable to continue (try -v)

Comment 67

[Tommy Kuo (away forever...)]

Attachment: [Part 1] (v4) Update interface of nsIPresentationDevice, r=smaug

Fix patch failed. Carry r+ from comment #51.

Comment 68

[Tommy Kuo (away forever...)]

Attachment: [Part 2] (v4) Add DisplayDeviceProvider to Presentation API

Fix patch failed. Carry r+ from comment #65.

Comment 69

[Tommy Kuo (away forever...)]

Attachment: [Part 3] (v3) Leverage socket-based session transport to establish presentation connection

Fix patch failed. Carry r+ from comment #53.

Comment 70

[Tommy Kuo (away forever...)]

(In reply to Carsten Book [:Tomcat] from comment #66)
> has problems to apply (also with bug 1234492 applied)
> 
> adding 1208417 to series file
> renamed 1208417 -> Bug-1208417-Part-1-hg.patch
> applying Bug-1208417-Part-1-hg.patch
> patching file dom/presentation/PresentationSessionInfo.cpp
> Hunk #1 FAILED at 720
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/presentation/PresentationSessionInfo.cpp.rej
> patching file dom/presentation/interfaces/moz.build
> Hunk #1 FAILED at 5
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/presentation/interfaces/moz.build.rej
> patch failed, unable to continue (try -v)

Fixed.

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa9bd3a5d1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f95de9b438f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2da65057928

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eaa9bd3a5d1b
https://hg.mozilla.org/mozilla-central/rev/4f95de9b438f
https://hg.mozilla.org/mozilla-central/rev/c2da65057928

Comment 73

[Tommy Kuo (away forever...)]

Comment on attachment 8746406 [details] [diff] [review]
[Part 3] (v3) Leverage socket-based session transport to establish presentation connection

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1208417
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 74

[Tommy Kuo (away forever...)]

Comment on attachment 8746405 [details] [diff] [review]
[Part 2] (v4) Add DisplayDeviceProvider to Presentation API

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1208417
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 75

[Tommy Kuo (away forever...)]

Comment on attachment 8746404 [details] [diff] [review]
[Part 1] (v4) Update interface of nsIPresentationDevice, r=smaug

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1208417
User impact if declined: Every patches for Presentation API need two versions of them, one for m-c and another one for b2g.
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): I think there is no risk with this patch.
String or UUID changes made by this patch: uuid updated

Comment 76

[Josh Cheng [:josh]]

Comment on attachment 8746404 [details] [diff] [review]
[Part 1] (v4) Update interface of nsIPresentationDevice, r=smaug

Approve for TV 2.6

Comment 77

[Josh Cheng [:josh]]

Comment on attachment 8746405 [details] [diff] [review]
[Part 2] (v4) Add DisplayDeviceProvider to Presentation API

Approve for TV 2.6

Comment 78

[Josh Cheng [:josh]]

Comment on attachment 8746406 [details] [diff] [review]
[Part 3] (v3) Leverage socket-based session transport to establish presentation connection

Approve for TV 2.6

Comment 79

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/8b500eab52c5e223dccc4ac04aac4589696a694f