Gets presentation ID from route ID upon route creation, and overrides previous presentation ID with…

Gets presentation ID from route ID upon route creation, and overrides previous presentation ID with this final ID.

This is a quick fix to get presentation ID to web app before we overhaul route/sink/source URN format.

With https://codereview.chromium.org/1199933006/, https://codereview.chromium.org/1209473003/ and this CL, send/receive message works end to end.

Committed: https://crrev.com/2a5675a6315369246d50ee9c74d5f6376c3240d8
Cr-Commit-Position: refs/heads/master@{#336326}

[haibinlu, 2015-06-24 00:50:31]

haibinlu@chromium.org changed reviewers:
+ imcheng@chromium.org, kmarshall@chromium.org

[haibinlu, 2015-06-24 00:52:02]

Please duplicate changes as in https://codereview.chromium.org/1199933006/. I
will rebase once that CL is landed.

[Kevin M, 2015-06-24 23:02:07]

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

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/create_session_request.cc:16: const std::string&
presentation_id,
Remove this? I don't see any users of this data...

imcheng: are there any plans to use this in the future?

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/create_session_request.cc:20: :
presentation_info_(presentation_url, presentation_id),
Should be removed, there are no clients of it.

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

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.cc:36: std::pair<std::string,
std::string> GetPresentationIdAndUrl(
This needs a plain description of how the URLs are formatted.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.cc:44: size_t first_delim =
id.find("/");
Use a string splitting function (base/strings/string_split.h) so you don't have
to call find repeatedly, or compute substrings.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
Presentation API support functions should be in preso API files.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
Document the parameters and return type

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
If a caller is going to use half the pair but discard the other half, then this
should be split into 2 functions vs. returning a pair to save on unnecessary
processing.

I recommend just making an additional function that takes a computed
presentation URL and returns a StringPiece representing the source substr.

https://codereview.chromium.org/1202963004/diff/40001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1202963004/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:466:
on_session_messages_callback_->Run(
Add a comment indicating that branch is an error handler (it took some digging
for me to understand what empty array meant)

[haibinlu, 2015-06-24 23:25:13]

PTAL

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

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.cc:36: std::pair<std::string,
std::string> GetPresentationIdAndUrl(
On 2015/06/24 23:02:06, Kevin M wrote:
> This needs a plain description of how the URLs are formatted.

Done.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.cc:44: size_t first_delim =
id.find("/");
On 2015/06/24 23:02:06, Kevin M wrote:
> Use a string splitting function (base/strings/string_split.h) so you don't
have
> to call find repeatedly, or compute substrings.

string_split will has issue because the URL can have delim '/'.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
File chrome/browser/media/router/media_route.h (right):

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
On 2015/06/24 23:02:07, Kevin M wrote:
> If a caller is going to use half the pair but discard the other half, then
this
> should be split into 2 functions vs. returning a pair to save on unnecessary
> processing.
> 
> I recommend just making an additional function that takes a computed
> presentation URL and returns a StringPiece representing the source substr.

yes, there are lot more fields in the route-id. I do not want to do it in the
CL. Prefer a separate CL for refactoring source/sink/route URN.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
On 2015/06/24 23:02:07, Kevin M wrote:
> Presentation API support functions should be in preso API files.

Ack. Constructing presentation session message is done here. Moving it to
presention_service_delegate_impl makes sense, but incurs extra cost of blinding
message_callback to itself, pass media_router.mojo's route message format, etc.

To avoid using media_router.mojo's route message format in presentation files,
we need to create another class shared by both, an unnecessary cost for carrying
message along.

https://codereview.chromium.org/1202963004/diff/20001/chrome/browser/media/ro...
chrome/browser/media/router/media_route.h:83: // a pair of empty strings is
returned.
On 2015/06/24 23:02:07, Kevin M wrote:
> Document the parameters and return type

They are obvious in method signature. Not sure about the value of adding more
doc.

https://codereview.chromium.org/1202963004/diff/40001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/1202963004/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:466:
on_session_messages_callback_->Run(
On 2015/06/24 23:02:07, Kevin M wrote:
> Add a comment indicating that branch is an error handler (it took some digging
> for me to understand what empty array meant)

Done.

[Kevin M, 2015-06-25 22:51:56]

lgtm

https://codereview.chromium.org/1202963004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.cc (right):

https://codereview.chromium.org/1202963004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_route.cc:47: size_t prefix_len =
strlen(kRouteUrnPrefix);
Use arraysize() - 1, it's done once statically

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

https://codereview.chromium.org/1202963004/diff/100001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:471: for (size_t i =
0; i < messages->size(); ++i) {
for (const auto& next_message : *messages)

[imcheng (use chromium acct), 2015-06-25 23:25:13]

imcheng@google.com changed reviewers:
+ imcheng@google.com

[imcheng (use chromium acct), 2015-06-25 23:25:13]

lgtm

[imcheng, 2015-06-26 00:09:06]

lgtm

[haibinlu, 2015-06-26 00:11:51]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-06-26 00:11:51]

The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com,
imcheng@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1202963004/#ps120001
(title: " ")

[haibinlu, 2015-06-26 00:12:00]

https://codereview.chromium.org/1202963004/diff/100001/chrome/browser/media/r...
File chrome/browser/media/router/media_route.cc (right):

https://codereview.chromium.org/1202963004/diff/100001/chrome/browser/media/r...
chrome/browser/media/router/media_route.cc:47: size_t prefix_len =
strlen(kRouteUrnPrefix);
On 2015/06/25 22:51:56, Kevin M wrote:
> Use arraysize() - 1, it's done once statically

Done.

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

https://codereview.chromium.org/1202963004/diff/100001/content/browser/presen...
content/browser/presentation/presentation_service_impl.cc:471: for (size_t i =
0; i < messages->size(); ++i) {
On 2015/06/25 22:51:56, Kevin M wrote:
> for (const auto& next_message : *messages)

with "for", I can not reverse the array size. 

mojo::Array<presentation::SessionMessagePtr> mojoMessages(0); 
for(const auto& message : *messages)
      mojoMessages.push_back(ToMojoSessionMessage(message));

[commit-bot: I haz the power, 2015-06-26 00:12:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202963004/120001

[commit-bot: I haz the power, 2015-06-26 00:42:18]

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

[commit-bot: I haz the power, 2015-06-26 00:42:19]

Exceeded global retry quota

[haibinlu, 2015-06-26 01:08:40]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2015-06-26 01:08:40]

The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@google.com,
imcheng@chromium.org, kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1202963004/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-06-26 01:09:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202963004/140001

[commit-bot: I haz the power, 2015-06-26 02:35:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-26 02:36:30]

Patchset 8 (id:??) landed as
https://crrev.com/2a5675a6315369246d50ee9c74d5f6376c3240d8
Cr-Commit-Position: refs/heads/master@{#336326}