[Presentation API] Replaces type converters with typemaps

Replace type converters for presentation.mojom with typemaps.

Typemap for ConnectionMessage will come in a separate patch, which should finish the conversion for presentation.mojom, barring further refactorings.

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

[mark a. foltz, 2017-01-06 23:38:05]

Description was changed from

==========
[WIP] Presentation type traits

BUG=
==========

to

==========
[WIP] presentation.mojom typemaps

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

[mark a. foltz, 2017-01-24 00:33:07]

Description was changed from

==========
[WIP] presentation.mojom typemaps

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

to

==========
Replace type converters for presentation.mojom with typemaps.

Typemap for ConnectionMessage will come in a separate patch, which should finish
the conversion for presentation.mojom, barring further refactorings.

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

[mark a. foltz, 2017-01-24 00:33:07]

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

[mark a. foltz, 2017-01-24 00:34:35]

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

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

+nasko@ for content/common and all SECURITY_OWNERS related changes
+imcheng@, zhaobin@ for all of it

PTAL

[zhaobin, 2017-01-24 01:42:37]

lgtm

[mark a. foltz, 2017-01-24 17:18:35]

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

[commit-bot: I haz the power, 2017-01-24 17:18:53]

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

[commit-bot: I haz the power, 2017-01-24 17:26:48]

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

[commit-bot: I haz the power, 2017-01-24 17:26:49]

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

[mark a. foltz, 2017-01-24 17:58:34]

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

[commit-bot: I haz the power, 2017-01-24 17:59:02]

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

[commit-bot: I haz the power, 2017-01-24 19:30:53]

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

[commit-bot: I haz the power, 2017-01-24 19:30:53]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-01-24 23:31:40]

lgtm

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

https://codereview.chromium.org/2613153003/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:48: const
content::PresentationSessionInfo& expected_value = expected;
nit: |expected_value| seems unnecessary; can it be removed? I don't remember why
it was written this way though.

https://codereview.chromium.org/2613153003/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:511:
OnDefaultSessionStarted(SessionInfoEquals(ByRef(session_info))))
Is ByRef still needed?

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
File content/common/presentation/OWNERS (right):

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/OWNERS:8: 
nit: rm empty lines

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

https://codereview.chromium.org/2613153003/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:385:
blink::WebURL(session_info.presentation_url),
This one invokes the blink::WebURL constructor explicitly but not in the other
places. Maybe replace the removed TypeConverter with a helper function?

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

https://codereview.chromium.org/2613153003/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:149:
blink::mojom::PresentationConnectionPtr,
nit: add names for the parameters.

[nasko, 2017-01-25 17:58:16]

Just first set of comments, since I'll have to continue the review later today.

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

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/presentation_struct_traits.h:75: return
blink::mojom::PresentationConnectionState::CONNECTING;
This is different than the code before, which returned
blink::mojom::PresentationConnectionState::TERMINATED. Is this intentional?

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/presentation_struct_traits.h:135: // TODO(mfoltz):
Add some validations?
Do we expect this to be resolved in this CL with subsequent patch or a different
CL?

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

https://codereview.chromium.org/2613153003/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:149:
blink::mojom::PresentationConnectionPtr,
On 2017/01/24 23:31:40, imcheng wrote:
> nit: add names for the parameters.

I wouldn't call this a nit, according to Chromium style, parameters need names
in headers.

[mark a. foltz, 2017-01-27 22:41:08]

PTAL

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

https://codereview.chromium.org/2613153003/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:48: const
content::PresentationSessionInfo& expected_value = expected;
On 2017/01/24 at 23:31:40, imcheng wrote:
> nit: |expected_value| seems unnecessary; can it be removed? I don't remember
why it was written this way though.

It's needed to unpack the reference passed via ByRef().  Removed.

https://codereview.chromium.org/2613153003/diff/140001/content/browser/presen...
content/browser/presentation/presentation_service_impl_unittest.cc:511:
OnDefaultSessionStarted(SessionInfoEquals(ByRef(session_info))))
On 2017/01/24 at 23:31:40, imcheng wrote:
> Is ByRef still needed?

Shouldn't be.  Removed.

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
File content/common/presentation/OWNERS (right):

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/OWNERS:8: 
On 2017/01/24 at 23:31:40, imcheng wrote:
> nit: rm empty lines

Done.

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

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/presentation_struct_traits.h:75: return
blink::mojom::PresentationConnectionState::CONNECTING;
On 2017/01/25 at 17:58:16, nasko wrote:
> This is different than the code before, which returned
blink::mojom::PresentationConnectionState::TERMINATED. Is this intentional?

No, changed to TERMINATED to keep behavior consistent.

https://codereview.chromium.org/2613153003/diff/140001/content/common/present...
content/common/presentation/presentation_struct_traits.h:135: // TODO(mfoltz):
Add some validations?
On 2017/01/25 at 17:58:16, nasko wrote:
> Do we expect this to be resolved in this CL with subsequent patch or a
different CL?

Added validations for the error message and presentation_id fields.

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

https://codereview.chromium.org/2613153003/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:385:
blink::WebURL(session_info.presentation_url),
On 2017/01/24 at 23:31:40, imcheng wrote:
> This one invokes the blink::WebURL constructor explicitly but not in the other
places. Maybe replace the removed TypeConverter with a helper function?

WebURL has a GURL constructor, so this was not necessary.

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

https://codereview.chromium.org/2613153003/diff/140001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:149:
blink::mojom::PresentationConnectionPtr,
On 2017/01/25 at 17:58:16, nasko wrote:
> On 2017/01/24 23:31:40, imcheng wrote:
> > nit: add names for the parameters.
> 
> I wouldn't call this a nit, according to Chromium style, parameters need names
in headers.

They're unused and should be commented out per style guide.  Done.

[mark a. foltz, 2017-01-27 23:54:57]

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

[commit-bot: I haz the power, 2017-01-27 23:55:41]

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

[commit-bot: I haz the power, 2017-01-28 03:21:53]

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

[commit-bot: I haz the power, 2017-01-28 03:21:54]

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

[mark a. foltz, 2017-01-31 06:57:57]

ping nasko@

[nasko, 2017-01-31 15:28:19]

LGTM

[mark a. foltz, 2017-01-31 17:25:23]

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

[mark a. foltz, 2017-01-31 17:25:24]

+rockot@ for one-line change to
mojo/public/tools/bindings/chromium_bindings_configuration.gni

[Ken Rockot(use gerrit already), 2017-01-31 21:06:58]

lgtm

[mark a. foltz, 2017-01-31 21:59:08]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2017-01-31 21:59:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
zhaobin@chromium.org, imcheng@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2613153003/#ps220001
(title: "Rebase again")

[mark a. foltz, 2017-01-31 21:59:35]

The CQ bit was unchecked by mfoltz@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:59:45]

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

[mark a. foltz, 2017-01-31 22:01:18]

The CQ bit was checked by mfoltz@chromium.org

[commit-bot: I haz the power, 2017-01-31 22:02: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, 2017-02-01 00:00:03]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[whywhat, 2017-02-01 00:13:54]

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

[whywhat, 2017-02-01 00:13:55]

Zhiqiang, for some reason the MR Android tests are failing. Do you have any
ideas why?

[Zhiqiang Zhang (Slow), 2017-02-01 11:14:08]

On 2017/02/01 00:13:55, whywhat_slow_PST_till_Feb_6 wrote:
> Zhiqiang, for some reason the MR Android tests are failing. Do you have any
> ideas why?

Looking into it

[Zhiqiang Zhang (Slow), 2017-02-01 14:55:41]

mfoltz@, avayvod@: I found the test failure cause.

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

https://codereview.chromium.org/2613153003/diff/220001/content/common/present...
content/common/presentation/presentation_struct_traits.h:163:
out->presentation_id.length() >
The failure cause is here. I got the presentation_id like
"mr_8fd9e0ed-3a94-44c2-92e3-df52028c1298", exceeding |kMaxIdLength| which is 32.

I think the failure is not specific to Android (as Desktop browser tests need
manual execution, which is not run on the bots).

Presentation ids are generated in MediaRouterBase::CreatePresentationId().

[mark a. foltz, 2017-02-03 18:42:48]

On 2017/02/01 at 14:55:41, zqzhang wrote:
> mfoltz@, avayvod@: I found the test failure cause.
> 
>
https://codereview.chromium.org/2613153003/diff/220001/content/common/present...
> File content/common/presentation/presentation_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2613153003/diff/220001/content/common/present...
> content/common/presentation/presentation_struct_traits.h:163:
out->presentation_id.length() >
> The failure cause is here. I got the presentation_id like
"mr_8fd9e0ed-3a94-44c2-92e3-df52028c1298", exceeding |kMaxIdLength| which is 32.
> 
> I think the failure is not specific to Android (as Desktop browser tests need
manual execution, which is not run on the bots).
> 
> Presentation ids are generated in MediaRouterBase::CreatePresentationId().

@zqzhang, thank you very much for tracking this down.  I didn't think to re-run
the integration tests with the latest patchset.  My fault!

[mark a. foltz, 2017-02-03 18:43:04]

The CQ bit was checked by mfoltz@chromium.org

[mark a. foltz, 2017-02-03 18:43:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
imcheng@chromium.org, nasko@chromium.org, zhaobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2613153003/#ps240001
(title: "Extend presentation ID max length to 64.")

[commit-bot: I haz the power, 2017-02-03 18:43:43]

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-03 20:04:22]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486147384817150,
"parent_rev": "c03dcfbe13aecc48b0b4cd0a107a8d659bdda939", "commit_rev":
"dfbd7c3e439e77b2d685ba90e0a90178a164410c"}

[commit-bot: I haz the power, 2017-02-03 20:06:05]

Description was changed from

==========
Replace type converters for presentation.mojom with typemaps.

Typemap for ConnectionMessage will come in a separate patch, which should finish
the conversion for presentation.mojom, barring further refactorings.

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

to

==========
Replace type converters for presentation.mojom with typemaps.

Typemap for ConnectionMessage will come in a separate patch, which should finish
the conversion for presentation.mojom, barring further refactorings.

BUG=632623

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

[commit-bot: I haz the power, 2017-02-03 20:06:08]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/dfbd7c3e439e77b2d685ba90e0a9...