Add PPAPI_BEGIN_MESSAGE_MAP and PPAPI_END_MESSAGE_MAP to be used when dispatching IPCs using PPAPI_…

Add PPAPI_BEGIN_MESSAGE_MAP and PPAPI_END_MESSAGE_MAP to be used when dispatching IPCs using PPAPI_DISPATCH_*. This is because the IPC_BEGIN_MESSAGE_MAP macros are closely tied to IPC_MESSAGE_HANDLERS.

This is split off from https://codereview.chromium.org/283623002/

BUG=304341
R=brettw@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270218

[Tom Sepez, 2014-05-13 20:03:46]

LGTM otherwise. Thanks.

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_renderer_connection.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_renderer_connection.cc:141:
IPC_BEGIN_MESSAGE_MAP_EX(PepperRendererConnection, msg, *message_was_ok)
Did you mean to convert this to PPAPI_BEGIN_MESSAGE_MAP as well?

https://codereview.chromium.org/281803003/diff/1/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_browser_connection.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_browser_connection.cc:33:
IPC_MESSAGE_HANDLER(PpapiHostMsg_CreateResourceHostsFromHostReply,
ditto

https://codereview.chromium.org/281803003/diff/1/content/renderer/pepper/ppb_...
File content/renderer/pepper/ppb_broker_impl.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/renderer/pepper/ppb_...
content/renderer/pepper/ppb_broker_impl.cc:127:
IPC_MESSAGE_HANDLER(ViewMsg_PpapiBrokerChannelCreated,
ditto

[jam, 2014-05-13 20:19:39]

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
On 2014/05/13 20:03:47, Tom Sepez wrote:
> Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?

these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't change them. i
just fixed the indentation to match our convention

[jam, 2014-05-13 20:54:56]

+dmichael for ppapi

[brettw, 2014-05-13 21:55:33]

pappi lgtm

https://codereview.chromium.org/281803003/diff/1/ppapi/proxy/dispatch_reply_m...
File ppapi/proxy/dispatch_reply_message.h (right):

https://codereview.chromium.org/281803003/diff/1/ppapi/proxy/dispatch_reply_m...
ppapi/proxy/dispatch_reply_message.h:132: #define
PPAPI_BEGIN_MESSAGE_MAP(class_name, msg) \
Can you add a comment about why we need a different one?

[jam, 2014-05-13 21:59:56]

https://codereview.chromium.org/281803003/diff/1/ppapi/proxy/dispatch_reply_m...
File ppapi/proxy/dispatch_reply_message.h (right):

https://codereview.chromium.org/281803003/diff/1/ppapi/proxy/dispatch_reply_m...
ppapi/proxy/dispatch_reply_message.h:132: #define
PPAPI_BEGIN_MESSAGE_MAP(class_name, msg) \
On 2014/05/13 21:55:33, brettw wrote:
> Can you add a comment about why we need a different one?

Done.

[dmichael (off chromium), 2014-05-14 15:40:06]

lgtm too, though I don't see the comment brettw requested uploaded yet.

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):

https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
On 2014/05/13 20:19:39, jam wrote:
> On 2014/05/13 20:03:47, Tom Sepez wrote:
> > Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?
> 
> these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't change them.
i
> just fixed the indentation to match our convention
Note, this is one of clang-format's idiosyncrasies. I haven't turned on
presubmit checks to enforce it yet, because I'm not done converting all of
Pepper over. It's easy enough for me to change back, so I don't mind either way.
I care little enough about the difference here that in the long run I was
planning to accept the slightly less nice way to get the benefit of
clang-format. I can't think of any good way to make clang-format understand
this, unless we teach it about our IPC macros.

[jam, 2014-05-14 16:28:13]

On 2014/05/14 15:40:06, dmichael wrote:
> lgtm too, though I don't see the comment brettw requested uploaded yet.

I didn't upload a new version with that change, just committed it.

> 
>
https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):
> 
>
https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
> IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
> On 2014/05/13 20:19:39, jam wrote:
> > On 2014/05/13 20:03:47, Tom Sepez wrote:
> > > Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?
> > 
> > these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't change
them.
> i
> > just fixed the indentation to match our convention
> Note, this is one of clang-format's idiosyncrasies. I haven't turned on
> presubmit checks to enforce it yet, because I'm not done converting all of
> Pepper over. It's easy enough for me to change back, so I don't mind either
way.
> I care little enough about the difference here that in the long run I was
> planning to accept the slightly less nice way to get the benefit of
> clang-format. I can't think of any good way to make clang-format understand
> this, unless we teach it about our IPC macros.

sigh. others might call this idiosyncrasies, but i call this formatting bugs.
when a subset of the codebase chooses to break a convention that has been
observed in the rest of the codebase for ~7 years, that makes things worse not
better.

[brettw, 2014-05-14 16:43:19]

On 2014/05/14 16:28:13, jam wrote:
> On 2014/05/14 15:40:06, dmichael wrote:
> > lgtm too, though I don't see the comment brettw requested uploaded yet.
> 
> I didn't upload a new version with that change, just committed it.
> 
> > 
> >
>
https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
> > File content/browser/renderer_host/pepper/pepper_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/281803003/diff/1/content/browser/renderer_hos...
> > content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
> > IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
> > On 2014/05/13 20:19:39, jam wrote:
> > > On 2014/05/13 20:03:47, Tom Sepez wrote:
> > > > Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?
> > > 
> > > these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't change
> them.
> > i
> > > just fixed the indentation to match our convention
> > Note, this is one of clang-format's idiosyncrasies. I haven't turned on
> > presubmit checks to enforce it yet, because I'm not done converting all of
> > Pepper over. It's easy enough for me to change back, so I don't mind either
> way.
> > I care little enough about the difference here that in the long run I was
> > planning to accept the slightly less nice way to get the benefit of
> > clang-format. I can't think of any good way to make clang-format understand
> > this, unless we teach it about our IPC macros.
> 
> sigh. others might call this idiosyncrasies, but i call this formatting bugs.
> when a subset of the codebase chooses to break a convention that has been
> observed in the rest of the codebase for ~7 years, that makes things worse not
> better.

I agree with this.

[dmichael (off chromium), 2014-05-14 17:11:52]

On Wed, May 14, 2014 at 10:43 AM, <brettw@chromium.org> wrote:

> On 2014/05/14 16:28:13, jam wrote:
>
>> On 2014/05/14 15:40:06, dmichael wrote:
>> > lgtm too, though I don't see the comment brettw requested uploaded yet.
>>
>
>  I didn't upload a new version with that change, just committed it.
>>
> FWIW I really think we should try to upload the final change prior to
committing. It makes life easier for sheriffs if something breaks and in
general makes doing software archaeology a lot easier.


>
>  >
>> >
>>
>
> https://codereview.chromium.org/281803003/diff/1/content/
> browser/renderer_host/pepper/pepper_message_filter.cc
>
>> > File content/browser/renderer_host/pepper/pepper_message_filter.cc
>> (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/281803003/diff/1/content/
> browser/renderer_host/pepper/pepper_message_filter.cc#newcode22
>
>> > content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
>> > IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
>> > On 2014/05/13 20:19:39, jam wrote:
>> > > On 2014/05/13 20:03:47, Tom Sepez wrote:
>> > > > Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?
>> > >
>> > > these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't
>> change
>> them.
>> > i
>> > > just fixed the indentation to match our convention
>> > Note, this is one of clang-format's idiosyncrasies. I haven't turned on
>> > presubmit checks to enforce it yet, because I'm not done converting all
>> of
>> > Pepper over. It's easy enough for me to change back, so I don't mind
>> either
>> way.
>> > I care little enough about the difference here that in the long run I
>> was
>> > planning to accept the slightly less nice way to get the benefit of
>> > clang-format. I can't think of any good way to make clang-format
>> understand
>> > this, unless we teach it about our IPC macros.
>>
>
>  sigh. others might call this idiosyncrasies, but i call this formatting
>> bugs.
>> when a subset of the codebase chooses to break a convention that has been
>> observed in the rest of the codebase for ~7 years, that makes things
>> worse not
>> better.
>>
>
> I agree with this.

I agree with you both that:
1) The old style is slightly more readable.
2) It's bad to have inconsistent style for this.

But I personally don't think it's worth wringing our hands about it. The
style is readable enough either way IMO. I look forward to not having to
worry about style while writing or reviewing, and I'm personally willing to
accept some minor differences vs established style. I can get used to
reading any reasonable style much more easily than I can remember to write
or enforce a particular style.

In any case, I did file a bug for this particular issue and CCed you both:
https://code.google.com/p/chromium/issues/detail?id=373340
...we'll see if there's a reasonable approach to make the tool match
established style.


>
> https://codereview.chromium.org/281803003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-05-14 17:22:22]

On 2014/05/14 17:11:52, dmichael wrote:
> On Wed, May 14, 2014 at 10:43 AM, <mailto:brettw@chromium.org> wrote:
> 
> > On 2014/05/14 16:28:13, jam wrote:
> >
> >> On 2014/05/14 15:40:06, dmichael wrote:
> >> > lgtm too, though I don't see the comment brettw requested uploaded yet.
> >>
> >
> >  I didn't upload a new version with that change, just committed it.
> >>
> > FWIW I really think we should try to upload the final change prior to
> committing. It makes life easier for sheriffs if something breaks and in
> general makes doing software archaeology a lot easier.
> 
> 
> >
> >  >
> >> >
> >>
> >
> > https://codereview.chromium.org/281803003/diff/1/content/
> > browser/renderer_host/pepper/pepper_message_filter.cc
> >
> >> > File content/browser/renderer_host/pepper/pepper_message_filter.cc
> >> (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/281803003/diff/1/content/
> > browser/renderer_host/pepper/pepper_message_filter.cc#newcode22
> >
> >> > content/browser/renderer_host/pepper/pepper_message_filter.cc:22:
> >> > IPC_BEGIN_MESSAGE_MAP_EX(PepperMessageFilter, msg, *message_was_ok)
> >> > On 2014/05/13 20:19:39, jam wrote:
> >> > > On 2014/05/13 20:03:47, Tom Sepez wrote:
> >> > > > Did you mean to convert these to PPAI_BEGIN_MESSAGE_MAP as well?
> >> > >
> >> > > these ones still use IPC_MESSAGE_HANDLER, so that's why i didn't
> >> change
> >> them.
> >> > i
> >> > > just fixed the indentation to match our convention
> >> > Note, this is one of clang-format's idiosyncrasies. I haven't turned on
> >> > presubmit checks to enforce it yet, because I'm not done converting all
> >> of
> >> > Pepper over. It's easy enough for me to change back, so I don't mind
> >> either
> >> way.
> >> > I care little enough about the difference here that in the long run I
> >> was
> >> > planning to accept the slightly less nice way to get the benefit of
> >> > clang-format. I can't think of any good way to make clang-format
> >> understand
> >> > this, unless we teach it about our IPC macros.
> >>
> >
> >  sigh. others might call this idiosyncrasies, but i call this formatting
> >> bugs.
> >> when a subset of the codebase chooses to break a convention that has been
> >> observed in the rest of the codebase for ~7 years, that makes things
> >> worse not
> >> better.
> >>
> >
> > I agree with this.
> 
> I agree with you both that:
> 1) The old style is slightly more readable.
> 2) It's bad to have inconsistent style for this.
> 
> But I personally don't think it's worth wringing our hands about it. The
> style is readable enough either way IMO. I look forward to not having to
> worry about style while writing or reviewing, and I'm personally willing to
> accept some minor differences vs established style. I can get used to
> reading any reasonable style much more easily than I can remember to write
> or enforce a particular style.
> 
> In any case, I did file a bug for this particular issue and CCed you both:
> https://code.google.com/p/chromium/issues/detail?id=373340
> ...we'll see if there's a reasonable approach to make the tool match
> established style.

Please don't enable this formatting plugin by default in any directory where it
breaks the convention from chrome. if there's a bug in the tool, then find a way
to fix the tool that you want to format the code, before unilaterally changing
the convention in subset of the code.