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

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655, 673942, 654110
Review-Url: https://codereview.chromium.org/2552343009
Cr-Commit-Position: refs/heads/master@{#444148}
Committed: https://chromium.googlesource.com/chromium/src/+/ad0eb0fa0ba570b4e41e28383dda35d1aca0a894

[zhaobin, 2016-12-09 01:51:07]

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

[mark a. foltz, 2016-12-09 05:38:04]

Thanks for doing this!  Overall looks good with a few minor comments based on
spec changes that have happened in the meantime.

PresentationDispatcher will need some fixes to handle availability for multiple
URLs per request.  Let's do that in a different patch.

I didn't review the layout tests closely in this round of review.

https://codereview.chromium.org/2552343009/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2552343009/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:248: // TODO(mfoltz):
Pass all URLs to PresentationService.
Can you change this to TODO(627655) ?

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:50:
controller->setDefaultRequestUrl(request ? request->urls()
Do we need to call setDefaultRequestUrl at all for an empty request?

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:135:
if (urls[i].isValid())
We should be throwing a SyntaxException in the ctor if there is an invalid URL

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:85:
exceptionState.throwTypeError("'" + urls[i] +
This should be a SyntaxError according to the spec

https://w3c.github.io/presentation-api/#constructing-a-presentationrequest

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:136: for
(const auto& url : m_urls) {
In the spec, we moved this check to the ctor, so it doesn't have to be repeated
in each PresentationRequest function :)

Fine to fix here or wait for another patch

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.h
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.h:22: class
MODULES_EXPORT PresentationRequest final
Why is MODULES_EXPORT required?  Is there something instantiating a
PresentationRequest outside of Blink?  (I don't know the module boundary so I'm
not sure when to use this.)

[whywhat, 2016-12-09 22:44:08]

avayvod@chromium.org changed reviewers:
+ avayvod@chromium.org

[whywhat, 2016-12-09 22:44:09]

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:50:
controller->setDefaultRequestUrl(request ? request->urls()
On 2016/12/09 at 05:38:03, mark a. foltz wrote:
> Do we need to call setDefaultRequestUrl at all for an empty request?

We probably do so that navigator.presentation.defaultRequest = null stops sink
observing and so on?

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.h
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.h:22: class
MODULES_EXPORT PresentationRequest final
On 2016/12/09 at 05:38:03, mark a. foltz wrote:
> Why is MODULES_EXPORT required?  Is there something instantiating a
PresentationRequest outside of Blink?  (I don't know the module boundary so I'm
not sure when to use this.)

It's needed for the unit tests (IIRC, in some build configurations on Windows).

[zhaobin, 2016-12-09 23:02:00]

https://codereview.chromium.org/2552343009/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2552343009/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_dispatcher.cc:248: // TODO(mfoltz):
Pass all URLs to PresentationService.
On 2016/12/09 05:38:03, mark a. foltz wrote:
> Can you change this to TODO(627655) ?

Done.

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:50:
controller->setDefaultRequestUrl(request ? request->urls()
On 2016/12/09 05:38:03, mark a. foltz wrote:
> Do we need to call setDefaultRequestUrl at all for an empty request?

Done.

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:135:
if (urls[i].isValid())
On 2016/12/09 05:38:03, mark a. foltz wrote:
> We should be throwing a SyntaxException in the ctor if there is an invalid URL

We have checked urls in PresentationRequest ctor, and rejected invalid urls
there. Urls should be valid when they arrive here, do we need to check again?

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:85:
exceptionState.throwTypeError("'" + urls[i] +
On 2016/12/09 05:38:03, mark a. foltz wrote:
> This should be a SyntaxError according to the spec
> 
> https://w3c.github.io/presentation-api/#constructing-a-presentationrequest

Done.

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:136: for
(const auto& url : m_urls) {
On 2016/12/09 05:38:03, mark a. foltz wrote:
> In the spec, we moved this check to the ctor, so it doesn't have to be
repeated
> in each PresentationRequest function :)
> 
> Fine to fix here or wait for another patch

Done.

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.h
(right):

https://codereview.chromium.org/2552343009/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.h:22: class
MODULES_EXPORT PresentationRequest final
On 2016/12/09 22:44:09, whywhat wrote:
> On 2016/12/09 at 05:38:03, mark a. foltz wrote:
> > Why is MODULES_EXPORT required?  Is there something instantiating a
> PresentationRequest outside of Blink?  (I don't know the module boundary so
I'm
> not sure when to use this.)
> 
> It's needed for the unit tests (IIRC, in some build configurations on
Windows).

Acknowledged.

[mlamouri (slow - plz ping), 2016-12-10 12:06:19]

This is a fairly large CL. Any chance it can be broken up in smaller CLs?

https://codereview.chromium.org/2552343009/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2552343009/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:43: return;
Wouldn't this prevent `navigator.presentation.defaultRequest = null;` to work?

[zhaobin, 2016-12-12 19:53:45]

Adding url property to PresentationConnection could be a seperate patch
(https://codereview.chromium.org/2423203002/).

Further break down is possible, waiting for @mfoltz for suggestions though.

https://codereview.chromium.org/2552343009/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/Presentation.cpp (right):

https://codereview.chromium.org/2552343009/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:43: return;
On 2016/12/10 12:06:18, mlamouri wrote:
> Wouldn't this prevent `navigator.presentation.defaultRequest = null;` to work?

Done.

https://codereview.chromium.org/2552343009/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/Presentation.cpp:43: return;
On 2016/12/10 12:06:18, mlamouri wrote:
> Wouldn't this prevent `navigator.presentation.defaultRequest = null;` to work?

yes, it will break defaultRequest = null. Reverted it back.

[mlamouri (slow - plz ping), 2016-12-13 13:50:53]

lgtm with comments applied. Feel free to ignore the nit/style comments.

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

https://codereview.chromium.org/2552343009/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:248: //
TODO(crbug.com/627655): Pass all URLs to PresentationService.
nit: usually, we do TODO(username) and mention the bug number in the comment.
Though, I don't mind TODO(bug) ;)

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:13:
navigator.presentation.defaultRequest = new
PresentationRequest(["https://example.org",
"cast://google.com/app_id=deadbeef"]);
What is this testing exactly? The test name says "multipleurls" but the actual
testing seems to check that reloading reset the `defaultRequest`.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:12: var
testUserGesture = function(t, requestArgument) {
Having the method called `testUserGesture` is confusing. I'm not sure where you
get the user gesture token too.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(left):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:127: }
You are fixing another bug here, right? Should it be added to BUG=?

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

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:21:
WTF::Vector<KURL> requestUrls = request->urls();
style: can you add a blank line before the `requestsUrls` definition. It will be
easier to read :)

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:22:
EXPECT_EQ(requestUrls.size(), static_cast<size_t>(1));
should this be (expected, actual)?

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:24:
EXPECT_EQ(requestUrls[0].getString(), "https://example.com/");
ditto

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:36:
WTF::Vector<KURL> requestUrls = request->urls();
style: ditto

[mark a. foltz, 2016-12-13 23:52:00]

In principle it is possible to land the controller/dispatcher changes in one
patch and then the module/idl changes as a second patch.

But, that would require converting singleton arguments from the API to vectors
in the first patch, then undoing those changes in the second patch.  I'm not
sure it's worth it, especially since all of the logic implementing the feature
has already been split into several other patches.

On the other hand, we do need to adjust the logic in the dispatcher to track
screen availability for multiple URLs per availability object, and that can be
done in a separate patch.  That might argue for splitting it up.

So I can see both sides to the argument :)

[zhaobin, 2016-12-13 23:56:04]

Description was changed from

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

Applying https://codereview.chromium.org/2148643002/

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

to

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

Applying https://codereview.chromium.org/2148643002/

BUG=627655,673942
==========

[zhaobin, 2016-12-14 01:51:54]

Resolving mlamouri's comments.

mfoltz@ multiple url support for getAvailability() has not been implemented yet.
That will be a seperate patch. Will keep this patch as it is.

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

https://codereview.chromium.org/2552343009/diff/40001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:248: //
TODO(crbug.com/627655): Pass all URLs to PresentationService.
On 2016/12/13 13:50:53, mlamouri wrote:
> nit: usually, we do TODO(username) and mention the bug number in the comment.
> Though, I don't mind TODO(bug) ;)

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:13:
navigator.presentation.defaultRequest = new
PresentationRequest(["https://example.org",
"cast://google.com/app_id=deadbeef"]);
On 2016/12/13 13:50:53, mlamouri wrote:
> What is this testing exactly? The test name says "multipleurls" but the actual
> testing seems to check that reloading reset the `defaultRequest`.

I think we test both navigation and mulitple urls in this test (we use multiple
url constructor to create defaultRequest).

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:12: var
testUserGesture = function(t, requestArgument) {
On 2016/12/13 13:50:53, mlamouri wrote:
> Having the method called `testUserGesture` is confusing. I'm not sure where
you
> get the user gesture token too.

Term "user gesture" may come from "The controlling user agent should initiate
presentation using the default presentation request only when the user has
expressed an intention to do so via a user gesture, for example by clicking a
button in the browser chrome." (https://w3c.github.io/presentation-api/)

PresentationRequest.start() will pop MR dialog, and it needs a user gesture to
continue (e.g. user selects a device). So the test is called testUserGesture...

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(left):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:127: }
On 2016/12/13 13:50:53, mlamouri wrote:
> You are fixing another bug here, right? Should it be added to BUG=?

Done.

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

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:21:
WTF::Vector<KURL> requestUrls = request->urls();
On 2016/12/13 13:50:53, mlamouri wrote:
> style: can you add a blank line before the `requestsUrls` definition. It will
be
> easier to read :)

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:22:
EXPECT_EQ(requestUrls.size(), static_cast<size_t>(1));
On 2016/12/13 13:50:53, mlamouri wrote:
> should this be (expected, actual)?

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:24:
EXPECT_EQ(requestUrls[0].getString(), "https://example.com/");
On 2016/12/13 13:50:53, mlamouri wrote:
> ditto

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:36:
WTF::Vector<KURL> requestUrls = request->urls();
On 2016/12/13 13:50:53, mlamouri wrote:
> style: ditto

Done.

[whywhat, 2016-12-14 02:20:39]

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:13:
navigator.presentation.defaultRequest = new
PresentationRequest(["https://example.org",
"cast://google.com/app_id=deadbeef"]);
On 2016/12/14 at 01:51:53, zhaobin wrote:
> On 2016/12/13 13:50:53, mlamouri wrote:
> > What is this testing exactly? The test name says "multipleurls" but the
actual
> > testing seems to check that reloading reset the `defaultRequest`.
> 
> I think we test both navigation and mulitple urls in this test (we use
multiple url constructor to create defaultRequest).

I think that just adding a ctor test in the presentation-api.html is enough for
this change. maybe getAvailability could be tested additionally for multiple
urls case when not all of them are incompatible or that the connection created
by start() has one of the URLs from the list - but that's for the follow up cls
and maybe additional test harness built (one can mock Mojo services in
LayoutTests now, for instance).

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:9: var
presentationUrl = "http://example.com";
I think in general your presentation-api.html addition covers all the changes
needed for this cl. We can maybe change the other tests to simply use a list of
urls instead of a single one to update them to the preferred ctor but I don't
feel there's a need to duplicate each test we have with a single URL atm.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:12: var
testUserGesture = function(t, requestArgument) {
On 2016/12/14 at 01:51:53, zhaobin wrote:
> On 2016/12/13 13:50:53, mlamouri wrote:
> > Having the method called `testUserGesture` is confusing. I'm not sure where
you
> > get the user gesture token too.
> 
> Term "user gesture" may come from "The controlling user agent should initiate
presentation using the default presentation request only when the user has
expressed an intention to do so via a user gesture, for example by clicking a
button in the browser chrome." (https://w3c.github.io/presentation-api/)
> 
> PresentationRequest.start() will pop MR dialog, and it needs a user gesture to
continue (e.g. user selects a device). So the test is called testUserGesture...

I think Mounir means that the name could be more specific, e.g.
testRequestStartThrowsWithoutUserGesture or similar. That would avoid his second
question about getting the gesture token (one would emulate a mouse click on a
button for that but since you're testing for the promise to be rejected, the
test must not get a token).

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:14:
request.start().catch(t.step_func(function(e) {
nit: you can use t.step_func_done and remove t.done() from the end.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:19: }));
you should probably also have then(t.unreached_func()) to verify the promise is
not resolved.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:22:
async_test(function(t) {
...or you could even use promise_test
(https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests) to
make the test even shorter :)

See
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/rem...
for an example.

[whywhat, 2016-12-14 02:21:41]

lgtm too btw

[zhaobin, 2016-12-14 03:00:27]

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:12: var
testUserGesture = function(t, requestArgument) {
On 2016/12/14 02:20:39, whywhat wrote:
> On 2016/12/14 at 01:51:53, zhaobin wrote:
> > On 2016/12/13 13:50:53, mlamouri wrote:
> > > Having the method called `testUserGesture` is confusing. I'm not sure
where
> you
> > > get the user gesture token too.
> > 
> > Term "user gesture" may come from "The controlling user agent should
initiate
> presentation using the default presentation request only when the user has
> expressed an intention to do so via a user gesture, for example by clicking a
> button in the browser chrome." (https://w3c.github.io/presentation-api/)
> > 
> > PresentationRequest.start() will pop MR dialog, and it needs a user gesture
to
> continue (e.g. user selects a device). So the test is called
testUserGesture...
> 
> I think Mounir means that the name could be more specific, e.g.
> testRequestStartThrowsWithoutUserGesture or similar. That would avoid his
second
> question about getting the gesture token (one would emulate a mouse click on a
> button for that but since you're testing for the promise to be rejected, the
> test must not get a token).

I misunderstood "token"...sorry for that.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:14:
request.start().catch(t.step_func(function(e) {
On 2016/12/14 02:20:39, whywhat wrote:
> nit: you can use t.step_func_done and remove t.done() from the end.

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:19: }));
On 2016/12/14 02:20:39, whywhat wrote:
> you should probably also have then(t.unreached_func()) to verify the promise
is
> not resolved.

Done.

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:22:
async_test(function(t) {
On 2016/12/14 02:20:39, whywhat wrote:
> ...or you could even use promise_test
> (https://github.com/w3c/testharness.js/blob/master/docs/api.md#promise-tests)
to
> make the test even shorter :)
> 
> See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/rem...
> for an example.

Done.

[whywhat, 2016-12-14 03:53:14]

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:12: var
testUserGesture = function(t, requestArgument) {
On 2016/12/14 at 03:00:27, zhaobin wrote:
> On 2016/12/14 02:20:39, whywhat wrote:
> > On 2016/12/14 at 01:51:53, zhaobin wrote:
> > > On 2016/12/13 13:50:53, mlamouri wrote:
> > > > Having the method called `testUserGesture` is confusing. I'm not sure
where
> > you
> > > > get the user gesture token too.
> > > 
> > > Term "user gesture" may come from "The controlling user agent should
initiate
> > presentation using the default presentation request only when the user has
> > expressed an intention to do so via a user gesture, for example by clicking
a
> > button in the browser chrome." (https://w3c.github.io/presentation-api/)
> > > 
> > > PresentationRequest.start() will pop MR dialog, and it needs a user
gesture to
> > continue (e.g. user selects a device). So the test is called
testUserGesture...
> > 
> > I think Mounir means that the name could be more specific, e.g.
> > testRequestStartThrowsWithoutUserGesture or similar. That would avoid his
second
> > question about getting the gesture token (one would emulate a mouse click on
a
> > button for that but since you're testing for the promise to be rejected, the
> > test must not get a token).
> 
> I misunderstood "token"...sorry for that.

No worries, IIUC it's a Blink specific terminology around the user gesture which
is again a somewhat special term roughly meaning user has to tap/click on the
page for some API to work :) We should've not used the term that might have been
unfamiliar to you in the first place.

[mark a. foltz, 2016-12-14 06:47:10]

Nice patch.

lgtm with one test case request.

Can you please copy and update the original CL description to include moving the
mixed-content check to the ctor?

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

https://codereview.chromium.org/2552343009/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:248: // TODO((mfoltz):
Pass all URLs to PresentationService. See crbug.com/627655.
Nit: Extra (

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

https://codereview.chromium.org/2552343009/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:58:
TEST(PresentationRequestTest, TestMultipleUrlConstructorMixedContent) {
Can you add a test case for mixed content with the single URL constructor?

[zhaobin, 2016-12-14 22:03:48]

Description was changed from

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

Applying https://codereview.chromium.org/2148643002/

BUG=627655,673942
==========

to

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to the PresentationRequest ctor

BUG=627655,673942
==========

[zhaobin, 2016-12-14 22:04:24]

Description was changed from

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to the PresentationRequest ctor

BUG=627655,673942
==========

to

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655,673942
==========

[zhaobin, 2016-12-15 00:48:33]

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

[commit-bot: I haz the power, 2016-12-15 00:49:13]

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

[zhaobin, 2016-12-15 00:49:21]

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

https://codereview.chromium.org/2552343009/diff/80001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:248: // TODO((mfoltz):
Pass all URLs to PresentationService. See crbug.com/627655.
On 2016/12/14 06:47:10, mark a. foltz wrote:
> Nit: Extra (

Done.

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

https://codereview.chromium.org/2552343009/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequestTest.cpp:58:
TEST(PresentationRequestTest, TestMultipleUrlConstructorMixedContent) {
On 2016/12/14 06:47:10, mark a. foltz wrote:
> Can you add a test case for mixed content with the single URL constructor?

Done.

[commit-bot: I haz the power, 2016-12-15 00:51:23]

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

[commit-bot: I haz the power, 2016-12-15 00:51:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[zhaobin, 2016-12-15 01:36:44]

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

[commit-bot: I haz the power, 2016-12-15 01:36:58]

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-12-15 01:42:39]

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

[commit-bot: I haz the power, 2016-12-15 01:42:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[imcheng, 2016-12-20 20:11:27]

lgtm

https://codereview.chromium.org/2552343009/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2552343009/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:72:
WTF::Vector<KURL> parsedUrls(urls.size());
We should handle the case of passing an empty array into the constructor. It
seems the code downstream (PresentationDispatcher) depends on it having at least
one URL. Ok to address in a separate patch with tests.

https://github.com/w3c/presentation-api/issues/327

[zhaobin, 2017-01-04 19:16:33]

zhaobin@chromium.org changed reviewers:
+ foolip@chromium.org

[zhaobin, 2017-01-04 19:16:33]

+foolip to review layout test files:

third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
   
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt

[zhaobin, 2017-01-04 19:18:28]

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

[commit-bot: I haz the power, 2017-01-04 19:18:48]

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, 2017-01-04 22:23:58]

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

[commit-bot: I haz the power, 2017-01-04 22:23:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zhaobin, 2017-01-04 23:42:46]

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

[commit-bot: I haz the power, 2017-01-04 23:43:11]

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

[haraken, 2017-01-04 23:53:51]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-01-04 23:53:52]

https://codereview.chromium.org/2552343009/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2552343009/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:68:
WTF::Vector<String> urls(1);

Nit: We won't need the WTF:: prefix.

[commit-bot: I haz the power, 2017-01-05 02:29:27]

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

[commit-bot: I haz the power, 2017-01-05 02:29:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zhaobin, 2017-01-06 18:30:34]

https://codereview.chromium.org/2552343009/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2552343009/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:68:
WTF::Vector<String> urls(1);
On 2017/01/04 23:53:52, haraken wrote:
> 
> Nit: We won't need the WTF:: prefix.

Done.

[zhaobin, 2017-01-06 18:30:47]

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

[commit-bot: I haz the power, 2017-01-06 18:31:18]

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, 2017-01-06 18:55:51]

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

[commit-bot: I haz the power, 2017-01-06 18:55:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[zhaobin, 2017-01-06 21:27:13]

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

[commit-bot: I haz the power, 2017-01-06 21:27:34]

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, 2017-01-06 21:53:32]

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

[commit-bot: I haz the power, 2017-01-06 21:53:33]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[zhaobin, 2017-01-10 21:56:47]

Patchset #14 (id:260001) has been deleted

[zhaobin, 2017-01-11 00:10:16]

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

[commit-bot: I haz the power, 2017-01-11 00:10:54]

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, 2017-01-11 01:47:48]

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

[commit-bot: I haz the power, 2017-01-11 01:47:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[foolip, 2017-01-11 16:26:10]

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-api.html:42: }, "Test
PresentationRequest API types for multiple URLs.");
This test would pass even without the new code I think because the array is
stringified to something which can be parsed as a URL. Can you also check
request.id?

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:11:
}, "Test that navigator.defaultRequest with multiple URLs isn't kept alive after
reload.")
Can you move this into an iframe that you navigate? I don't know if this
structure of test would work in web-platform-tests.
https://github.com/w3c/web-platform-tests/commit/6653d7ac9b1a56e3a712586ea8f0...
is an example of such a test.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:27: gc();
What object should this GC? The newly created PresentationRequest instance is
reachable and can't be collected.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.idl:17:
readonly attribute DOMString url;
It's a USVString in
https://w3c.github.io/presentation-api/#interface-presentationconnection, why
not here? You can change id too, because using USVString in this context
(readonly attribute) doesn't actually change the generated code)

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:5: //
Defined in:
https://w3c.github.io/presentation-api/#interface-presentationrequest
The "Defined in:" bit isn't needed, the mast majority of IDL files simply paste
a URL with no comment.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:11:
Constructor(DOMString[] urls),
Please match
https://w3c.github.io/presentation-api/#interface-presentationrequest

sequence<USVString> urls.

It would be good to change url as well, and this seems like a good a time as
any. If you write tests for the sequence variant, then it will be trivial to
test that as a drive-by too. In order to test the difference between DOMString
and USVString, you can use a string with a lone surrogate pair, and check that
it's replaced by a U+FFFD. This is defined in
https://heycam.github.io/webidl/#dfn-obtain-unicode

[zhaobin, 2017-01-12 00:19:31]

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-api.html:42: }, "Test
PresentationRequest API types for multiple URLs.");
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> This test would pass even without the new code I think because the array is
> stringified to something which can be parsed as a URL. Can you also check
> request.id?

Yes, this test will pass even without the new code. Unfortunately Request does
not expose id or url property, so there is no way to check that.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:11:
}, "Test that navigator.defaultRequest with multiple URLs isn't kept alive after
reload.")
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> Can you move this into an iframe that you navigate? I don't know if this
> structure of test would work in web-platform-tests.
>
https://github.com/w3c/web-platform-tests/commit/6653d7ac9b1a56e3a712586ea8f0...
> is an example of such a test.

It affects presentation-navigation.html and
presentation-navigation-multipleurls.html. Will fix it in a seperate patch.
Created crbug.com/680298.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:27: gc();
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> What object should this GC? The newly created PresentationRequest instance is
> reachable and can't be collected.

It is an existing test. It seems to test that
defaultRequest.onconnectionavailable is still available after gc(). Not sure why
we have this, but there are some similar gc-tests in layout test, e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/even....

Leave it as it is since it is not quite related to this CL?

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.idl:17:
readonly attribute DOMString url;
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> It's a USVString in
> https://w3c.github.io/presentation-api/#interface-presentationconnection, why
> not here? You can change id too, because using USVString in this context
> (readonly attribute) doesn't actually change the generated code)

Done.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:5: //
Defined in:
https://w3c.github.io/presentation-api/#interface-presentationrequest
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> The "Defined in:" bit isn't needed, the mast majority of IDL files simply
paste
> a URL with no comment.

Done.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:11:
Constructor(DOMString[] urls),
On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> Please match
> https://w3c.github.io/presentation-api/#interface-presentationrequest
> 
> sequence<USVString> urls.
> 
> It would be good to change url as well, and this seems like a good a time as
> any. If you write tests for the sequence variant, then it will be trivial to
> test that as a drive-by too. In order to test the difference between DOMString
> and USVString, you can use a string with a lone surrogate pair, and check that
> it's replaced by a U+FFFD. This is defined in
> https://heycam.github.io/webidl/#dfn-obtain-unicode

Done.

[zhaobin, 2017-01-12 19:02:42]

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

[commit-bot: I haz the power, 2017-01-12 19:03:26]

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, 2017-01-12 21:33:19]

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

[commit-bot: I haz the power, 2017-01-12 21:33:20]

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

[zhaobin, 2017-01-13 02:11:25]

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

[commit-bot: I haz the power, 2017-01-13 02:11:53]

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, 2017-01-13 04:08:43]

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

[commit-bot: I haz the power, 2017-01-13 04:08:44]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-01-13 20:29:27]

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-api.html:42: }, "Test
PresentationRequest API types for multiple URLs.");
On 2017/01/12 00:19:31, zhaobin wrote:
> On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > This test would pass even without the new code I think because the array is
> > stringified to something which can be parsed as a URL. Can you also check
> > request.id?
> 
> Yes, this test will pass even without the new code. Unfortunately Request does
> not expose id or url property, so there is no way to check that.

What does it take to get a PresentationConnection which could be used to check
this? Are there no tests like this as LayoutTests yet?

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-navigation-multipleurls.html:11:
}, "Test that navigator.defaultRequest with multiple URLs isn't kept alive after
reload.")
On 2017/01/12 00:19:31, zhaobin wrote:
> On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > Can you move this into an iframe that you navigate? I don't know if this
> > structure of test would work in web-platform-tests.
> >
>
https://github.com/w3c/web-platform-tests/commit/6653d7ac9b1a56e3a712586ea8f0...
> > is an example of such a test.
> 
> It affects presentation-navigation.html and
> presentation-navigation-multipleurls.html. Will fix it in a seperate patch.
> Created crbug.com/680298.

Acknowledged.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:27: gc();
On 2017/01/12 00:19:31, zhaobin wrote:
> On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > What object should this GC? The newly created PresentationRequest instance
is
> > reachable and can't be collected.
> 
> It is an existing test. It seems to test that
> defaultRequest.onconnectionavailable is still available after gc(). Not sure
why
> we have this, but there are some similar gc-tests in layout test, e.g.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/even....
> 
> Leave it as it is since it is not quite related to this CL?

You are expanding on the tests here, but if I'm correct the new test isn't
actually testing anything. Maybe ask whoever wrote the test originally what the
intent is?

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:11:
Constructor(DOMString[] urls),
On 2017/01/12 00:19:31, zhaobin wrote:
> On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > Please match
> > https://w3c.github.io/presentation-api/#interface-presentationrequest
> > 
> > sequence<USVString> urls.
> > 
> > It would be good to change url as well, and this seems like a good a time as
> > any. If you write tests for the sequence variant, then it will be trivial to
> > test that as a drive-by too. In order to test the difference between
DOMString
> > and USVString, you can use a string with a lone surrogate pair, and check
that
> > it's replaced by a U+FFFD. This is defined in
> > https://heycam.github.io/webidl/#dfn-obtain-unicode
> 
> Done.

Is this also impossible to test because the url doesn't show up anywhere without
actually connecting?

[zhaobin, 2017-01-14 00:46:06]

Add presentation-start.html layout test to address foolip's comments.

mlamouri@, do you know what is the intent of GC test in
Presentationrequest.html? foolip@ commented that "What object should this GC?
The newly created PresentationRequest instance is reachable and can't be
collected."

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentation-api.html (right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-api.html:42: }, "Test
PresentationRequest API types for multiple URLs.");
On 2017/01/13 20:29:27, foolip_slow_very_sorry wrote:
> On 2017/01/12 00:19:31, zhaobin wrote:
> > On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > > This test would pass even without the new code I think because the array
is
> > > stringified to something which can be parsed as a URL. Can you also check
> > > request.id?
> > 
> > Yes, this test will pass even without the new code. Unfortunately Request
does
> > not expose id or url property, so there is no way to check that.
> 
> What does it take to get a PresentationConnection which could be used to check
> this? Are there no tests like this as LayoutTests yet?

Done.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentationrequest.html
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentationrequest.html:27: gc();
On 2017/01/13 20:29:27, foolip_slow_very_sorry wrote:
> On 2017/01/12 00:19:31, zhaobin wrote:
> > On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > > What object should this GC? The newly created PresentationRequest instance
> is
> > > reachable and can't be collected.
> > 
> > It is an existing test. It seems to test that
> > defaultRequest.onconnectionavailable is still available after gc(). Not sure
> why
> > we have this, but there are some similar gc-tests in layout test, e.g.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/even....
> > 
> > Leave it as it is since it is not quite related to this CL?
> 
> You are expanding on the tests here, but if I'm correct the new test isn't
> actually testing anything. Maybe ask whoever wrote the test originally what
the
> intent is?

Asking mlamouri@.

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.idl
(right):

https://codereview.chromium.org/2552343009/diff/270001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.idl:11:
Constructor(DOMString[] urls),
On 2017/01/13 20:29:27, foolip_slow_very_sorry wrote:
> On 2017/01/12 00:19:31, zhaobin wrote:
> > On 2017/01/11 16:26:10, foolip_slow_very_sorry wrote:
> > > Please match
> > > https://w3c.github.io/presentation-api/#interface-presentationrequest
> > > 
> > > sequence<USVString> urls.
> > > 
> > > It would be good to change url as well, and this seems like a good a time
as
> > > any. If you write tests for the sequence variant, then it will be trivial
to
> > > test that as a drive-by too. In order to test the difference between
> DOMString
> > > and USVString, you can use a string with a lone surrogate pair, and check
> that
> > > it's replaced by a U+FFFD. This is defined in
> > > https://heycam.github.io/webidl/#dfn-obtain-unicode
> > 
> > Done.
> 
> Is this also impossible to test because the url doesn't show up anywhere
without
> actually connecting?

Done.

[zhaobin, 2017-01-14 01:39:47]

Patchset #16 (id:310001) has been deleted

[foolip, 2017-01-16 12:01:13]

lgtm for me, the GC bits seem odd but I'll leave to mlamouri@ to judge.

https://codereview.chromium.org/2552343009/diff/330001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/presentation/presentation-start.html
(right):

https://codereview.chromium.org/2552343009/diff/330001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-start.html:37: var
connectionUrl = "http://example.com/" + encodeURIComponent('\ufffd') +
"/example.html"
Nice, thanks for the test!

[zhaobin, 2017-01-17 18:31:13]

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

[commit-bot: I haz the power, 2017-01-17 18:32:09]

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

[zhaobin, 2017-01-17 18:41:37]

Description was changed from

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655,673942
==========

to

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655,673942,654110
==========

[commit-bot: I haz the power, 2017-01-17 20:04:43]

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

[commit-bot: I haz the power, 2017-01-17 20:04:44]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-01-17 21:19:53]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-01-17 21:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, avayvod@chromium.org, mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2552343009/#ps330001
(title: "resolve code review comments from foolip")

[commit-bot: I haz the power, 2017-01-17 21:20:15]

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, 2017-01-17 21:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1484687993887740,
"parent_rev": "3cf31809c9d342cfa7c1ae2844bdacf342dd39b9", "commit_rev":
"ad0eb0fa0ba570b4e41e28383dda35d1aca0a894"}

[commit-bot: I haz the power, 2017-01-17 21:26:28]

Description was changed from

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655,673942,654110
==========

to

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

Applying https://codereview.chromium.org/2148643002/

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
- Moves mixed-content check to PresentationRequest ctor

BUG=627655,673942,654110

Review-Url: https://codereview.chromium.org/2552343009
Cr-Commit-Position: refs/heads/master@{#444148}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad0eb0fa0ba570b4e41e28383dda...
==========

[commit-bot: I haz the power, 2017-01-17 21:26:30]

Committed patchset #16 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/ad0eb0fa0ba570b4e41e28383dda...