[Presentation API] Mojo typemap for content::PresentationConnectionMessage

Converts presentation.mojom to use a typemap for content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can be derived from which Optional field is set.

4. Tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as Cast and non-Cast sinks have different limits.





BUG=632623
Review-Url: https://codereview.chromium.org/2706463002
Cr-Commit-Position: refs/heads/master@{#455143}
Committed: https://chromium.googlesource.com/chromium/src/+/0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a

[mark a. foltz, 2017-02-23 20:50:04]

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

[commit-bot: I haz the power, 2017-02-23 20:50:33]

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, 2017-02-23 21:05:32]

Description was changed from

==========
[WIP] [PresentationAPI] Typemapping for PresentationConnectionMessage

BUG=632623
==========

to

==========
Converts presentation.mojom to use a typemap for content::PresentationConnection
Message.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. MediaRouter no longer takes a unique_ptr for SendBinaryMessage().

5. A lot of tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

[commit-bot: I haz the power, 2017-02-23 21:26:29]

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

[commit-bot: I haz the power, 2017-02-23 21:26:30]

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

[mark a. foltz, 2017-02-23 22:59:25]

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

[commit-bot: I haz the power, 2017-02-23 23:00:13]

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-02-24 00:44:44]

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

[commit-bot: I haz the power, 2017-02-24 00:44:46]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2017-02-24 01:19:19]

mfoltz@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, hubbe@chromium.org,
imcheng@chromium.org

[mark a. foltz, 2017-02-24 01:19:20]

PTAL

imcheng@: Everything
hubbe@:   chrome/browser/media/cast_*
dcheng@:  mojom changes
avi@:     content/public

[mark a. foltz, 2017-02-24 01:20:41]

Description was changed from

==========
Converts presentation.mojom to use a typemap for content::PresentationConnection
Message.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. MediaRouter no longer takes a unique_ptr for SendBinaryMessage().

5. A lot of tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

to

==========
Converts presentation.mojom to use a typemap for
content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. MediaRouter no longer takes a unique_ptr for SendBinaryMessage().

5. A lot of tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

[Avi (use Gerrit), 2017-02-24 04:08:51]

lgtm

https://codereview.chromium.org/2706463002/diff/100001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2706463002/diff/100001/content/public/browser...
content/public/browser/presentation_service_delegate.h:30: // Param #0: a vector
of messages that are received.
Drop the "#0" now that there's only one?

[imcheng, 2017-02-25 01:08:41]

lgtm, a few comments

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:185: message.binary =
std::move(data);
Since data is a const ref, doesn't that mean we are still making a copy here?

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
File content/public/common/presentation_connection_message.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:19: :
message(std::move(message)) {}
I believe this is still making a copy of the input string - the input needs to
be non const for std::move to actually "move".

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

https://codereview.chromium.org/2706463002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:79:
PresentationConnectionMessage connection_message,
can this be PresentationConnectionMessage&& ?

[dcheng, 2017-02-25 07:17:27]

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:185: message.binary =
std::move(data);
On 2017/02/25 01:08:40, imcheng wrote:
> Since data is a const ref, doesn't that mean we are still making a copy here?

Yes =)

https://codereview.chromium.org/2706463002/diff/100001/content/common/present...
File content/common/presentation/presentation_struct_traits.h (right):

https://codereview.chromium.org/2706463002/diff/100001/content/common/present...
content/common/presentation/presentation_struct_traits.h:227:
content::PresentationConnectionMessage* out) {
Please out-of-line this method.

https://codereview.chromium.org/2706463002/diff/100001/content/common/present...
content/common/presentation/presentation_struct_traits.h:229: out->message =
std::string();
This is unnecessary.

https://codereview.chromium.org/2706463002/diff/100001/content/common/present...
content/common/presentation/presentation_struct_traits.h:237: out->data =
std::vector<uint8_t>();
Nit: please omit this.

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
File content/public/common/presentation_connection_message.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:28: data =
std::move(other.data);
It's legal to just write:

data = std::move(other.data);
message = std::move(other.message);

which means this can just be written:

PCM::PCM(PCM&&) = default;

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:41: return
!this->is_binary() && this->message == other.message;
Or just write return this->data == other.data && this->message ==
other->message;

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:49: this->data =
std::move(other.data);
Ditto to the move ctor comments: it's legal to just unconditionally move both
fields (at which point this can be defaulted)

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

https://codereview.chromium.org/2706463002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:729: return new
SendMessageRequest(session_info,
Please add a TODO to make this and the function below return std::unique_ptr

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

https://codereview.chromium.org/2706463002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:79:
PresentationConnectionMessage connection_message,
On 2017/02/25 01:08:40, imcheng wrote:
> can this be PresentationConnectionMessage&& ?

Chromium style is to use pass-by-value for move-only types, and use std::move().

[mark a. foltz, 2017-02-27 20:43:02]

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

[commit-bot: I haz the power, 2017-02-27 20:43:42]

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-02-27 20:46:41]

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

[commit-bot: I haz the power, 2017-02-27 20:46:42]

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

[mark a. foltz, 2017-02-27 23:24:54]

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

[mark a. foltz, 2017-02-27 23:25:15]

PTAL

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:185: message.binary =
std::move(data);
On 2017/02/25 at 07:17:27, dcheng wrote:
> On 2017/02/25 01:08:40, imcheng wrote:
> > Since data is a const ref, doesn't that mean we are still making a copy
here?
> 
> Yes =)

Done

https://codereview.chromium.org/2706463002/diff/100001/content/public/browser...
File content/public/browser/presentation_service_delegate.h (right):

https://codereview.chromium.org/2706463002/diff/100001/content/public/browser...
content/public/browser/presentation_service_delegate.h:30: // Param #0: a vector
of messages that are received.
On 2017/02/24 at 04:08:51, Avi wrote:
> Drop the "#0" now that there's only one?

Done.

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
File content/public/common/presentation_connection_message.cc (right):

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:19: :
message(std::move(message)) {}
On 2017/02/25 at 01:08:40, imcheng wrote:
> I believe this is still making a copy of the input string - the input needs to
be non const for std::move to actually "move".

Done.

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:28: data =
std::move(other.data);
On 2017/02/25 at 07:17:27, dcheng wrote:
> It's legal to just write:
> 
> data = std::move(other.data);
> message = std::move(other.message);
> 
> which means this can just be written:
> 
> PCM::PCM(PCM&&) = default;

Done.

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:41: return
!this->is_binary() && this->message == other.message;
On 2017/02/25 at 07:17:27, dcheng wrote:
> Or just write return this->data == other.data && this->message ==
other->message;

Done.

https://codereview.chromium.org/2706463002/diff/100001/content/public/common/...
content/public/common/presentation_connection_message.cc:49: this->data =
std::move(other.data);
On 2017/02/25 at 07:17:27, dcheng wrote:
> Ditto to the move ctor comments: it's legal to just unconditionally move both
fields (at which point this can be defaulted)

Done.

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

https://codereview.chromium.org/2706463002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:729: return new
SendMessageRequest(session_info,
On 2017/02/25 at 07:17:27, dcheng wrote:
> Please add a TODO to make this and the function below return std::unique_ptr

Done.

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

https://codereview.chromium.org/2706463002/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:79:
PresentationConnectionMessage connection_message,
On 2017/02/25 at 07:17:27, dcheng wrote:
> On 2017/02/25 01:08:40, imcheng wrote:
> > can this be PresentationConnectionMessage&& ?
> 
> Chromium style is to use pass-by-value for move-only types, and use
std::move().

Acknowledged.

[commit-bot: I haz the power, 2017-02-27 23:25:27]

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-02-28 01:49:55]

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

[commit-bot: I haz the power, 2017-02-28 01:49:56]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-28 06:45:25]

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

[dcheng, 2017-02-28 06:45:27]

+yzshen for mojo utf8 string question

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.cc:178:
base::android::ConvertUTF8ToJavaString(env, message);
There's probably not a lot of gain to using move semantics here: we can't use
move semantics to move ownership of the data into Java. In addition, every layer
that passes by value has an additional bit of code to run the destructor, which
makes the binary a bit bigger.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:174: message.text =
text;
Either use std::move() or pass by const ref. Given the Android limitations, I
suspect using const ref may be better, but I'm not an expert in this code.

(Ditto below)

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/browser_presentation_connection_proxy.cc
(right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/browser_presentation_connection_proxy.cc:35:
router_->SendRouteBinaryMessage(route_id_, message.data.value(),
This has to copy anyway, since |message| is const. So const ref is better here.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:339:
base::Unretained(this), route_id, std::move(message),
Ah... I guess this is where move semantics might be useful. However, this should
be using base::Passed(). std::move() transfers ownership into the callback, but
the callback won't transfer callback to the invoked function, so it ends up
copying there.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:958:
router_->SendRouteBinaryMessage(route_id, std::move(message.data.value()),
Note that this doesn't move, since |message| is a const ref.

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
File content/common/presentation/presentation_struct_traits.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:38: if
(!base::IsStringUTF8(out->message) ||
I think Mojo requires that strings are utf-8. +yzshen to confirm

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:51: //    out->message
= std::string();
Nit: remove commented out code here and below

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
File content/common/presentation/presentation_struct_traits.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.h:16: //#include
"mojo/common/common_custom_types_struct_traits.h"
Nit: remove commented out code

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
File content/public/common/presentation_connection_message.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
content/public/common/presentation_connection_message.h:46: };
Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is intended to be
move-only

[yzshen1, 2017-02-28 18:22:07]

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
File content/common/presentation/presentation_struct_traits.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:38: if
(!base::IsStringUTF8(out->message) ||
On 2017/02/28 06:45:27, dcheng wrote:
> I think Mojo requires that strings are utf-8. +yzshen to confirm

We don't enforce it during validation at the moment. At the blink side, we
convert to WTF::String using fromUTF8() which will be null if the string is not
valid UTF8.

You and I chatted about whether to enforce it a while ago. But I haven't got
around to measure the performance impact. :/

[mark a. foltz, 2017-02-28 18:35:21]

Thanks for pointing out the extra copies.  I updated the typemap to note that
PCM is move-only, and that allowed elimination of most of them.

The remainder will happen because of:
- Conversion of WebString
- Passing string to Java
- Copying to RouteMessage, which I plan to eliminate in a followup change.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/a...
File chrome/browser/media/android/router/media_router_android.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/a...
chrome/browser/media/android/router/media_router_android.cc:178:
base::android::ConvertUTF8ToJavaString(env, message);
On 2017/02/28 at 06:45:26, dcheng wrote:
> There's probably not a lot of gain to using move semantics here: we can't use
move semantics to move ownership of the data into Java. In addition, every layer
that passes by value has an additional bit of code to run the destructor, which
makes the binary a bit bigger.

Acknowledged.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:174: message.text =
text;
On 2017/02/28 at 06:45:26, dcheng wrote:
> Either use std::move() or pass by const ref. Given the Android limitations, I
suspect using const ref may be better, but I'm not an expert in this code.
> 
> (Ditto below)

Done.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/browser_presentation_connection_proxy.cc
(right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/browser_presentation_connection_proxy.cc:35:
router_->SendRouteBinaryMessage(route_id_, message.data.value(),
On 2017/02/28 at 06:45:26, dcheng wrote:
> This has to copy anyway, since |message| is const. So const ref is better
here.

Now use std::move() since message is now non-const.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:339:
base::Unretained(this), route_id, std::move(message),
On 2017/02/28 at 06:45:26, dcheng wrote:
> Ah... I guess this is where move semantics might be useful. However, this
should be using base::Passed(). std::move() transfers ownership into the
callback, but the callback won't transfer callback to the invoked function, so
it ends up copying there.

Done.

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_service_delegate_impl.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/chrome/browser/media/r...
chrome/browser/media/router/presentation_service_delegate_impl.cc:958:
router_->SendRouteBinaryMessage(route_id, std::move(message.data.value()),
On 2017/02/28 at 06:45:26, dcheng wrote:
> Note that this doesn't move, since |message| is a const ref.

message is now non-const

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
File content/common/presentation/presentation_struct_traits.cc (right):

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:51: //    out->message
= std::string();
On 2017/02/28 at 06:45:27, dcheng wrote:
> Nit: remove commented out code here and below

Done.

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
File content/common/presentation/presentation_struct_traits.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/common/present...
content/common/presentation/presentation_struct_traits.h:16: //#include
"mojo/common/common_custom_types_struct_traits.h"
On 2017/02/28 at 06:45:27, dcheng wrote:
> Nit: remove commented out code

Done

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
File content/public/common/presentation_connection_message.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
content/public/common/presentation_connection_message.h:46: };
On 2017/02/28 at 06:45:27, dcheng wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is intended to
be move-only

The compiler automatically deletes the default copy/assign since it has a rvalue
reference ctor.

[dcheng, 2017-03-01 00:55:50]

mojo lgtm

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
File content/public/common/presentation_connection_message.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
content/public/common/presentation_connection_message.h:46: };
On 2017/02/28 18:35:21, mark a. foltz wrote:
> On 2017/02/28 at 06:45:27, dcheng wrote:
> > Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is intended to
> be move-only
> 
> The compiler automatically deletes the default copy/assign since it has a
rvalue
> reference ctor.

I think I would still prefer to be explicit about it.

https://codereview.chromium.org/2706463002/diff/180001/chrome/browser/media/r...
File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right):

https://codereview.chromium.org/2706463002/diff/180001/chrome/browser/media/r...
chrome/browser/media/router/mojo/media_router_mojo_impl.cc:340:
base::Passed(std::move(message)), callback));
base::Passed(&message) here and below

https://codereview.chromium.org/2706463002/diff/180001/content/common/present...
File content/common/presentation/presentation_struct_traits.cc (right):

https://codereview.chromium.org/2706463002/diff/180001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:21:
!base::IsStringASCII(out->presentation_id) ||
It seems pretty unusual to do this: no other struct traits does this. Out of
curiosity, why is it important for the presentation code to do this?

[hubbe, 2017-03-01 01:05:57]

https://codereview.chromium.org/2706463002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/cast_remoting_connector_unittest.cc (right):

https://codereview.chromium.org/2706463002/diff/180001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:168: std::string text,
I don't get why we're doing this.
We're just moving where the copy happens to a less obvious place.

https://codereview.chromium.org/2706463002/diff/180001/chrome/browser/media/c...
chrome/browser/media/cast_remoting_connector_unittest.cc:179:
std::vector<uint8_t> data,
This incurs a copy, why?

[mark a. foltz, 2017-03-02 22:01:56]

Description was changed from

==========
Converts presentation.mojom to use a typemap for
content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. MediaRouter no longer takes a unique_ptr for SendBinaryMessage().

5. A lot of tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

to

==========
Converts presentation.mojom to use a typemap for
content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. Tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

[mark a. foltz, 2017-03-02 22:03:54]

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

[commit-bot: I haz the power, 2017-03-02 22:04:29]

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-03-02 22:07:31]

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

[commit-bot: I haz the power, 2017-03-02 22:07:32]

Dry run: Try jobs failed on following builders:
  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...)

[mark a. foltz, 2017-03-02 22:14:32]

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

[commit-bot: I haz the power, 2017-03-02 22:15:12]

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-03-02 22:27:31]

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

[commit-bot: I haz the power, 2017-03-02 22:27:32]

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

[mark a. foltz, 2017-03-03 19:18:14]

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

[commit-bot: I haz the power, 2017-03-03 19:18:39]

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-03-03 20:58:51]

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

[commit-bot: I haz the power, 2017-03-03 20:58:53]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2017-03-03 21:03:21]

I reverted the Media Router changes.  They were tangential to introducing the
typemap and seemed to be confusing for folks to review.  Also, the APIs that
were changed are planned to be removed anyway.

There weren't any substantial changes to the remaining files, other than
presentation_service_delegate_impl.cc to convert back to the existing Media
Router API, and to rebase.  I'll wait a business day in case folks want to look
again.

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
File content/public/common/presentation_connection_message.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
content/public/common/presentation_connection_message.h:46: };
On 2017/03/01 at 00:55:49, dcheng wrote:
> On 2017/02/28 18:35:21, mark a. foltz wrote:
> > On 2017/02/28 at 06:45:27, dcheng wrote:
> > > Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is intended
to
> > be move-only
> > 
> > The compiler automatically deletes the default copy/assign since it has a
rvalue
> > reference ctor.
> 
> I think I would still prefer to be explicit about it.

The compiler actually won't allow use of DISALLOW_COPY_AND_ASSIGN if it has
already deleted them, because you'll be overriding a deleted ctor with a private
one.

https://codereview.chromium.org/2706463002/diff/180001/content/common/present...
File content/common/presentation/presentation_struct_traits.cc (right):

https://codereview.chromium.org/2706463002/diff/180001/content/common/present...
content/common/presentation/presentation_struct_traits.cc:21:
!base::IsStringASCII(out->presentation_id) ||
On 2017/03/01 at 00:55:50, dcheng wrote:
> It seems pretty unusual to do this: no other struct traits does this. Out of
curiosity, why is it important for the presentation code to do this?

To avoid invalid ids from a compromised renderer process?  This is a
Chrome-specific restriction so it could be relaxed to valid UTF-8 to be more
flexible.

[mark a. foltz, 2017-03-06 20:58:59]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2017-03-06 20:58:59]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
imcheng@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2706463002/#ps280001
(title: "Fix compile error after rebase")

[commit-bot: I haz the power, 2017-03-06 20:59:34]

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

[dcheng, 2017-03-06 22:14:55]

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
File content/public/common/presentation_connection_message.h (right):

https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
content/public/common/presentation_connection_message.h:46: };
On 2017/03/03 21:03:21, mark a. foltz wrote:
> On 2017/03/01 at 00:55:49, dcheng wrote:
> > On 2017/02/28 18:35:21, mark a. foltz wrote:
> > > On 2017/02/28 at 06:45:27, dcheng wrote:
> > > > Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is
intended
> to
> > > be move-only
> > > 
> > > The compiler automatically deletes the default copy/assign since it has a
> rvalue
> > > reference ctor.
> > 
> > I think I would still prefer to be explicit about it.
> 
> The compiler actually won't allow use of DISALLOW_COPY_AND_ASSIGN if it has
> already deleted them, because you'll be overriding a deleted ctor with a
private
> one.

This shouldn't block the commit but:

$ cat test.cc
class A {
 public:
  A(A&&) = default;

 private:
  A(const A&);
};
$ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11 -c test.cc
$

So I think it does work?

[mark a. foltz, 2017-03-06 22:17:45]

On 2017/03/06 at 22:14:55, dcheng wrote:
>
https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
> File content/public/common/presentation_connection_message.h (right):
> 
>
https://codereview.chromium.org/2706463002/diff/160001/content/public/common/...
> content/public/common/presentation_connection_message.h:46: };
> On 2017/03/03 21:03:21, mark a. foltz wrote:
> > On 2017/03/01 at 00:55:49, dcheng wrote:
> > > On 2017/02/28 18:35:21, mark a. foltz wrote:
> > > > On 2017/02/28 at 06:45:27, dcheng wrote:
> > > > > Nit: DISALLOW_COPY_AND_ASSIGN, since it seems like this class is
intended
> > to
> > > > be move-only
> > > > 
> > > > The compiler automatically deletes the default copy/assign since it has
a
> > rvalue
> > > > reference ctor.
> > > 
> > > I think I would still prefer to be explicit about it.
> > 
> > The compiler actually won't allow use of DISALLOW_COPY_AND_ASSIGN if it has
> > already deleted them, because you'll be overriding a deleted ctor with a
private
> > one.
> 
> This shouldn't block the commit but:
> 
> $ cat test.cc
> class A {
>  public:
>   A(A&&) = default;
> 
>  private:
>   A(const A&);
> };
> $ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11 -c test.cc
> $
> 
> So I think it does work?

I got a compiler error... I can give it a shot in a followup patch.

[commit-bot: I haz the power, 2017-03-07 01:50:55]

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

[commit-bot: I haz the power, 2017-03-07 01:50:57]

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, 2017-03-07 15:44:07]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:45:05]

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-03-07 18:24:24]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1488901447848040,
"parent_rev": "ff5ba024e4149f83c45711c070c2473a417b7963", "commit_rev":
"0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a"}

[commit-bot: I haz the power, 2017-03-07 18:25:07]

Description was changed from

==========
Converts presentation.mojom to use a typemap for
content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. Tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623
==========

to

==========
Converts presentation.mojom to use a typemap for
content::PresentationConnectionMessage.

This triggered a number of knock-on refactorings:

1. PresentationConnectionMessage is now a move-only type to enforce no
extraneous copies as it is passed in the browser.

2. presentation_constants.h is folded into presentation_connection_message.h as
it defines a constant only for PCM.

3. The mojo message is now a union and the type field in PCM is removed.  It can
be derived from which Optional field is set.

4. Tests are refactored to remove needless proxy/trampoline methods.

There are still more tangentially related cleanups to do:

- RouteMessage is not necessary and can be replaced with PCM.
- Unit tests use message loops in needlessly complicated ways.
- The message size limit needs to be more flexible and enforced downstream as
Cast and non-Cast sinks have different limits.





BUG=632623

Review-Url: https://codereview.chromium.org/2706463002
Cr-Commit-Position: refs/heads/master@{#455143}
Committed:
https://chromium.googlesource.com/chromium/src/+/0dc2a6bdf85013def2fcbb1b157e...
==========

[commit-bot: I haz the power, 2017-03-07 18:25:09]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/0dc2a6bdf85013def2fcbb1b157e...