[Presentation API] Add support to content/ for multiple URLs per PresentationRequest.

[Presentation API] Add support to content/ for multiple URLs per PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

BUG=627655
Committed: https://crrev.com/fcad5262059e8b20621dd601a44ea5383b04cca0
Cr-Commit-Position: refs/heads/master@{#416433}

[mark a. foltz, 2016-07-22 19:20:50]

Description was changed from

==========
[WIP] [Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

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

to

==========
[WIP] [Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR

TODO: Update unittests

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

[mark a. foltz, 2016-07-22 23:00:58]

Description was changed from

==========
[WIP] [Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR

TODO: Update unittests

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

to

==========
[WIP] [Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

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

[mark a. foltz, 2016-07-22 23:00:58]

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

[mark a. foltz, 2016-07-22 23:01:06]

PTAL

[mark a. foltz, 2016-07-22 23:03:51]

Side note:

GMock has horrible support for move-only types.  Combined with endless type
conversions between blink::WebVector/mojo::Array/std::vector and WTF::String,
blink::WebString, mojo::String and std::string, this plumbing is full of
needlessly complex boilerplate.

[mlamouri (slow - plz ping), 2016-07-25 12:28:35]

lgtm. Note that if you want to reduce the plumbing, you should be able to call
the mojo service from Blink directly nowadays. It would allow you to remove the
content/renderer/ layer. It's not a trivial change to do though :)

[mark a. foltz, 2016-07-25 17:05:40]

Description was changed from

==========
[WIP] [Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

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

to

==========
[Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

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

[mark a. foltz, 2016-07-25 17:05:40]

mfoltz@chromium.org changed reviewers:
+ apacible@chromium.org
- imcheng@chromium.org

[mark a. foltz, 2016-07-25 17:05:53]

Derek is OOO.  Jennifer, can you PTAL?

[mark a. foltz, 2016-07-25 17:08:33]

On 2016/07/25 at 12:28:35, mlamouri wrote:
> lgtm. Note that if you want to reduce the plumbing, you should be able to call
the mojo service from Blink directly nowadays. It would allow you to remove the
content/renderer/ layer. It's not a trivial change to do though :)

Message queueing and other logic in presentation_dispatcher we would have to be
refactored into Blink.  It's possible, but as you say, nontrivial.

[apacible, 2016-07-26 00:39:44]

lgtm

https://codereview.chromium.org/2174693004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2174693004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:675: //
TODO(crbug.com/627655): Handle multiple URLs
nitty nit: here and below, period at the end of a comment.

[mark a. foltz, 2016-07-29 23:39:51]

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

[commit-bot: I haz the power, 2016-07-29 23:40:08]

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

[mark a. foltz, 2016-07-29 23:41:15]

mfoltz@chromium.org changed reviewers:
+ mkwst@chromium.org

[mark a. foltz, 2016-07-29 23:41:15]

+mkwst for presentation.mojom

I've rebased this patch on crrev.com/2181163003 which eliminates conversions in
PresentationServiceImpl and definitely simplifies the unit test.

PTAL

[commit-bot: I haz the power, 2016-07-30 03:11:53]

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

[commit-bot: I haz the power, 2016-07-30 03:11:54]

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

[dcheng, 2016-07-30 10:21:02]

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

[dcheng, 2016-07-30 10:21:03]

https://codereview.chromium.org/2174693004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2174693004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:58:
SetDefaultPresentationUrls(array<string> presentation_urls);
Please pass URLs as URL types. For Mojo, that is url.mojom.Url. This gets type
mapped to GURL in Chromium and KURL in Blink.

[mark a. foltz, 2016-08-18 18:37:19]

Okay, done.

[mark a. foltz, 2016-08-18 18:37:27]

On 2016/08/18 at 18:37:19, mfoltz_ooo_7-29_8-14 wrote:
> Okay, done.

PTAL.

[dcheng, 2016-08-24 22:14:19]

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

https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:546: url::mojom::UrlPtr
url = url::mojom::Url::New();
What's the status on typemapping? We shouldn't have to manually marshall stuff
like this.

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:41:
virtual void startSession(const WebVector<WebString>& presentationUrls,
WebPresentationConnectionClientCallbacks*) = 0;
These should be WebURLs as well, no?

[mark a. foltz, 2016-08-24 22:37:11]

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

https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:546: url::mojom::UrlPtr
url = url::mojom::Url::New();
On 2016/08/24 at 22:14:19, dcheng wrote:
> What's the status on typemapping? We shouldn't have to manually marshall stuff
like this.

Sorry I don't understand the question.  The revised API takes a UrlPtr for
|session_info|.

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:41:
virtual void startSession(const WebVector<WebString>& presentationUrls,
WebPresentationConnectionClientCallbacks*) = 0;
On 2016/08/24 at 22:14:19, dcheng wrote:
> These should be WebURLs as well, no?

We have dozens of files/APIs that need to be changed from string types to KURL,
WebURL, GURL, etc. and sweeping (if mechanical) type changes up and down the
stack is not the purpose of this patch.  This sort of change can be handled in a
separate patch that updates our code in Blink to use KURL/WebString.

We need to add multiple URL support to implement the current Presentation API
spec which is currently in CR.

[dcheng, 2016-08-24 23:12:17]

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

https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:546: url::mojom::UrlPtr
url = url::mojom::Url::New();
On 2016/08/24 22:37:11, mark a. foltz wrote:
> On 2016/08/24 at 22:14:19, dcheng wrote:
> > What's the status on typemapping? We shouldn't have to manually marshall
stuff
> like this.
> 
> Sorry I don't understand the question.  The revised API takes a UrlPtr for
> |session_info|.

At this point, it's quite unusual to use the raw mojo types directly. It's much
nicer for code to just deal with some native Blink C++ struct. Then we use a
typemap + StructTraits to turn it into the mojo wire format, and then the same
thing on the other end to turn it back into a native C++ struct.

This is important, because the StructTraits actually have some checks: for
example, for GURL, it checks that the resulting GURL is actually valid and that
doesn't exceed max chars:
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits.h?rcl=0&...

It also results in simpler client code. For example, let's say we have:

interface PageLoadMetrics {
  void ReportLoaded(url.mojom.Url url);
};

Without typemapping, I'd have to write something like this:
  url::mojom::UrlPtr url = url::mojom::Url::New();
  url->url = gurl.possibly_invalid_spec();
  metrics->ReportLoaded(std::move(url));

But with typemapping, I can just write this:
  metrics->ReportLoaded(gurl);

FWIW, there's basically already a bug filed for this:
https://bugs.chromium.org/p/chromium/issues/detail?id=632623

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:41:
virtual void startSession(const WebVector<WebString>& presentationUrls,
WebPresentationConnectionClientCallbacks*) = 0;
On 2016/08/24 22:37:11, mark a. foltz wrote:
> On 2016/08/24 at 22:14:19, dcheng wrote:
> > These should be WebURLs as well, no?
> 
> We have dozens of files/APIs that need to be changed from string types to
KURL,
> WebURL, GURL, etc. and sweeping (if mechanical) type changes up and down the
> stack is not the purpose of this patch.  This sort of change can be handled in
a
> separate patch that updates our code in Blink to use KURL/WebString.
> 
> We need to add multiple URL support to implement the current Presentation API
> spec which is currently in CR.

Can someone commit to doing this in an immediate followup? It's unfortunate this
was missed until now, but we really should be using richer types to describe
data when possible. That we have to write code like this:

 269   std::vector<std::string> presentation_urls_for_delegate(
 270       presentation_urls.size());
 271   std::transform(presentation_urls.begin(), presentation_urls.end(),
 272                  presentation_urls_for_delegate.begin(),
 273                  [](const url::mojom::UrlPtr& url) { return url->url; });

is quite unfortunate.

[mark a. foltz, 2016-08-25 17:11:17]

On 2016/08/24 at 23:12:17, dcheng wrote:
>
https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
> File content/renderer/presentation/presentation_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
> content/renderer/presentation/presentation_dispatcher.cc:546:
url::mojom::UrlPtr url = url::mojom::Url::New();
> On 2016/08/24 22:37:11, mark a. foltz wrote:
> > On 2016/08/24 at 22:14:19, dcheng wrote:
> > > What's the status on typemapping? We shouldn't have to manually marshall
stuff
> > like this.
> > 
> > Sorry I don't understand the question.  The revised API takes a UrlPtr for
> > |session_info|.
> 
> At this point, it's quite unusual to use the raw mojo types directly. It's
much nicer for code to just deal with some native Blink C++ struct. Then we use
a typemap + StructTraits to turn it into the mojo wire format, and then the same
thing on the other end to turn it back into a native C++ struct.
> 
> This is important, because the StructTraits actually have some checks: for
example, for GURL, it checks that the resulting GURL is actually valid and that
doesn't exceed max chars:
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits.h?rcl=0&...
> 
> It also results in simpler client code. For example, let's say we have:
> 
> interface PageLoadMetrics {
>   void ReportLoaded(url.mojom.Url url);
> };
> 
> Without typemapping, I'd have to write something like this:
>   url::mojom::UrlPtr url = url::mojom::Url::New();
>   url->url = gurl.possibly_invalid_spec();
>   metrics->ReportLoaded(std::move(url));
> 
> But with typemapping, I can just write this:
>   metrics->ReportLoaded(gurl);
> 
> FWIW, there's basically already a bug filed for this:
https://bugs.chromium.org/p/chromium/issues/detail?id=632623

I see.  Yes, this code should get cleaned up when we do the string -> *URL
conversion assuming the type converters don't introduce regressions.

> 
>
https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
> File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):
> 
>
https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
>
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:41:
virtual void startSession(const WebVector<WebString>& presentationUrls,
WebPresentationConnectionClientCallbacks*) = 0;
> On 2016/08/24 22:37:11, mark a. foltz wrote:
> > On 2016/08/24 at 22:14:19, dcheng wrote:
> > > These should be WebURLs as well, no?
> > 
> > We have dozens of files/APIs that need to be changed from string types to
KURL,
> > WebURL, GURL, etc. and sweeping (if mechanical) type changes up and down the
> > stack is not the purpose of this patch.  This sort of change can be handled
in a
> > separate patch that updates our code in Blink to use KURL/WebString.
> > 
> > We need to add multiple URL support to implement the current Presentation
API
> > spec which is currently in CR.
> 
> Can someone commit to doing this in an immediate followup? It's unfortunate
this was missed until now, but we really should be using richer types to
describe data when possible. That we have to write code like this:
> 
>  269   std::vector<std::string> presentation_urls_for_delegate(
>  270       presentation_urls.size());
>  271   std::transform(presentation_urls.begin(), presentation_urls.end(),
>  272                  presentation_urls_for_delegate.begin(),
>  273                  [](const url::mojom::UrlPtr& url) { return url->url; });
> 
> is quite unfortunate.

This cleanup is on our radar and we are committed to undertake it.  As I said,
it will take several phases to covert upstream and downstream APIs to use the
corresponding *URL types so it will not happen right away, but it will be
started soon.

[dcheng, 2016-08-26 18:13:33]

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:88:
std::vector<std::string> urls(1);
std::vector<std::string> urls = {"http://foo.fakeUrl"}; here and elsewhere will
save some typing

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:372:
render_process_id, routing_id, std::vector<std::string>(), callback);
Similarly, you can just put "{}" here instead of writing
std::vector<std::string>()

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:604:
service_->client_->OnScreenAvailabilityUpdated(std::move(url), available);
Since this is typemapped, I think you should be able to just use GURL? I think
this is true of all the other places in this CL that are now using UrlPtr.

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:155:
std::vector<url::mojom::UrlPtr> presentation_urls) override;
url.mojom.UrlPtr is already typemapped: does const std::vector<GURL>& work here
(and elsewhere in this interface impl) instead?

[mark a. foltz, 2016-08-29 23:54:55]

https://codereview.chromium.org/2174693004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2174693004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/presentation_service_delegate_impl.cc:675: //
TODO(crbug.com/627655): Handle multiple URLs
On 2016/07/26 at 00:39:44, apacible wrote:
> nitty nit: here and below, period at the end of a comment.

Done.

https://codereview.chromium.org/2174693004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/presentation/presentation.mojom
(right):

https://codereview.chromium.org/2174693004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/presentation.mojom:58:
SetDefaultPresentationUrls(array<string> presentation_urls);
On 2016/07/30 at 10:21:03, dcheng wrote:
> Please pass URLs as URL types. For Mojo, that is url.mojom.Url. This gets type
mapped to GURL in Chromium and KURL in Blink.

Done.

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:88:
std::vector<std::string> urls(1);
On 2016/08/26 at 18:13:33, dcheng wrote:
> std::vector<std::string> urls = {"http://foo.fakeUrl"}; here and elsewhere
will save some typing

Done.

https://codereview.chromium.org/2174693004/diff/140001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:372:
render_process_id, routing_id, std::vector<std::string>(), callback);
On 2016/08/26 at 18:13:33, dcheng wrote:
> Similarly, you can just put "{}" here instead of writing
std::vector<std::string>()

It doesn't allow you to take a reference to a temporary constant expression:

error: chosen constructor is explicit in copy-initialization

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/2174693004/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl.h:155:
std::vector<url::mojom::UrlPtr> presentation_urls) override;
On 2016/08/26 at 18:13:33, dcheng wrote:
> url.mojom.UrlPtr is already typemapped: does const std::vector<GURL>& work
here (and elsewhere in this interface impl) instead?

Done.

Oddly the generated mojom.h interface switched from url.mojom.UrlPtr to GURL
without any changes on my behalf.  Is this a newly landed Mojo feature?

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

https://codereview.chromium.org/2174693004/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:546: url::mojom::UrlPtr
url = url::mojom::Url::New();
On 2016/08/24 at 23:12:17, dcheng wrote:
> On 2016/08/24 22:37:11, mark a. foltz wrote:
> > On 2016/08/24 at 22:14:19, dcheng wrote:
> > > What's the status on typemapping? We shouldn't have to manually marshall
stuff
> > like this.
> > 
> > Sorry I don't understand the question.  The revised API takes a UrlPtr for
> > |session_info|.
> 
> At this point, it's quite unusual to use the raw mojo types directly. It's
much nicer for code to just deal with some native Blink C++ struct. Then we use
a typemap + StructTraits to turn it into the mojo wire format, and then the same
thing on the other end to turn it back into a native C++ struct.
> 
> This is important, because the StructTraits actually have some checks: for
example, for GURL, it checks that the resulting GURL is actually valid and that
doesn't exceed max chars:
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits.h?rcl=0&...
> 
> It also results in simpler client code. For example, let's say we have:
> 
> interface PageLoadMetrics {
>   void ReportLoaded(url.mojom.Url url);
> };
> 
> Without typemapping, I'd have to write something like this:
>   url::mojom::UrlPtr url = url::mojom::Url::New();
>   url->url = gurl.possibly_invalid_spec();
>   metrics->ReportLoaded(std::move(url));
> 
> But with typemapping, I can just write this:
>   metrics->ReportLoaded(gurl);
> 
> FWIW, there's basically already a bug filed for this:
https://bugs.chromium.org/p/chromium/issues/detail?id=632623

Now passing GURL into all the Mojo interfaces.

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2174693004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:41:
virtual void startSession(const WebVector<WebString>& presentationUrls,
WebPresentationConnectionClientCallbacks*) = 0;
On 2016/08/24 at 23:12:17, dcheng wrote:
> On 2016/08/24 22:37:11, mark a. foltz wrote:
> > On 2016/08/24 at 22:14:19, dcheng wrote:
> > > These should be WebURLs as well, no?
> > 
> > We have dozens of files/APIs that need to be changed from string types to
KURL,
> > WebURL, GURL, etc. and sweeping (if mechanical) type changes up and down the
> > stack is not the purpose of this patch.  This sort of change can be handled
in a
> > separate patch that updates our code in Blink to use KURL/WebString.
> > 
> > We need to add multiple URL support to implement the current Presentation
API
> > spec which is currently in CR.
> 
> Can someone commit to doing this in an immediate followup? It's unfortunate
this was missed until now, but we really should be using richer types to
describe data when possible. That we have to write code like this:
> 
>  269   std::vector<std::string> presentation_urls_for_delegate(
>  270       presentation_urls.size());
>  271   std::transform(presentation_urls.begin(), presentation_urls.end(),
>  272                  presentation_urls_for_delegate.begin(),
>  273                  [](const url::mojom::UrlPtr& url) { return url->url; });
> 
> is quite unfortunate.

Added TODO for WebString -> WebURL conversion.

[dcheng, 2016-08-30 05:47:32]

LGTM with comments addressed.

> Oddly the generated mojom.h interface switched from url.mojom.UrlPtr to GURL
> without any changes on my behalf.  Is this a newly landed Mojo feature?

I have no idea: typemapping is not a particularly recent feature. I guess
there's some magic somewhere that makes it Just Work, but I'm not that familiar
with those bits of the system.

https://codereview.chromium.org/2174693004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2174693004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:33:
const char* const kPresentationUrl1 = "http://foo.fakeUrl.com/";
Nit: const char kPresentationUrl1[] here and below (the meaning is not quite the
same)

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:53: const
char* const kPresentationId = "presentationId";
Ditto: const char[] is slightly preferable

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:552: urls[0]
= kPresentationUrl1;
Nit:I think initializer list syntax should also work here.

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

https://codereview.chromium.org/2174693004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:109:
WebVector<WebString> presentationUrls(static_cast<size_t>(1));
presentationUrl(1U) here and below

[mark a. foltz, 2016-08-30 19:57:54]

+sky for content/test/BUILD.gn?

https://codereview.chromium.org/2174693004/diff/200001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc
(right):

https://codereview.chromium.org/2174693004/diff/200001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl_unittest.cc:33:
const char* const kPresentationUrl1 = "http://foo.fakeUrl.com/";
On 2016/08/30 at 05:47:31, dcheng wrote:
> Nit: const char kPresentationUrl1[] here and below (the meaning is not quite
the same)

Done.

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:53: const
char* const kPresentationId = "presentationId";
On 2016/08/30 at 05:47:31, dcheng wrote:
> Ditto: const char[] is slightly preferable

Done.

https://codereview.chromium.org/2174693004/diff/200001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:552: urls[0]
= kPresentationUrl1;
On 2016/08/30 at 05:47:31, dcheng wrote:
> Nit:I think initializer list syntax should also work here.

Done.

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

https://codereview.chromium.org/2174693004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:109:
WebVector<WebString> presentationUrls(static_cast<size_t>(1));
On 2016/08/30 at 05:47:31, dcheng wrote:
> presentationUrl(1U) here and below

Done.

[mark a. foltz, 2016-08-30 19:58:05]

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

[mark a. foltz, 2016-08-30 19:58:33]

mfoltz@chromium.org changed reviewers:
+ sky@chromium.org

[commit-bot: I haz the power, 2016-08-30 19:58:38]

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

[sky, 2016-08-30 20:42:34]

sky@chromium.org changed reviewers:
+ jam@chromium.org
- sky@chromium.org

[sky, 2016-08-30 20:42:35]

sky->jam (content/test/BUILD.gn)

[commit-bot: I haz the power, 2016-08-30 21:02:39]

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

[commit-bot: I haz the power, 2016-08-30 21:02:40]

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

[jam, 2016-08-31 16:29:01]

lgtm

[mark a. foltz, 2016-09-02 18:50:55]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-09-02 18:50:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
apacible@chromium.org, mlamouri@chromium.org, dcheng@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2174693004/#ps240001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-02 18:51:16]

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-09-02 22:29:19]

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

[commit-bot: I haz the power, 2016-09-02 22:29:21]

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

[mark a. foltz, 2016-09-02 22:43:18]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2016-09-02 22:43:42]

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-09-03 03:08:50]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-09-03 03:10:41]

Description was changed from

==========
[Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

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

to

==========
[Presentation API] Add support to content/ for multiple URLs per
PresentationRequest.

This is the first part of a multi-part series of patches:

   1   This patch
   2-N Additional support in Blink, Media Router, UX, etc.
   N+1 https://codereview.chromium.org/2148643002 with Intent to Implement+Ship

This patch:

- Adds support to WebPresentationClient for passing multiple URLs to
startSession/joinSession/setDefaultPresentationUrls
- Updates PresentationService mojo interface
- Updates PresentationServiceImpl/PresentationServiceDispatcher
- Passes a single URL from Blink to WebPresentationClient
- Passes a single URL from PresentationServiceDispatcherImpl to MR
- Updates unittests

BUG=627655

Committed: https://crrev.com/fcad5262059e8b20621dd601a44ea5383b04cca0
Cr-Commit-Position: refs/heads/master@{#416433}
==========

[commit-bot: I haz the power, 2016-09-03 03:10:42]

Patchset 13 (id:??) landed as
https://crrev.com/fcad5262059e8b20621dd601a44ea5383b04cca0
Cr-Commit-Position: refs/heads/master@{#416433}