Convert ManifestManager IPCs to Mojo.

Convert ManifestManager IPCs to Mojo.

BUG=593607, 577685

[Sam McNally, 2016-04-22 05:35:04]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-04-22 10:45:04]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-04-22 10:45:15]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-04-22 10:47:06]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-04-26 03:30:16]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-04-26 03:30:46]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2016-04-26 08:18:14]

Patchset #2 (id:140001) has been deleted

[Sam McNally, 2016-04-26 08:21:13]

Patchset #1 (id:120001) has been deleted

[Sam McNally, 2016-04-26 08:22:40]

sammc@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-04-26 14:30:07]

There are some "geometry" changes. Did another change got merged here?

[Sam McNally, 2016-04-27 06:51:26]

On 2016/04/26 14:30:07, Mounir Lamouri wrote:
> There are some "geometry" changes. Did another change got merged here?

I'm not sure which changes you're referring to.

[mlamouri (slow - plz ping), 2016-04-27 13:38:09]

Sorry, I had a quick glance and I couldn't understand why geometry was added in
many places. I had a longer look today and it seems that Size is defined there.

lgtm

https://codereview.chromium.org/1913043002/diff/160001/content/browser/manife...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/1913043002/diff/160001/content/browser/manife...
content/browser/manifest/manifest_manager_host.cc:81: OnConnectionError();
OnConnectionError is for deleting? That sounds odd but I guess you are the mojo
specialist here :)

https://codereview.chromium.org/1913043002/diff/160001/content/common/manifes...
File content/common/manifest_type_converters.h (right):

https://codereview.chromium.org/1913043002/diff/160001/content/common/manifes...
content/common/manifest_type_converters.h:24: };
Should these be in a string_type_coverters.h? or be added to
common_type_converters.h?

https://codereview.chromium.org/1913043002/diff/160001/content/renderer/manif...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/1913043002/diff/160001/content/renderer/manif...
content/renderer/manifest/manifest_manager.cc:159: }
style: remove { }

https://codereview.chromium.org/1913043002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/manifest/manifest.mojom (right):

https://codereview.chromium.org/1913043002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/manifest/manifest.mojom:52: int64
background_color;
OOC, would int32? work?

[Sam McNally, 2016-04-28 08:14:02]

Patchset #2 (id:180001) has been deleted

[Sam McNally, 2016-04-28 08:25:26]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org, jam@chromium.org

[Sam McNally, 2016-04-28 08:25:28]

+esprehn for third_party/WebKit/public/

+dcheng for ipc/, the mojoms, content/common/content_message_generator.h and
content/common/manifest_manager_messages.h

+jam for content/ and mojo/

https://codereview.chromium.org/1913043002/diff/160001/content/browser/manife...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/1913043002/diff/160001/content/browser/manife...
content/browser/manifest/manifest_manager_host.cc:81: OnConnectionError();
On 2016/04/27 13:38:08, Mounir Lamouri wrote:
> OnConnectionError is for deleting? That sounds odd but I guess you are the
mojo
> specialist here :)

Renamed it to not be specific to connection errors.

https://codereview.chromium.org/1913043002/diff/160001/content/common/manifes...
File content/common/manifest_type_converters.h (right):

https://codereview.chromium.org/1913043002/diff/160001/content/common/manifes...
content/common/manifest_type_converters.h:24: };
On 2016/04/27 13:38:08, Mounir Lamouri wrote:
> Should these be in a string_type_coverters.h? or be added to
> common_type_converters.h?

Done.

https://codereview.chromium.org/1913043002/diff/160001/content/renderer/manif...
File content/renderer/manifest/manifest_manager.cc (right):

https://codereview.chromium.org/1913043002/diff/160001/content/renderer/manif...
content/renderer/manifest/manifest_manager.cc:159: }
On 2016/04/27 13:38:08, Mounir Lamouri wrote:
> style: remove { }

Done.

https://codereview.chromium.org/1913043002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/manifest/manifest.mojom (right):

https://codereview.chromium.org/1913043002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/manifest/manifest.mojom:52: int64
background_color;
On 2016/04/27 13:38:08, Mounir Lamouri wrote:
> OOC, would int32? work?

int32 + a bool would work.

[jam, 2016-04-28 15:47:32]

why are we adding more type converters instead of using struct traits? the
latter are more efficient and also nicer to read.

[yzshen1, 2016-04-28 15:54:40]

On 2016/04/28 15:47:32, jam wrote:
> why are we adding more type converters instead of using struct traits? the
> latter are more efficient and also nicer to read.

I think that is because typemapping support is not complete yet (such as mapping
nested struct, array, etc.)
Working on it right now...

[jam, 2016-04-28 16:03:39]

On 2016/04/28 15:54:40, yzshen1 wrote:
> On 2016/04/28 15:47:32, jam wrote:
> > why are we adding more type converters instead of using struct traits? the
> > latter are more efficient and also nicer to read.
> 
> I think that is because typemapping support is not complete yet (such as
mapping
> nested struct, array, etc.)
> Working on it right now...

can we just wait for it before converting this file then?

[pfeldman, 2016-04-28 23:22:13]

I'm looking at the manifest usage and it seems like we only use parsed Manifest
in browser/. The only usage in renderer is around proprietary gcm_sender_id. It
seems like it would be simpler to move the parser into the browser altogether
and parse the proprietary extension using ad-hoc parser in content/. That's
significantly simplify all the plumbing.

[pfeldman, 2016-04-29 00:04:01]

(or is that the base/values json parsing of user data in browser that is in the
way?)

[Sam McNally, 2016-04-29 03:26:28]

On 2016/04/28 16:03:39, jam wrote:
> On 2016/04/28 15:54:40, yzshen1 wrote:
> > On 2016/04/28 15:47:32, jam wrote:
> > > why are we adding more type converters instead of using struct traits? the
> > > latter are more efficient and also nicer to read.
> > 
> > I think that is because typemapping support is not complete yet (such as
> mapping
> > nested struct, array, etc.)
> > Working on it right now...
> 
> can we just wait for it before converting this file then?

We can, but I was planning to change the existing content::Manifest users, other
than the parser which will be moving into blink, to use the generated mojo
struct directly.

On 2016/04/29 00:04:01, pfeldman wrote:
> (or is that the base/values json parsing of user data in browser that is in
the
> way?)

I think so.

[mlamouri (slow - plz ping), 2016-04-29 10:40:47]

On 2016/04/28 at 23:22:13, pfeldman wrote:
> I'm looking at the manifest usage and it seems like we only use parsed
Manifest in browser/. The only usage in renderer is around proprietary
gcm_sender_id. It seems like it would be simpler to move the parser into the
browser altogether and parse the proprietary extension using ad-hoc parser in
content/. That's significantly simplify all the plumbing.

The parsing happens in the renderer by design. We don't want to parse things in
the browser process for security reasons.

[Sam McNally, 2016-05-16 05:01:33]

Patchset #6 (id:280001) has been deleted

[Sam McNally, 2016-05-16 05:45:41]

Patchset #6 (id:300001) has been deleted

[Sam McNally, 2016-05-16 05:45:43]

Patchset #6 (id:320001) has been deleted

[Sam McNally, 2016-05-16 05:45:46]

Patchset #6 (id:340001) has been deleted

[Sam McNally, 2016-05-16 05:48:23]

Patchset #6 (id:360001) has been deleted

[esprehn, 2016-05-17 22:37:44]

What's the status here?

[Sam McNally, 2016-05-18 00:29:13]

Patchset #6 (id:380001) has been deleted

[Sam McNally, 2016-05-18 00:48:50]

Patchset #7 (id:420001) has been deleted

[Sam McNally, 2016-05-18 05:34:38]

Patchset #10 (id:500001) has been deleted

[Sam McNally, 2016-05-18 05:37:50]

On 2016/05/17 22:37:44, esprehn wrote:
> What's the status here?

I've changed this to use typemaps instead of type converters so this is ready
for another look now.

[Sam McNally, 2016-05-18 06:51:52]

Patchset #11 (id:540001) has been deleted

[esprehn, 2016-06-01 05:11:22]

dcheng@ Can you take this? :)

[dcheng, 2016-06-01 23:38:42]

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
File content/public/common/manifest_struct_traits.cc (right):

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.cc:140: out->sizes =
sizes.To<std::vector<gfx::Size>>();
Do we need to manually do this because it's a mojo::Array?
Does this end up using the StructTraits for <mojo::SizePtr, gfx::Size>?

(Also, I notice that the StructTraits for gfx::Size is missing some checks, but
that should be fixed in a separate CL)

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.cc:148: return
TruncateString(m.platform);
Similar to the comments about color: why do we check in the ipc layer? Wouldn't
it be better if the code that sets this value didn't set overly long values so
we can just DCHECK() that here?

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
File content/public/common/manifest_struct_traits.h (right):

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.h:38: return
content::Manifest::kInvalidOrMissingColor;
Why do we need to do this check? From the comments, it looks like returning
theme_color as it is should be the same effect.

(Same comment for background_color)

[Sam McNally, 2016-06-02 05:22:14]

Description was changed from

==========
Convert ManifestManager IPCs to Mojo.

BUG=593607
==========

to

==========
Convert ManifestManager IPCs to Mojo.

BUG=593607,577685
==========

[Sam McNally, 2016-06-02 05:54:06]

Patchset #13 (id:600001) has been deleted

[Sam McNally, 2016-06-02 05:54:19]

Patchset #13 (id:620001) has been deleted

[Sam McNally, 2016-06-02 07:02:21]

Patchset #13 (id:640001) has been deleted

[Sam McNally, 2016-06-02 07:02:40]

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
File content/public/common/manifest_struct_traits.cc (right):

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.cc:140: out->sizes =
sizes.To<std::vector<gfx::Size>>();
On 2016/06/01 23:38:42, dcheng wrote:
> Do we need to manually do this because it's a mojo::Array?
> Does this end up using the StructTraits for <mojo::SizePtr, gfx::Size>?
> 
> (Also, I notice that the StructTraits for gfx::Size is missing some checks,
but
> that should be fixed in a separate CL)

This was from before mojo::Size was typemapped to gfx::Size.

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.cc:148: return
TruncateString(m.platform);
On 2016/06/01 23:38:42, dcheng wrote:
> Similar to the comments about color: why do we check in the ipc layer?
Wouldn't
> it be better if the code that sets this value didn't set overly long values so
> we can just DCHECK() that here?

Done.

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
File content/public/common/manifest_struct_traits.h (right):

https://codereview.chromium.org/1913043002/diff/560001/content/public/common/...
content/public/common/manifest_struct_traits.h:38: return
content::Manifest::kInvalidOrMissingColor;
On 2016/06/01 23:38:42, dcheng wrote:
> Why do we need to do this check? From the comments, it looks like returning
> theme_color as it is should be the same effect.
> 
> (Same comment for background_color)

Done.

[mlamouri (slow - plz ping), 2016-06-02 17:30:22]

BTW, FWIW, I'm planning to land a CL using Optional<> for color after this lands
(https://codereview.chromium.org/2036803002) so the int64 hack will no longer be
around. Not sure if that really matters for this change but in case of.

[dcheng, 2016-06-07 22:00:53]

https://codereview.chromium.org/1913043002/diff/660001/content/browser/manife...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/1913043002/diff/660001/content/browser/manife...
content/browser/manifest/manifest_manager_host.cc:18: : parent_(parent),
render_frame_host_(render_frame_host) {
Nit: I think parent_ is a bit of a confusing name here, it'd kind of easy to
interpret this as being related to a frame parent.

https://codereview.chromium.org/1913043002/diff/660001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/manifest/manifest.mojom (right):

https://codereview.chromium.org/1913043002/diff/660001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/manifest/manifest.mojom:18: enum
ScreenOrientationLockType {
These enums have to match things in (not obviously) related modules. I'm a bit
worried about the long-term maintainability of this: shouldn't these enums be
defined in screen_orientation, etc so it lives next to the C++ enum? Also,
what's the status of EnumTraits? =)

[Sam McNally, 2016-06-08 01:36:23]

Patchset #14 (id:680001) has been deleted

[Sam McNally, 2016-06-08 01:46:42]

Patchset #15 (id:720001) has been deleted

[Sam McNally, 2016-06-08 10:28:08]

https://codereview.chromium.org/1913043002/diff/660001/content/browser/manife...
File content/browser/manifest/manifest_manager_host.cc (right):

https://codereview.chromium.org/1913043002/diff/660001/content/browser/manife...
content/browser/manifest/manifest_manager_host.cc:18: : parent_(parent),
render_frame_host_(render_frame_host) {
On 2016/06/07 22:00:52, dcheng wrote:
> Nit: I think parent_ is a bit of a confusing name here, it'd kind of easy to
> interpret this as being related to a frame parent.

Done.

https://codereview.chromium.org/1913043002/diff/660001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/manifest/manifest.mojom (right):

https://codereview.chromium.org/1913043002/diff/660001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/manifest/manifest.mojom:18: enum
ScreenOrientationLockType {
On 2016/06/07 22:00:52, dcheng wrote:
> These enums have to match things in (not obviously) related modules. I'm a bit
> worried about the long-term maintainability of this: shouldn't these enums be
> defined in screen_orientation, etc so it lives next to the C++ enum? 
Done.

> Also,
> what's the status of EnumTraits? =)

I think Yuzhu's planning on working on it this or next week.

[dcheng, 2016-06-09 06:26:25]

LGTM with updated mojom OWNERS for //third_party/WebKit/public/platform and
//third_party/WebKit/public/platform/modules/screen_orientation

[johnme, 2016-07-04 16:30:52]

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

[johnme, 2016-07-04 16:30:55]

Drive-by nits.

https://codereview.chromium.org/1913043002/diff/820001/mojo/mojo_public.gyp
File mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1913043002/diff/820001/mojo/mojo_public.gyp#n...
mojo/mojo_public.gyp:230: 'public/cpp/bindings/string_traits_string16.h',
Nit: duplicate entry

https://codereview.chromium.org/1913043002/diff/820001/mojo/mojo_public.gyp#n...
mojo/mojo_public.gyp:231:
'public/cpp/bindings/string_traits_nullable_string16.h',
Nit: not in alphabetic order

[johnme, 2016-07-04 16:31:15]

johnme@chromium.org changed reviewers:
- johnme@chromium.org