In latest spec: Some presentation displays may only be able to display a subset of Web content because of functional, security or hardware limitations. Examples are set-top boxes, smart TVs or networked speakers capable of rendering only audio. We say that such a display is a compatible presentation display for a presentation request URL if the user agent can reasonably guarantee that the presentation of the URL on that display will succeed. We'll need following modification in order to support it: 1. define interface for capability filtering in device management subsystem. 2. define protocol for query device capability in 2-UA protocol.

Bug 1228526 will create capability filtering interface in nsIPresentationDevice. WebAPI / Presentation Service / Device Selection Prompt modifications will be put on this bug.

@sc, please take a look. Not sure if this patch is inside the range of this bug.

Comment on attachment 8793691 [details] [diff] [review] patch - v1 Review of attachment 8793691 [details] [diff] [review]: ----------------------------------------------------------------- The current algorithm for updating device availability is O(M*N), which M is number of availability objects and N is number of URLs in one availability object. This is might be a performance bottle neck since device monitoring is running periodically in background. I'd like to see an improvement on the big-O if it doesn't make the code too complex to read. ::: dom/presentation/PresentationAvailability.cpp @@ +80,5 @@ > + nsID uuid; > + uuidgen->GenerateUUIDInPlace(&uuid); > + char buffer[NSID_LENGTH]; > + uuid.ToProvidedString(buffer); > + CopyASCIItoUTF16(buffer, mAvailabilityId); Can use |nsIDToCString| if mAvailabilityId is a nsCString. ::: dom/presentation/PresentationAvailability.h @@ +65,5 @@ > nsTArray<RefPtr<Promise>> mPromises; > > nsTArray<nsString> mUrls; > + > + nsString mAvailabilityId; Use |nsCString| to reduce the memory consumption if possible. ::: dom/presentation/PresentationService.cpp @@ +433,5 @@ > + while (iter.HasMore()) { > + RefPtr<AvailabilityListenerWrapper> listener = iter.GetNext(); > + Unused << > + NS_WARN_IF(NS_FAILED(listener->MaybeNotifyAvailableChange(aDevice, > + aIsAvailable))); Maybe replace the array iteration with |NS_OBSERVER_ARRAY_NOTIFY_XPCOM_OBSERVERS|? ::: dom/presentation/interfaces/nsIPresentationService.idl @@ +161,4 @@ > * @param listener: The listener to register. > */ > + [noscript] void registerAvailabilityListener( > + in DOMString availabilityId, use |AUTF8String| if we change availabilityId to nsCString. ::: dom/presentation/ipc/PPresentation.ipdl @@ +88,5 @@ > async __delete__(); > > + async RegisterAvailabilityHandler(nsString aAvailabilityId, > + nsString[] aAvailabilityUrls); > + async UnregisterAvailabilityHandler(nsString aAvailabilityId); might use |nsCString| here as well. ::: dom/presentation/ipc/PresentationIPCService.h @@ +56,5 @@ > virtual ~PresentationIPCService(); > nsresult SendRequest(nsIPresentationServiceCallback* aCallback, > const PresentationIPCRequest& aRequest); > > + nsRefPtrHashtable<nsStringHashKey, You can use nsPtrHashKey<nsIPresentationAvailabilityListener>, so you don't need to generate availabilityId. For IPDL you can use a uint_64 step counter for muxing availability event on PPresentation. ::: dom/presentation/ipc/PresentationParent.h @@ +91,5 @@ > nsTArray<nsString> mSessionIdsAtController; > nsTArray<nsString> mSessionIdsAtReceiver; > nsTArray<uint64_t> mWindowIds; > ContentParentId mChildId; > + nsTArray<nsString> mAvailabilityIDs; s/ID/Id

Rebase and modifiy based on the last discussion.

SC, Please take a look. Thanks.

Comment on attachment 8801642 [details] [diff] [review] patch - v3 Review of attachment 8801642 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/PresentationAvailability.cpp @@ +166,5 @@ > + return NS_ERROR_FAILURE; > + } > + } > + > + mAvailableDevicesCount += aAvailabilityUrls.Length() * (aIsAvailable ? 1 : -1); This value is not the actual count of total available devices. For example, 1. availability object query for URL_1 and URL_2. 2. a new device added that supports both URL_1 and URL_2. mAvailabilityDevicesCount will be 2 instead of 1. This value actually represents the summation of device count for each URL. It would be better to rename this variable for clarity. @@ +169,5 @@ > + > + mAvailableDevicesCount += aAvailabilityUrls.Length() * (aIsAvailable ? 1 : -1); > + if (mAvailableDevicesCount < 0) { > + mAvailableDevicesCount = 0; > + } If it goes to 0, it means the accounting algorithm has flaw and we should assert it. ::: dom/presentation/PresentationService.cpp @@ +939,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + uint32_t length; nit: s/length/numOfDevices @@ +951,5 @@ > + bool isSupported; > + if (NS_SUCCEEDED(device->IsRequestedUrlSupported(url, &isSupported)) && > + isSupported) { > + availabilityUrlArray.AppendElement(url); > + } For each URL, we only need to find one device that supports it. I'll suggest to adjust the algorithm as following: availableUrls := empty array for (device in devices) { for (url in urls) { if (device.support(url)) { availableUrls.add(url); urls.remove(url); } } } ::: dom/presentation/PresentationServiceBase.h @@ +185,5 @@ > + function<bool(const nsAString&)> aIsUrlSupportedFunc, > + bool aAvailable) > + { > + nsClassHashtable<nsPtrHashKey<nsIPresentationAvailabilityListener>, > + nsTArray<nsString>> availabilityListenerTable; Add a typedef for this type. ::: dom/presentation/ipc/PresentationIPCService.cpp @@ +246,4 @@ > if (sPresentationChild) { > Unused << > + NS_WARN_IF(!sPresentationChild->SendRegisterAvailabilityHandler( > + aAvailabilityUrls)); The listener management doesn't matched with the accounting algorithm in PresentationAvailability. For example, if system has one device supports every URL: 1. 1st availability JS object is created for URL_1 2. IPC service save callback_1 for URL_1, and notify parent side 3. PresentationService save callback_parent for URL_1 and notify device available for URL_1 4. 1st availability JS object account +1 5. 2nd availability JS object is created for URL_1 and URL_2 6. IPC service save callback_2 for URL_1 and URL_2, and notify parent side 7. PresentationService save callback_parent for URL_2 and notify device available for URL_1 and URL_2 8. 1st availability JS object account +1 (which double count the device) and 2nd availability JS object account +2

Address based on the last comment. Please take a look. Thanks.

Comment on attachment 8803280 [details] [diff] [review] patch - v4 Review of attachment 8803280 [details] [diff] [review]: ----------------------------------------------------------------- In e10s scenario, the table of monitoring URL in content process might not reflect the change to chrome process after unregister. This data inconsistency will be error prone and might bite you in the future. I'll suggest to add |urls| in nsIPresentationService.unregisterAvailabilityListener to fine-grain control the availability listener in chrome process. ::: dom/presentation/PresentationAvailability.cpp @@ +174,4 @@ > return NS_DispatchToCurrentThread(NewRunnableMethod > <bool>(this, > &PresentationAvailability::UpdateAvailabilityAndDispatchEvent, > + available)); Need update mIsAvailable and only fire event when the overall availability is changed. Following code are required: ``` if (mIsAvailable == available) { return NS_OK; } mIsAvailable = available; return NS_Dispatch.... ``` ::: dom/presentation/PresentationService.cpp @@ +323,5 @@ > HandleShutdown(); > return NS_OK; > } else if (!strcmp(aTopic, PRESENTATION_DEVICE_CHANGE_TOPIC)) { > + NS_ConvertUTF16toUTF8 dataStr(aData); > + if (strcmp(dataStr.get(), "update") != 0) { Whitelisting "add" and "remove" would improve the readability. In addition, you can compare aData directly to unicode string primitive without conversion. I'll prefer: ``` if (!strcmp(aData, u"add") || !strcmp(aData, u"remove")) { return HandleDeviceChange(); } ``` @@ +411,5 @@ > + } > + > + nsCOMPtr<nsIArray> devices; > + rv = deviceManager->GetAvailableDevices(availabilityUrlArray, > + getter_AddRefs(devices)); Providing availabilityUrl in getAvailableDevices() will introduce unnecessary device filtering in PresentationDeviceManager. You can put nullptr here without creating an nsIArray. ::: dom/presentation/ipc/PresentationParent.cpp @@ +234,5 @@ > MOZ_ASSERT(mService); > + > + if (--mAvailabilityListenerCount == 0) { > + Unused << > + NS_WARN_IF(NS_FAILED(mService->UnregisterAvailabilityListener(this))); Register is happened every time but unregister only happened at the last one. This will cause AvailabilityManager still hold PresentationParent in the listener table. ::: dom/presentation/tests/mochitest/PresentationDeviceInfoChromeScript.js @@ +58,5 @@ > + return null; > + }, > + disconnect: function() {}, > + isRequestedUrlSupported: function(requestedUrl) { > + return true; suggest to create a partial support device, so that you can verify availability only fired on appropriate availability object in content process. ::: dom/presentation/tests/mochitest/test_presentation_availability.html @@ +138,5 @@ > + ok(true, "Should get onchange event."); > + aResolve(); > + } > + }); > + }), how about? ``` request1.getAvailability().then(function(aAvailability) { return new Promise(function(aResolve, aReject) { aAvailability.onchange = function() { aAvailability.onchange = null; ok(true, "Should get onchange event."); aResolve(); }; }); }, ```

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #8) > Comment on attachment 8803280 [details] [diff] [review] > patch - v4 > > Review of attachment 8803280 [details] [diff] [review]: > ----------------------------------------------------------------- > > In e10s scenario, the table of monitoring URL in content process might not > reflect the change to chrome process after unregister. This data > inconsistency will be error prone and might bite you in the future. I'll > suggest to add |urls| in > nsIPresentationService.unregisterAvailabilityListener to fine-grain control > the availability listener in chrome process. > Ok, I will try to do this in next patch. > ::: dom/presentation/PresentationAvailability.cpp > @@ +174,4 @@ > > return NS_DispatchToCurrentThread(NewRunnableMethod > > <bool>(this, > > &PresentationAvailability::UpdateAvailabilityAndDispatchEvent, > > + available)); > > Need update mIsAvailable and only fire event when the overall availability > is changed. Following code are required: > > ``` > if (mIsAvailable == available) { > return NS_OK; > } > > mIsAvailable = available; > > return NS_Dispatch.... > ``` > It seems that we already do the similar thing in [1]. Do you prefer to move this logic in |PresentationAvailability::NotifyAvailableChange|? [1] http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/dom/presentation/PresentationAvailability.cpp#172-174

(In reply to Kershaw Chang [:kershaw] from comment #9) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #8) > > ::: dom/presentation/PresentationAvailability.cpp > > @@ +174,4 @@ > > > return NS_DispatchToCurrentThread(NewRunnableMethod > > > <bool>(this, > > > &PresentationAvailability::UpdateAvailabilityAndDispatchEvent, > > > + available)); > > > > Need update mIsAvailable and only fire event when the overall availability > > is changed. Following code are required: > > > > ``` > > if (mIsAvailable == available) { > > return NS_OK; > > } > > > > mIsAvailable = available; > > > > return NS_Dispatch.... > > ``` > > > > It seems that we already do the similar thing in [1]. Do you prefer to move > this logic in |PresentationAvailability::NotifyAvailableChange|? > > [1] > http://searchfox.org/mozilla-central/rev/ > 84075be5067b68dc6cb3b89f999645650e68c05b/dom/presentation/ > PresentationAvailability.cpp#172-174 Ah, I didn't check |PresentationAvailability::UpdateAvailabilityAndDispatchEvent|. Your modification here is good enough.

Previous comments are addressed. Please take a look.

Comment on attachment 8804585 [details] [diff] [review] patch - v5 Review of attachment 8804585 [details] [diff] [review]: ----------------------------------------------------------------- f+ with my comment addressed. ::: dom/presentation/PresentationServiceBase.h @@ +190,5 @@ > + } > + > + if (aAvailabilityUrls.IsEmpty()) { > + return; > + } MOZ_ASSERT for aListener and aAvailabilityUrls check? ::: dom/presentation/interfaces/nsIPresentationService.idl @@ +171,1 @@ > * @param listener: The listener to unregister. nit: update the comment since a new parameter is introduced. ::: dom/presentation/ipc/PresentationIPCService.cpp @@ +248,4 @@ > if (sPresentationChild) { > Unused << > + NS_WARN_IF(!sPresentationChild->SendRegisterAvailabilityHandler( > + aAvailabilityUrls)); Why not doing similar check as in UnregisterAvailabilityListener? Only Send IPC for new added URLs can reduce IPC message and also guarantee no duplicate PresentationParent being registered for one URL. ::: dom/presentation/tests/mochitest/test_presentation_availability.html @@ +134,5 @@ > + request1.getAvailability().then(function(aAvailability) { > + aAvailability.onchange = function() { > + aAvailability.onchange = null; > + ok(true, "Should get onchange event."); > + aResolve(); This looks wrong to me. You should resolve only this test step but not the overall 'aResolve'. Correct code should looks like: ``` return Promise.all([ request1.getAvailability().then(function(aAvailability) { return new Promise(function(resolve) { aAvailability.onchange = function() { aAvailability.onchange = null; ok(true, "Should get onchange event."); resolve(); }; }); }), request2.getAvailability().then(function(aAvailability) { return new Promise(function(resolve) { aAvailability.onchange = function() { aAvailability.onchange = null; ok(true, "Should get onchange event."); resolve(); }; }); }), new Promise(function(aResolve) { gScript.sendAsyncMessage('trigger-add-multiple-devices'); aResolve(); }), ]).then(new Promise(function(aResolve) { gScript.sendAsyncMessage('trigger-remove-multiple-devices'); aResolve(); }); @@ +160,5 @@ > + let request1 = new PresentationRequest(["https://supportedUrl.com"]); > + let request2 = new PresentationRequest(["http://notSupportedUrl.com"]); > + > + return new Promise(function(aResolve, aReject) { > + Promise.all([ fix the promise chain here, too. ``` return Promise.all([...]) .then(new Promise(function(aResolve) { gScript.sendAsyncMessage('trigger-remove-https-devices'); aResolve(); }); ```

Fix the previous comments from sc.

(In reply to Kershaw Chang [:kershaw] from comment #14) > Created attachment 8804976 [details] [diff] [review] > patch - v6 > > Fix the previous comments from sc. Hi smaug, When your review queue open again, could you please have a look at this patch? Thanks.

Comment on attachment 8804976 [details] [diff] [review] patch - v6 > PresentationService::Observe(nsISupports* aSubject, > const char* aTopic, > const char16_t* aData) > { > if (!strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) { > HandleShutdown(); > return NS_OK; > } else if (!strcmp(aTopic, PRESENTATION_DEVICE_CHANGE_TOPIC)) { >- return HandleDeviceChange(); >+ if (!NS_strcmp(aData, u"add") || >+ !NS_strcmp(aData, u"remove")) { >+ return HandleDeviceChange(); >+ } This could use a comment why "update" isn't handled here >+ nsresult DoNotifyAvailableChange(const nsTArray<nsString>& aAvailabilityUrls, >+ bool aAvailable) >+ { >+ typedef nsClassHashtable<nsPtrHashKey<nsIPresentationAvailabilityListener>, >+ nsTArray<nsString>> ListenerToUrlsMap; >+ ListenerToUrlsMap availabilityListenerTable; >+ // Create a mapping from nsIPresentationAvailabilityListener to >+ // availabilityUrls. >+ for (auto it = mAvailbilityUrlTable.ConstIter(); !it.Done(); it.Next()) { >+ if (aAvailabilityUrls.Contains(it.Key())) { >+ nsCOMArray<nsIPresentationAvailabilityListener>* listenerArray = >+ it.UserData(); >+ for (uint32_t i = 0; i < listenerArray->Length(); ++i) { >+ nsIPresentationAvailabilityListener* listener = >+ listenerArray->ObjectAt(i); >+ nsTArray<nsString>* urlArray; >+ if (!availabilityListenerTable.Get(listener, &urlArray)) { >+ urlArray = new nsTArray<nsString>(); >+ availabilityListenerTable.Put(listener, urlArray); >+ } >+ urlArray->AppendElement(it.Key()); >+ } >+ } >+ } >+ >+ for (auto it = availabilityListenerTable.ConstIter(); !it.Done(); it.Next()) { >+ Unused << >+ NS_WARN_IF(NS_FAILED(it.Key()->NotifyAvailableChange(*it.UserData(), >+ aAvailable))); >+ } This looks a bit unsafe since availabilityListenerTable doesn't keep a strong reference to the key, and if NotifyAvailableChange happens to somehow remove the listener from mAvailabilityUrlTable, we're dealing with deleted objects. So, I think ListenerToUrlsMap should use nsISupportsHashKey or some such to ensure the key stays alive. >+ private: >+ // A mapping from an availabilityUrl to nsIPresentationAvailabilityListener. >+ nsClassHashtable<nsStringHashKey, >+ nsCOMArray<nsIPresentationAvailabilityListener>> mAvailbilityUrlTable; mAvailabilityUrlTable perhaps? (missing one 'a') >+ }; >+[ref] native URLArrayRef(const nsTArray<nsString>); >+ > [scriptable, uuid(0105f837-4279-4715-9d5b-2dc3f8b65353)] > interface nsIPresentationAvailabilityListener : nsISupports > { > /* > * Called when device availability changes. > */ >- void notifyAvailableChange(in bool available); >+ void notifyAvailableChange(in URLArrayRef urls, >+ in bool available); Shouldn't the method be [noscript], and then we could drop scriptable from the interface >- void registerAvailabilityListener(in nsIPresentationAvailabilityListener listener); >+ [noscript] void registerAvailabilityListener( >+ in URLArrayRef availabilityUrls, >+ in nsIPresentationAvailabilityListener listener); > > /* > * Unregister an availability listener. Must be called from the main thread. >+ * >+ * @param availabilityUrls: The Urls that are registered before. > * @param listener: The listener to unregister. > */ >- void unregisterAvailabilityListener(in nsIPresentationAvailabilityListener listener); >+ void unregisterAvailabilityListener( >+ in URLArrayRef availabilityUrls, >+ in nsIPresentationAvailabilityListener listener); Shouldn't also this be [noscript]

> ::: dom/presentation/ipc/PresentationIPCService.cpp > @@ +248,4 @@ > > if (sPresentationChild) { > > Unused << > > + NS_WARN_IF(!sPresentationChild->SendRegisterAvailabilityHandler( > > + aAvailabilityUrls)); > > Why not doing similar check as in UnregisterAvailabilityListener? Only Send > IPC for new added URLs can reduce IPC message and also guarantee no > duplicate PresentationParent being registered for one URL. > I found that we still need to do |SendRegisterAvailabilityHandler| every time when there is a new availability listener created in content process because the first |NotifyAvailableChange| is triggered by calling |RegisterAvailabilityListener| in parent process. What do you think?

(In reply to Kershaw Chang [:kershaw] from comment #17) > > ::: dom/presentation/ipc/PresentationIPCService.cpp > > @@ +248,4 @@ > > > if (sPresentationChild) { > > > Unused << > > > + NS_WARN_IF(!sPresentationChild->SendRegisterAvailabilityHandler( > > > + aAvailabilityUrls)); > > > > Why not doing similar check as in UnregisterAvailabilityListener? Only Send > > IPC for new added URLs can reduce IPC message and also guarantee no > > duplicate PresentationParent being registered for one URL. > > > I found that we still need to do |SendRegisterAvailabilityHandler| every > time when there is a new availability listener created in content process > because the first |NotifyAvailableChange| is triggered by calling > |RegisterAvailabilityListener| in parent process. > > What do you think? Two choices I can think of: 1) Cache the availability result for each URL in both content process and chrome process. Each time there is an availability listener registered, you can reply to listener with local information and ask for the missing part. In chrome process you can also use this information to reduce the number of queries while devices changes, e.g., query for only unavailable URLs while device added and query for only available URLs while device removed. 2) Create another IPDL method in PPresentation for querying availability on given URLs and add a method in PresentationService to update availability only to designated listener.

Add the cached availability in |mAvailabilityUrlTable| as the previous comment said. Please have a look. Thanks.

Comment on attachment 8806617 [details] [diff] [review] Add cached availability value in table Review of attachment 8806617 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/PresentationService.cpp @@ +325,5 @@ > // removal of the device. > + if (!NS_strcmp(aData, u"add")) { > + return HandleDeviceChange(true); > + } > + else if(!NS_strcmp(aData, u"remove")) { nit: remove new line after "{" @@ +383,5 @@ > } > } > > nsresult > +PresentationService::HandleDeviceChange(bool aAvailabile) I was thinking about separating device add and remove handling. In HandleDeviceAdded(aDevice), you can do: ``` mAvailabilityManager.GetUrlsByAvailability(urls, false); for (url of urls) { if (!aDevice->IsRequestedUrlSupported(url)) { urls.remove(url); } } mAvailabilityManager.DoNotifyAvailableChange(urls, true); ``` In HandleDeviceRemoved, you can do the same thing as the HandleDeviceChange. This optimization is optional and is up to you. ::: dom/presentation/PresentationServiceBase.h @@ +193,5 @@ > + // Urls. The availability value for new Urls will be updated in > + // |DoNotifyAvailableChange|. > + if (!aAddedUrls.IsEmpty()) { > + return; > + } The next `DoNotifyAvailableChange` will only update the availability information for new-added URLs. You still need to update the cached known available URLS to that listener. @@ +199,5 @@ > + // |NotifyAvailableChange| has already been called in content process, > + // no need to do it again in parent. > + if (XRE_IsParentProcess()) { > + return; > + } You do the same thing in both chrome and content process: always trigger NotifyAvailableChange for known available URLs, and trigger NotifyAvailableChange for new-added URLs after scanning through available devices in chrome process or receiving next NotifyAvailableChange in content process. @@ +261,5 @@ > if (aAvailabilityUrls.Contains(it.Key())) { > + ListenerPair* pair = it.UserData(); > + pair->first() = aAvailable; > + > + for (uint32_t i = 0; i < pair->second().Length(); ++i) { cache the result of pair->second() with following code: ``` const nsCOMArray<nsIPresentationAvailabilityListener>& listenerArray = pair->second(); ```

Address based on the previous comment.

Comment on attachment 8807488 [details] [diff] [review] patch - v7 Review of attachment 8807488 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/PresentationService.cpp @@ +469,5 @@ > + false); > + } > + > + return mAvailabilityManager.DoNotifyAvailableChange(supportedAvailabilityUrl, > + true); Now we only use this function to search for new unavailable URLs. This line can be removed if we can have a more precise function name. ::: dom/presentation/PresentationServiceBase.h @@ +170,5 @@ > + return; > + } > + > + aAddedUrls.Clear(); > + nsTArray<nsString> supportedAvailabilityUrl; rename to `knownAvailableUrls`, it's simpler and more meaningful. @@ +197,5 @@ > + if (aAddedUrls.IsEmpty()) { > + Unused << > + NS_WARN_IF( > + NS_FAILED(aListener->NotifyAvailableChange(aAvailabilityUrls, > + false))); The default availability value is `false`, so you have nothing to update. @@ +265,5 @@ > + } > + } > + } > + > + for (auto it = availabilityListenerTable.ConstIter(); !it.Done(); it.Next()) { Why not just use `Iter()`? then you don't need to do const_cast in next line.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #23) > Comment on attachment 8807488 [details] [diff] [review] > patch - v7 > > Review of attachment 8807488 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/presentation/PresentationService.cpp > @@ +469,5 @@ > > + false); > > + } > > + > > + return mAvailabilityManager.DoNotifyAvailableChange(supportedAvailabilityUrl, > > + true); > > Now we only use this function to search for new unavailable URLs. This line > can be removed if we can have a more precise function name. > This line is still needed since this function is also used in |PresentationService::RegisterAvailabilityListener|. We need this to notify the availability of newly added Urls. > ::: dom/presentation/PresentationServiceBase.h > @@ +170,5 @@ > > + return; > > + } > > + > > + aAddedUrls.Clear(); > > + nsTArray<nsString> supportedAvailabilityUrl; > > rename to `knownAvailableUrls`, it's simpler and more meaningful. > > @@ +197,5 @@ > > + if (aAddedUrls.IsEmpty()) { > > + Unused << > > + NS_WARN_IF( > > + NS_FAILED(aListener->NotifyAvailableChange(aAvailabilityUrls, > > + false))); > > The default availability value is `false`, so you have nothing to update. > Still need to update the availability result even if the values are all false, otherwise the promise returned by |getAvailability| would not be resolved. > @@ +265,5 @@ > > + } > > + } > > + } > > + > > + for (auto it = availabilityListenerTable.ConstIter(); !it.Done(); it.Next()) { > > Why not just use `Iter()`? then you don't need to do const_cast in next line. Yes, I can just use 'Iter()' here. In addition, the static_cast is for converting nsISupports to nsIPresentationAvailabilityListener.

Hi smaug, This patch will need your review again. The major difference is that we also cache the availability value for each url in AvailabilityManager. Please take a look. Thanks.

Comment on attachment 8808062 [details] [diff] [review] patch - v7.1 >+ aAddedUrls.Clear(); >+ nsTArray<nsString> knownAvailableUrls; >+ for (const auto& url : aAvailabilityUrls) { >+ ListenerPair* pair; >+ if (!mAvailabilityUrlTable.Get(url, &pair)) { >+ nsCOMArray<nsIPresentationAvailabilityListener> listenerArray; >+ pair = new ListenerPair(false, listenerArray); >+ mAvailabilityUrlTable.Put(url, pair); >+ aAddedUrls.AppendElement(url); >+ } >+ if (!pair->second().Contains(aListener)) { >+ pair->second().AppendElement(aListener); >+ } >+ if (pair->first()) { >+ knownAvailableUrls.AppendElement(url); >+ } Good example how horrible API 'Pair' is from readability point of view. http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/mfbt/Pair.h#142-144 Up to you, but I would just use a struct or class with properly named member variables or getter methods >+ void RemoveAvailabilityListener( >+ const nsTArray<nsString>& aAvailabilityUrls, >+ nsIPresentationAvailabilityListener *aListener, Nit, * goes with the type

- Address reviewer's comment. - Carry reviewer's name.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4975b2d475fb Filter the device availability by URL. r=smaug