[Presentation API] Convert presentation.mojom to new wrapper types.

[Presentation API] Convert presentation.mojom to new wrapper types.

This converts presentation.mojom to use the new (std::) Mojo wrapper types:
mojo::String => std::string
mojo::Array => std::vector

No behavior changes should ensue.

Optional values are represented as base::Optional<Foo> with the new wrapper
types requiring some additional changes.

This work will simplify further changes for crbug.com/627655.

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

[mark a. foltz, 2016-07-26 22:00:33]

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

[mark a. foltz, 2016-07-26 22:00:34]

dcheng@: PTAL at third_party/WebKit/public GYP/GN changes
imcheng@: PTAL at everything else

Note: The GYP build is failing for unrelated reasons at TOT, so I can't test the
GYP changes at the moment.

[dcheng, 2016-07-27 08:22:55]

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:264:
OnScreenAvailabilityUpdated(std::string(url), available))
Why explicitly copy here?

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:327:
expected_msgs[1]->data = std::vector<uint8_t>(binary_data);
Why do we need to explicitly copy here? Similar question elsewhere.

[mark a. foltz, 2016-07-27 18:33:53]

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:264:
OnScreenAvailabilityUpdated(std::string(url), available))
On 2016/07/27 at 08:22:54, dcheng wrote:
> Why explicitly copy here?

Fixed.

https://codereview.chromium.org/2181163003/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:327:
expected_msgs[1]->data = std::vector<uint8_t>(binary_data);
On 2016/07/27 at 08:22:54, dcheng wrote:
> Why do we need to explicitly copy here? Similar question elsewhere.

Fixed here and below.

[mark a. foltz, 2016-07-27 19:38:09]

FYI, gyp build succeeds and content_unittests pass with this patch.

[imcheng, 2016-07-27 20:59:38]

lgtm

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

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:74: // TODO(mfoltz):
Should these be CHECKs?
Probably not. We shouldn't make the browser crash due to invalid (untrusted)
input from the renderer.

By the way, it's possible for PresentationServiceImpl to pass in a nullptr
message to PresentationServiceDelegate, which doesn't appear to check for
nullptr resulting in segfault. We shouldn't pass in nullptr message to delegate.

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

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:49: bool
ArePresentationSessionMessagesEqual(
Do we still need this or can we use operator== directly?

[dcheng, 2016-07-28 01:48:44]

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

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:52: output->data =
std::vector<uint8_t>(std::move(*(input->data)));
I think this can just be std::move(*(input->data))

Also, I think I mentioned this earlier, but the conversion should really just be
taking place at a higher level: it's super awkward to plumb through the 'single
observer' bit, and the fact we need to do that means that our layering is
probably a little wrong.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:54: output->data =
std::vector<uint8_t>(*(input->data));
Ditto: I don't think we need to constructor the vector here?

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:78: if
(input->message->size() > content::kMaxPresentationSessionMessageSize)
We need to handle unset values here, this code is running in the browser process
and can't trust the data it receives. The same thing applies to all the other
DCHECKs that are being performed: now that we're using base::Optional to
represent these, these derefs become undefined behavior if the optional doesn't
contain a value.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:83:
input->message->swap(output->message);
Nit: std::move() instead here for consistency (same comment elsewhere)

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

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

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:52: output->data =
std::vector<uint8_t>(std::move(*(input->data)));
On 2016/07/28 at 01:48:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> I think this can just be std::move(*(input->data))

Done.
 
> Also, I think I mentioned this earlier, but the conversion should really just
be taking place at a higher level: it's super awkward to plumb through the
'single observer' bit, and the fact we need to do that means that our layering
is probably a little wrong.

Agree this is not ideal, will discuss with imcheng@ about how this came to be. 
But changing the layering of this API is out of scope for this patch.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:54: output->data =
std::vector<uint8_t>(*(input->data));
On 2016/07/28 at 01:48:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> Ditto: I don't think we need to constructor the vector here?

Done.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:74: // TODO(mfoltz):
Should these be CHECKs?
On 2016/07/27 at 20:59:38, imcheng wrote:
> Probably not. We shouldn't make the browser crash due to invalid (untrusted)
input from the renderer.

Now converted to return nullptr on invalid input.

> By the way, it's possible for PresentationServiceImpl to pass in a nullptr
message to PresentationServiceDelegate, which doesn't appear to check for
nullptr resulting in segfault. We shouldn't pass in nullptr message to delegate.

Fixed.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:78: if
(input->message->size() > content::kMaxPresentationSessionMessageSize)
On 2016/07/28 at 01:48:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> We need to handle unset values here, this code is running in the browser
process and can't trust the data it receives. The same thing applies to all the
other DCHECKs that are being performed: now that we're using base::Optional to
represent these, these derefs become undefined behavior if the optional doesn't
contain a value.

Done.

https://codereview.chromium.org/2181163003/diff/40001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:83:
input->message->swap(output->message);
On 2016/07/28 at 01:48:44, dcheng (OOO Aug 1 - Aug 11) wrote:
> Nit: std::move() instead here for consistency (same comment elsewhere)

Done.  No more swaps.

[dcheng, 2016-07-29 08:09:06]

LGTM with nits addressed.

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

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:110: *(output->data) =
std::move(input->data.value());
Nit: slightly more efficient to just call the move constructor directly when
resetting the unique_ptr:

output_data.reset(new std::vector<uint8_t>(std::move(input->data.value()));

Ditto on line 96

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:469:
std::transform(messages.begin(), messages.end(), mojoMessages.begin(),
Nit: this is existing code, but let's use mojo_messages to follow chromium
naming conventions.

Chrome also prefers for loops over functional style code

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:317:
expected_msgs[1]->data = std::vector<uint8_t>(binary_data);
Nit: just |binary_data| on the RHS should be sufficient.

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

https://codereview.chromium.org/2181163003/diff/60001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:421:
std::vector<blink::mojom::SessionMessagePtr> messages) {
FWIW, it's not clear to me why we don't just pass this by const ref.

[mark a. foltz, 2016-07-29 20:42:01]

Thanks for the thorough review.

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

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:110: *(output->data) =
std::move(input->data.value());
On 2016/07/29 at 08:09:06, dcheng (OOO Aug 1 - Aug 11) wrote:
> Nit: slightly more efficient to just call the move constructor directly when
resetting the unique_ptr:
> 
> output_data.reset(new std::vector<uint8_t>(std::move(input->data.value()));
> 
> Ditto on line 96

Done.

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:469:
std::transform(messages.begin(), messages.end(), mojoMessages.begin(),
On 2016/07/29 at 08:09:06, dcheng (OOO Aug 1 - Aug 11) wrote:
> Nit: this is existing code, but let's use mojo_messages to follow chromium
naming conventions.

Done.

> Chrome also prefers for loops over functional style code

I see both styles in use.  std::transform better expresses intent here IMO.

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
File content/browser/presentation/presentation_service_impl_unittest.cc (right):

https://codereview.chromium.org/2181163003/diff/60001/content/browser/present...
content/browser/presentation/presentation_service_impl_unittest.cc:317:
expected_msgs[1]->data = std::vector<uint8_t>(binary_data);
On 2016/07/29 at 08:09:06, dcheng (OOO Aug 1 - Aug 11) wrote:
> Nit: just |binary_data| on the RHS should be sufficient.

Done.

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

https://codereview.chromium.org/2181163003/diff/60001/content/renderer/presen...
content/renderer/presentation/presentation_dispatcher.cc:421:
std::vector<blink::mojom::SessionMessagePtr> messages) {
On 2016/07/29 at 08:09:06, dcheng (OOO Aug 1 - Aug 11) wrote:
> FWIW, it's not clear to me why we don't just pass this by const ref.

This is the method signature generated by mojo...

[mark a. foltz, 2016-07-29 20:42:18]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2016-07-29 20:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
imcheng@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2181163003/#ps80001
(title: "Respond to dcheng@ comments")

[commit-bot: I haz the power, 2016-07-29 20:42:46]

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

[commit-bot: I haz the power, 2016-07-29 22:11:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-29 22:12:53]

Description was changed from

==========
[Presentation API] Convert presentation.mojom to new wrapper types.

This converts presentation.mojom to use the new (std::) Mojo wrapper types:
mojo::String => std::string
mojo::Array => std::vector

No behavior changes should ensue.

Optional values are represented as base::Optional<Foo> with the new wrapper
types requiring some additional changes.

This work will simplify further changes for crbug.com/627655.

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

to

==========
[Presentation API] Convert presentation.mojom to new wrapper types.

This converts presentation.mojom to use the new (std::) Mojo wrapper types:
mojo::String => std::string
mojo::Array => std::vector

No behavior changes should ensue.

Optional values are represented as base::Optional<Foo> with the new wrapper
types requiring some additional changes.

This work will simplify further changes for crbug.com/627655.

BUG=627655

Committed: https://crrev.com/59e7b40948030815a49af887b922b370ba048b8b
Cr-Commit-Position: refs/heads/master@{#408781}
==========

[commit-bot: I haz the power, 2016-07-29 22:12:54]

Patchset 5 (id:??) landed as
https://crrev.com/59e7b40948030815a49af887b922b370ba048b8b
Cr-Commit-Position: refs/heads/master@{#408781}

[tapted, 2016-08-01 03:12:03]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2196143002/ by tapted@chromium.org.

The reason for reverting is: Suspect (not the first) for widespread NaCl test
failures since
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/bui...

Errors like

[4224:3200:0729/162247:ERROR:nacl_process_host.cc(337)] NaCl process exited with
status -1073741674 (0xc0000096)
...
  Actual: "Plugin crashed. 'NaCl module crashed'"

Wasn't r408782 or r408787. Running out of candidates...

[tapted, 2016-08-01 05:26:35]

Relanding now - the revert didn't help the NaCl problem. But, FYI, this CL may
be missing some build dependencies. Please look into the error here:

https://build.chromium.org/p/chromium.memory.full/builders/Chromium%20Windows...

FAILED: obj/content/browser/frame_host/content.render_frame_host_impl.obj 
ninja -t msvc -e environment.x86 --
"C:\b\depot_tools\win_toolchain\vs_files\95ddda401ec5678f15eeed01d2bee08fcbc5ee97\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC
@obj\content\browser\frame_host\content.render_frame_host_impl.obj.rsp /c
..\..\content\browser\frame_host\render_frame_host_impl.cc
/Foobj\content\browser\frame_host\content.render_frame_host_impl.obj
/Fdobj\content\content.cc.pdb 
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(152):error
C3668: 'content::PresentationServiceImpl::SetDefaultPresentationURL': method
with override specifier 'override' did not override any base class methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(154):error
C3668: 'content::PresentationServiceImpl::ListenForScreenAvailability': method
with override specifier 'override' did not override any base class methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(155):error
C3668: 'content::PresentationServiceImpl::StopListeningForScreenAvailability':
method with override specifier 'override' did not override any base class
methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(156):error
C3668: 'content::PresentationServiceImpl::StartSession': method with override
specifier 'override' did not override any base class methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(159):error
C3668: 'content::PresentationServiceImpl::JoinSession': method with override
specifier 'override' did not override any base class methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(166):error
C3668: 'content::PresentationServiceImpl::CloseConnection': method with override
specifier 'override' did not override any base class methods
c:\b\build\slave\drm-cr\build\src\content\browser\presentation\presentation_service_impl.h(168):error
C3668: 'content::PresentationServiceImpl::Terminate': method with override
specifier 'override' did not override any base class methods

This occurred while the CL was reverted, but that shouldn't have resulted in
flaky compile errors.