1228526 - [Presentation WebAPI] support device filtering in presentation device management

Status: RESOLVED FIXED

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

Description

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

Gecko support "app" protocol URL in PresentationRequest to launch the corresponding app installed on target device. However this request might failed eventually on device that:
  1) doesn't support "app" protocol (e.g. ChromeCast).
  2) doesn't install the app beforehand.

Gecko should be able to provide such kind of information on device selection list to help user choosing the appropriate device for presentation. On UI we can either grayed out the unsupported device or even exclude it from selection list.

Comment 1

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

I put the detail protocol design on https://wiki.mozilla.org/WebAPI/PresentationAPI:Protocol_Draft for device capability query.

Comment 2

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

Attachment: Bug 1228526 - Part 1, support device filtering by requested presentation URL.

Review commit: https://reviewboard.mozilla.org/r/73564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73564/

Comment 3

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

Attachment: Bug 1228526 - Part 2, show devices supported request URL in selection prompt dialog.

Review commit: https://reviewboard.mozilla.org/r/73566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73566/

Comment 4

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

@kershaw, you might want to take a look at this patch, as the foundation of bug 1288297.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8784207 [details]
> Bug 1228526 - Part 1, support device filtering by requested presentation URL.
> 
> @kershaw, you might want to take a look at this patch, as the foundation of
> bug 1288297.

Thanks. I'll modify my part based on this patch.

Comment 6

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

https://reviewboard.mozilla.org/r/73564/#review71600

::: dom/presentation/PresentationDeviceManager.cpp:189
(Diff revision 1)
> +    if (presentationUrls.IsEmpty()) {
> -    devices->AppendElement(mDevices[i], false);
> +      devices->AppendElement(mDevices[i], false);
> +      continue;
> +    }
> +
> +    for (uint32_t i = 0; i < presentationUrls.Length(); ++i) {

bug found by @kershaw, should not reuse "i".

::: dom/presentation/PresentationDeviceManager.cpp:195
(Diff revision 1)
> +      bool isSupported;
> +      if (NS_SUCCEEDED(mDevices[i]->IsRequestedUrlSupported(presentationUrls[i],
> +                                                            &isSupported)) &&
> +          isSupported) {
> +        devices->AppendElement(mDevices[i], false);
> +        continue;

Bug found by @kershaw, should use |break;| here.

Comment 7

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73564/diff/1-2/

Comment 8

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

Comment on attachment 8784208 [details]
Bug 1228526 - Part 2, show devices supported request URL in selection prompt dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73566/diff/1-2/

Comment 9

[C.M.Chang[:chunmin]]

Comment on attachment 8784208 [details]
Bug 1228526 - Part 2, show devices supported request URL in selection prompt dialog.

https://reviewboard.mozilla.org/r/73566/#review71628

Comment 10

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

https://reviewboard.mozilla.org/r/73564/#review71866

r- because of missing data: support and some helper method to check commonly used schemes.
Maybe it could be named something as silly as IsSchemeHttpHttpsData(nsIURL* aUrl)

Or explain why you don't want to support data:. I would assume it was rather useful for testing at least.

::: dom/presentation/provider/DisplayDeviceProvider.cpp:141
(Diff revision 2)
> +  if (NS_FAILED(rv) || !uri) {
> +    return NS_OK;
> +  }
> +
> +  nsAutoCString scheme;
> +  uri->GetScheme(scheme);

Don't you want to support also data: urls?

I'd like to see some helper method to check this kind of commonly supported schemes, so that each place supporting http would remember to check also https and possibly data:

::: dom/presentation/provider/LegacyMDNSDeviceProvider.cpp:47
(Diff revision 2)
>    PREF_PRESENTATION_DISCOVERY_TIMEOUT_MS,
>    PREF_PRESENTATION_DEVICE_NAME,
>    nullptr
>  };
>  
> +static const char* kFxTVPresentationAppUrls[] = {

Hmm, a bit ugly. At least add a comment that if this is changed, also MulticastDNSDeviceProvider.cpp
should be changed.
And add similar comment then also to MulticastDNSDeviceProvider.cpp

A bit less ugly would be to use some pref for these. Its value could be maybe space separated list of urls. But up to you.

Comment 11

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

https://reviewboard.mozilla.org/r/73564/#review71866

> Don't you want to support also data: urls?
> 
> I'd like to see some helper method to check this kind of commonly supported schemes, so that each place supporting http would remember to check also https and possibly data:

Sure I can move this to a helper function.
As for data: urls, we haven't figure out if it is secure enough to be supported on 2-UA device. We can file a follow-up bug to support it later.

> Hmm, a bit ugly. At least add a comment that if this is changed, also MulticastDNSDeviceProvider.cpp
> should be changed.
> And add similar comment then also to MulticastDNSDeviceProvider.cpp
> 
> A bit less ugly would be to use some pref for these. Its value could be maybe space separated list of urls. But up to you.

Move this to a helper function.

Comment 12

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73564/diff/2-3/

Comment 13

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

Comment on attachment 8784208 [details]
Bug 1228526 - Part 2, show devices supported request URL in selection prompt dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73566/diff/2-3/

Comment 14

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

Why data: url wouldn't be secure if http or https is? But sure, followup bug is fine.

Comment 15

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

https://reviewboard.mozilla.org/r/73564/#review71984

Comment 16

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

Comment on attachment 8784207 [details]
Bug 1228526 - Part 1, support device filtering by requested presentation URL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73564/diff/3-4/

Comment 17

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

Comment on attachment 8784208 [details]
Bug 1228526 - Part 2, show devices supported request URL in selection prompt dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73566/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e127d3ece92
Part 1, support device filtering by requested presentation URL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/bc6197438658
Part 2, show devices supported request URL in selection prompt dialog. r=chunmin

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e127d3ece92
https://hg.mozilla.org/mozilla-central/rev/bc6197438658