[Presentation API] Adds DOMString[] constructor to PresentationRequest.

Adds DOMString[] constructor to PresentationRequest.

This updates the Blink and platform layers of the Presentation API to support multiple URLs per PresentationRequest.  Until support is plumbed through to the service layer, only the first URL in the array will be passed through the PresentationService API.

This also:

- Updates layout tests to exercise the new constructor
- Adds a unit test to PresentationRequest
- Updates PresentationAvailabilityTest
- Adds a 'url' attribute to PresentationConnection

TODO: Intent to Implement/Ship

BUG=627655

[mark a. foltz, 2016-07-18 23:00:27]

Description was changed from

==========
[WIP] Support PresentationRequest(DOMString[]) in Blink.

BUG=627655
==========

to

==========
This updates the Blink and platform layers of the Presentation API to support
multiple URLs per PresentationRequest.


BUG=627655
==========

[mark a. foltz, 2016-07-18 23:10:10]

Description was changed from

==========
This updates the Blink and platform layers of the Presentation API to support
multiple URLs per PresentationRequest.


BUG=627655
==========

to

==========
Adds DOMString[] constructor to PresentationRequest.

This updates the Blink and platform layers of the Presentation API to support
multiple URLs per PresentationRequest.  Until support is plumbed through to the
service layer, only the first URL in the array will be passed through the
PresentationService API.

This also:

- Updates layout tests to exercise the new constructor
- Adds a unit test to PresentationRequest
- Updates PresentationAvailabilityTest
- Adds a 'url' attribute to PresentationConnection

TODO: Intent to Implement/Ship

BUG=627655
==========

[mark a. foltz, 2016-07-18 23:10:10]

mfoltz@chromium.org changed reviewers:
+ imcheng@chromium.org, mlamouri@chromium.org

[mark a. foltz, 2016-07-18 23:10:24]

Derek/Mounir: PTAL

[mark a. foltz, 2016-07-18 23:10:38]

The CQ bit was checked by mfoltz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 23:11:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 00:42:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 00:42:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-07-19 12:45:03]

Looks great. I think we only need to figure out what we want to send for intent
to implement and ship. Alternatively, if we can get this behind a
RuntimeEnabledFlag, we can delay most of the discussion (still requires Intent
to Implement).

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:108: // TODO(mfoltz):
Pass all URLs to PresentationService.
Should this be implemented first? or at least, I think we should have
DOMString[] ctor working by taking one URL that would be supported (instead of
the first in the list) in order to ship.

What I would recommend to minimise the boilerplate is to pass all the URLs to
the service and as soon as we reach the chrome/ layer, pick one and ignore all
the others.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:54:
}
style: we don't use { } when not needed usually.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:55:
m_urls.swap(data);
nit: I am probably missing something but could you use `m_urls` instead of
`data` above?

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:128: }
style: maybe no { } ?

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:5: [
We usually put a link on top of the IDL files for reference.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:31:
urls.append("cast://google.com/appid=deadbeef");
I'm genuinely surprised this is a valid URL :) I would expect something like
"cast://google.com/?appid=deadbeef". I wonder how "appid=deadbeef" is parsed.
Out of curiosity, could that be "cast://deadbeef" ? Sounds like the "google.com"
part isn't really needed, is it?

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:64:
// display for |availabilityUrl|.  The ownership of the callbacks argument
nit: |availabilityUrls|

[mark a. foltz, 2016-07-19 17:38:48]

Responded to comments.  I can start the backend patch in parallel, and also I'll
share a draft Intent to Implement soon.

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:108: // TODO(mfoltz):
Pass all URLs to PresentationService.
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> Should this be implemented first? or at least, I think we should have
DOMString[] ctor working by taking one URL that would be supported (instead of
the first in the list) in order to ship.
> 
> What I would recommend to minimise the boilerplate is to pass all the URLs to
the service and as soon as we reach the chrome/ layer, pick one and ignore all
the others.

We don't know which URL is valid yet (they all need to get passed to the MR to
create a sink -> URL map for the UI).

I could land a patch to modify the PresentationService and downstream code to
support multiple URLs first and land this afterwards, but we'd need to modify
the dispatcher to convert the single URL argument to an array.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:54:
}
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> style: we don't use { } when not needed usually.

Done.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:55:
m_urls.swap(data);
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> nit: I am probably missing something but could you use `m_urls` instead of
`data` above?

WebVector does not provide any method to resize itself once allocated.  To
populate the vector, the sample code suggests using swap().  See:

https://cs-staging.chromium.org/chromium/src/third_party/WebKit/public/platfo...

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:128: }
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> style: maybe no { } ?

Done.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:5: [
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> We usually put a link on top of the IDL files for reference.

Done.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:31:
urls.append("cast://google.com/appid=deadbeef");
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> I'm genuinely surprised this is a valid URL :) I would expect something like
"cast://google.com/?appid=deadbeef". I wonder how "appid=deadbeef" is parsed.
Out of curiosity, could that be "cast://deadbeef" ? Sounds like the "google.com"
part isn't really needed, is it?

Changed to cast:://deadbeef?param=foo and the test still passes.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:64:
// display for |availabilityUrl|.  The ownership of the callbacks argument
On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> nit: |availabilityUrls|

Done.

[mark a. foltz, 2016-07-19 17:39:35]

The CQ bit was checked by mfoltz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 17:39:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 19:00:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 19:00:54]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-07-20 13:02:10]

looks good to me but I think we should have something better than picking the
first entry of the list to ship.

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2148643002/diff/60001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:108: // TODO(mfoltz):
Pass all URLs to PresentationService.
On 2016/07/19 at 17:38:47, mark a. foltz wrote:
> On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> > Should this be implemented first? or at least, I think we should have
DOMString[] ctor working by taking one URL that would be supported (instead of
the first in the list) in order to ship.
> > 
> > What I would recommend to minimise the boilerplate is to pass all the URLs
to the service and as soon as we reach the chrome/ layer, pick one and ignore
all the others.
> 
> We don't know which URL is valid yet (they all need to get passed to the MR to
create a sink -> URL map for the UI).
> 
> I could land a patch to modify the PresentationService and downstream code to
support multiple URLs first and land this afterwards, but we'd need to modify
the dispatcher to convert the single URL argument to an array.

Maybe the simplest change would be to pass all the URLs to the
PresentationService then add a method in PresentationServiceImpl that ask the
PresentationServiceDelegate to pick one URL in the list? This way you minimise
the change but you still allow different embedders to have different
implementations. WDYT?

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:55:
m_urls.swap(data);
On 2016/07/19 at 17:38:47, mark a. foltz wrote:
> On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> > nit: I am probably missing something but could you use `m_urls` instead of
`data` above?
> 
> WebVector does not provide any method to resize itself once allocated.  To
populate the vector, the sample code suggests using swap().  See:
> 
>
https://cs-staging.chromium.org/chromium/src/third_party/WebKit/public/platfo...

I'm speechless :)

[imcheng, 2016-07-20 17:20:46]

lgtm overall, just some comments about corner cases and future work.

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp
(right):

https://codereview.chromium.org/2148643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationAvailability.cpp:55:
m_urls.swap(data);
On 2016/07/20 13:02:10, Mounir Lamouri wrote:
> On 2016/07/19 at 17:38:47, mark a. foltz wrote:
> > On 2016/07/19 at 12:45:03, Mounir Lamouri wrote:
> > > nit: I am probably missing something but could you use `m_urls` instead of
> `data` above?
> > 
> > WebVector does not provide any method to resize itself once allocated.  To
> populate the vector, the sample code suggests using swap().  See:
> > 
> >
>
https://cs-staging.chromium.org/chromium/src/third_party/WebKit/public/platfo...
> 
> I'm speechless :)

But you can initialize m_urls with a size param, right?

https://codereview.chromium.org/2148643002/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2148643002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:109: 
should this code check the given vector is not empty? and/or, the static factory
method for PresentationRequest? Same for other methods in this file.

https://codereview.chromium.org/2148643002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:337:
observer->availabilityChanged(available);
Add a TODO here to pass the availability_url into the observer/callback, which
will then decide the "overall" availability to send back to the page.

https://codereview.chromium.org/2148643002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/presentation/PresentationAvailabilityCallbacks.cpp
(right):

https://codereview.chromium.org/2148643002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationAvailabilityCallbacks.cpp:26:
void PresentationAvailabilityCallbacks::onSuccess(bool value)
I think some changes are required here in the future to pass the correct
availability value to the resolver. IIUC, we can resolve it with true if any url
is available, or false if all of them are unavailable.