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

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

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.

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.

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

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

(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?

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

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?

QI to nsPIDOMWindow and call Close on that.

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

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.

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).

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

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

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!

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.

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 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 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 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.

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

(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.

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.)

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.

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

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 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;

Add reviewer's name. Carry r+.

> > +#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!

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

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.

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 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.

(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"

Some changes on header files according the part 2.

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 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.

(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.

Add a new interface nsIPresentationLocalDevice inherited nsIPresentationDevice.

Changes based on Part 1 (Use nsIPresentationLocalDevice).

Changes based on Part 1 and Part 2.

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 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 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?)

(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).

(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 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 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.

(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!

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

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

(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

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

(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!

(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 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.

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)

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

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

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

(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.

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

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 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 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 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 on attachment 8746404 [details] [diff] [review] [Part 1] (v4) Update interface of nsIPresentationDevice, r=smaug Approve for TV 2.6

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

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