mandoline: Add automatic tracing at mojo call sites.

mandoline: Add automatic tracing at mojo call sites.

This modifies the mojo bindings generator to wrap all calls to
implementation of mojo interfaces in TRACE_EVENTs. It also makes various
mandoline pieces connect to mojo:tracing and names threads so the traces
are nicer.

With this patch, --trace-startup gives useful traces now. Further work
should be done to manually annotate callback methods with TRACE_EVENTS,
but traces now have useful data.

BUG=534895
R=ben@chromium.org,yzshen@chromium.org
TBR=tsepez@chromium.org

Committed: https://crrev.com/1fecf1452526cf274927645acc06c8a6d10ee99b
Cr-Commit-Position: refs/heads/master@{#355901}

[Elliot Glaysher, 2015-10-21 22:06:06]

Description was changed from

==========
mandoline: Add automatic tracing at mojo call sites.

This modifies the mojo bindings generator to wrap all calls to
implementation of mojo interfaces in TRACE_EVENTs. It also makes various
mandoline pieces connect to mojo:tracing and names threads so the traces
are nicer.

BUG=534895
==========

to

==========
mandoline: Add automatic tracing at mojo call sites.

This modifies the mojo bindings generator to wrap all calls to
implementation of mojo interfaces in TRACE_EVENTs. It also makes various
mandoline pieces connect to mojo:tracing and names threads so the traces
are nicer.

With this patch, --trace-startup gives useful traces now. Further work
should be done to manually annotate callback methods with TRACE_EVENTS,
but traces now have useful data.

BUG=534895
==========

[Elliot Glaysher, 2015-10-21 22:06:06]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[yzshen1, 2015-10-21 23:23:54]

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

[yzshen1, 2015-10-21 23:23:54]

https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
File
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
(right):

https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:37:
#ifdef PostMessage
Is it because some mojom files define PostMessage?

I feel that this should be handled outside of mojom generator: if some user
decides to use a name conflicting with OS definitions, it is his/her
responsibility to undef things properly when he/she tries to include both this
generated file and <windows.h>

WDYT?

[sky, 2015-10-21 23:27:42]

On 2015/10/21 23:23:54, yzshen1 wrote:
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
> File
>
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
> (right):
> 
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
>
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:37:
> #ifdef PostMessage
> Is it because some mojom files define PostMessage?
> 
> I feel that this should be handled outside of mojom generator: if some user
> decides to use a name conflicting with OS definitions, it is his/her
> responsibility to undef things properly when he/she tries to include both this
> generated file and <windows.h>
> 
> WDYT?

I agree. For the record I tried playing whack-a-mole with a mojom that had
CreateWindow() in it. It worked for a bit, but then failed at some point. Better
to go with different names.

[Elliot Glaysher, 2015-10-22 19:07:38]

erg@chromium.org changed reviewers:
+ mek@chromium.org

[Elliot Glaysher, 2015-10-22 19:07:38]

sky, yzshen: ptal.

mek: I've removed the name "PostMessage" from some mojom files since it
conflicts with windows.h.

[Marijn Kruisselbrink, 2015-10-22 19:28:13]

On 2015/10/21 at 23:23:54, yzshen wrote:
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
> File
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
(right):
> 
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
>
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:37:
#ifdef PostMessage
> Is it because some mojom files define PostMessage?
> 
> I feel that this should be handled outside of mojom generator: if some user
decides to use a name conflicting with OS definitions, it is his/her
responsibility to undef things properly when he/she tries to include both this
generated file and <windows.h>
Of course after this change that's no longer possible, since now the mojom
generated file includes windows.h (through base/trace_event/trace_event.h). But
the current state with where windows.h is and isn't included in chromium is
already pretty terrible, so just not using any of the names windows.h messes up
is probably the best way forward indeed.

[yzshen1, 2015-10-22 19:56:05]

On 2015/10/22 19:28:13, Marijn Kruisselbrink wrote:
> On 2015/10/21 at 23:23:54, yzshen wrote:
> >
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
> > File
>
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
> (right):
> > 
> >
>
https://codereview.chromium.org/1409223004/diff/20002/third_party/mojo/src/mo...
> >
>
third_party/mojo/src/mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:37:
> #ifdef PostMessage
> > Is it because some mojom files define PostMessage?
> > 
> > I feel that this should be handled outside of mojom generator: if some user
> decides to use a name conflicting with OS definitions, it is his/her
> responsibility to undef things properly when he/she tries to include both this
> generated file and <windows.h>
> Of course after this change that's no longer possible, since now the mojom
> generated file includes windows.h (through base/trace_event/trace_event.h).
But
> the current state with where windows.h is and isn't included in chromium is
> already pretty terrible, so just not using any of the names windows.h messes
up
> is probably the best way forward indeed.

Ah, didn't realize that trace_event.h introduces <windows.h>.
This is a little unfortunate. :/
But not sure it worths the effort to remove this include.

I agree that it seems easier and cleaner to avoid conflicting names.

LGTM

[Elliot Glaysher, 2015-10-22 21:51:22]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-10-22 21:51:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409223004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409223004/120001

[commit-bot: I haz the power, 2015-10-22 21:59:55]

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

[commit-bot: I haz the power, 2015-10-22 21:59:56]

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

[Elliot Glaysher, 2015-10-22 22:06:53]

erg@chromium.org changed reviewers:
+ ben@chromium.org

[Elliot Glaysher, 2015-10-22 22:06:53]

sky->ben: toplevel owners stamp (there are so many directories.)

[Ben Goodger (Google), 2015-10-23 20:44:21]

lgtm

[Elliot Glaysher, 2015-10-23 20:47:42]

Description was changed from

==========
mandoline: Add automatic tracing at mojo call sites.

This modifies the mojo bindings generator to wrap all calls to
implementation of mojo interfaces in TRACE_EVENTs. It also makes various
mandoline pieces connect to mojo:tracing and names threads so the traces
are nicer.

With this patch, --trace-startup gives useful traces now. Further work
should be done to manually annotate callback methods with TRACE_EVENTS,
but traces now have useful data.

BUG=534895
==========

to

==========
mandoline: Add automatic tracing at mojo call sites.

This modifies the mojo bindings generator to wrap all calls to
implementation of mojo interfaces in TRACE_EVENTs. It also makes various
mandoline pieces connect to mojo:tracing and names threads so the traces
are nicer.

With this patch, --trace-startup gives useful traces now. Further work
should be done to manually annotate callback methods with TRACE_EVENTS,
but traces now have useful data.

BUG=534895
R=ben@chromium.org,yzshen@chromium.org
TBR=tsepez@chromium.org
==========

[Elliot Glaysher, 2015-10-23 20:47:42]

erg@chromium.org changed reviewers:
+ tsepez@chromium.org

[Elliot Glaysher, 2015-10-23 20:48:34]

+Tom to the TBR line because for some reason, Ben's top level approval isn't
being accepted there?

Tom: This just renames stuff to not conflict with windows.h.

[Elliot Glaysher, 2015-10-23 20:48:41]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-10-23 20:48:41]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1409223004/#ps140001
(title: "Safety rebase to ToT")

[commit-bot: I haz the power, 2015-10-23 20:49:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1409223004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1409223004/140001

[Tom Sepez, 2015-10-23 20:55:55]

Mojom LGTM.

[commit-bot: I haz the power, 2015-10-23 22:08:07]

Committed patchset #9 (id:140001)

[commit-bot: I haz the power, 2015-10-23 22:09:09]

Patchset 9 (id:??) landed as
https://crrev.com/1fecf1452526cf274927645acc06c8a6d10ee99b
Cr-Commit-Position: refs/heads/master@{#355901}