962344 - Log messages in IPDL should contain from/to information; Log message should go to logcat on Gonk devices.

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ted Clancy [:tedders1]]

1) IPDL log messages currently include the current process ID, but it would be useful for them to also include the ID of the process that the IPDL message is going to or coming from.

2) IPDL log messages currently go to stderr, but stderr isn't viewable when debugging Gonk devices. The log messages should go to logcat.

Comment 1

[Ted Clancy [:tedders1]]

Adding Ben to the CC.

Comment 2

[Ted Clancy [:tedders1]]

Attachment: patch-logging-ipdl.diff

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8363330 [details] [diff] [review]
patch-logging-ipdl.diff

Review of attachment 8363330 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Ted, would you mind posting a sample of the relevant generated code as well? That makes reviewing much easier. Thanks!

Comment 4

[Ted Clancy [:tedders1]]

Attachment: PBrowser.h

Comment 5

[Ted Clancy [:tedders1]]

Attachment: PBrowserChild.cpp

Comment 6

[Ted Clancy [:tedders1]]

Attachment: PBlobParent.cpp

Comment 7

[Ted Clancy [:tedders1]]

Attachment: ipc_logcat_output.txt

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8363330 [details] [diff] [review]
patch-logging-ipdl.diff

Review of attachment 8363330 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Ted, this looks great! Thanks for working on this.

I have a few suggestion:

::: ipc/ipdl/ipdl/lower.py
@@ +1860,5 @@
>  
>      logger.addstmt(appendToMsg(ExprLiteral.String('[TODO])\\n')))
>  
> +    logger.addstmts([
> +        CppDirective('ifdef', 'MOZ_WIDGET_GONK'),

I believe this should be using the |ANDROID| define since this mechanism won't be used only on Gonk. However, I'm not 100% sure that we always define that for B2G. Let's check and get this switched over to |ANDROID| if we can.

@@ +1862,5 @@
>  
> +    logger.addstmts([
> +        CppDirective('ifdef', 'MOZ_WIDGET_GONK'),
> +        StmtExpr(ExprCall(
> +            ExprVar('__android_log_write'),

We should make sure that there is a proper include for this function at the top of each PFoo.h file:

  #ifdef ANDROID
  #include <android/log.h>
  #endif

@@ +1864,5 @@
> +        CppDirective('ifdef', 'MOZ_WIDGET_GONK'),
> +        StmtExpr(ExprCall(
> +            ExprVar('__android_log_write'),
> +            args=[ ExprVar('ANDROID_LOG_INFO'),
> +                   ExprLiteral.String('IPCMessage'),

Nit: Let's make this 'GeckoIPC', most other logs we create are called 'Gecko'.

Comment 9

[Ted Clancy [:tedders1]]

Attachment: patch-logging-ipdl.diff

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8369386 [details] [diff] [review]
patch-logging-ipdl.diff

Review of attachment 8369386 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks!

Comment 11

[Gregor Wagner [:gwagner]]

Great work Ted!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8cd50b4ce64b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cd50b4ce64b