1153063 - trun on logging for debug in test_tcp_control_channel.js

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Junior [inactive]]

Per bug 1152522 comment 3, trun on logging for debug in test_tcp_control_channel.js

Comment 2

[Junior [inactive]]

Attachment: turn-on-log

turn on log in mochitest and test_tcp_control_channel.js

Comment 3

[Junior [inactive]]

Attachment: turn-on-log

add one more log

Comment 4

[Junior [inactive]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b3f367724c

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8590731 [details] [diff] [review]
turn-on-log

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

::: dom/presentation/provider/TCPPresentationServer.js
@@ +13,2 @@
>  function log(aMsg) {
>    dump("-*- TCPPresentationServer.js: " + aMsg + "\n");

that should be DEBUG && dump(...); so that it applies to all calls to log() no?

@@ +57,5 @@
>      let serverSocketPort = (aPort !== 0) ? aPort : -1;
>  
>      this._serverSocket = Cc["@mozilla.org/network/server-socket;1"]
>                           .createInstance(Ci.nsIServerSocket);
> + 

nit: trailing whitespace

@@ +59,5 @@
>      this._serverSocket = Cc["@mozilla.org/network/server-socket;1"]
>                           .createInstance(Ci.nsIServerSocket);
> + 
> +    if(!this._serverSocket) {
> +      DEBUG && log("TCPPresentationServer - create server socket fail.");

and then you would not need the DEBUG here

::: modules/libpref/init/all.js
@@ +4534,5 @@
>  pref("dom.beforeAfterKeyboardEvent.enabled", false);
>  
>  // Presentation API
>  pref("dom.presentation.enabled", false);
> +pref("dom.presentation.tcpServer.debug", false);

We usually name prefs with snake_case, note camelCase. So that I would rather like `dom.presentation.tcp_server.debug`

Comment 6

[Junior [inactive]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> Comment on attachment 8590731 [details] [diff] [review]
> turn-on-log
> 
> Review of attachment 8590731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/provider/TCPPresentationServer.js
> @@ +13,2 @@
> >  function log(aMsg) {
> >    dump("-*- TCPPresentationServer.js: " + aMsg + "\n");
> 
> that should be DEBUG && dump(...); so that it applies to all calls to log()
> no?
> 
> @@ +59,5 @@
> >      this._serverSocket = Cc["@mozilla.org/network/server-socket;1"]
> >                           .createInstance(Ci.nsIServerSocket);
> > + 
> > +    if(!this._serverSocket) {
> > +      DEBUG && log("TCPPresentationServer - create server socket fail.");
> 
> and then you would not need the DEBUG here
> 

If we change the position of |DEBUG|, |log| will evaluate its parameter and be time-consuming since lots of |JSON.stringify| like here:
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/provider/TCPPresentationServer.js#182

Is that OK?

Comment 7

[[:fabrice] Fabrice Desré]

Right, you just added a missing one. Let's only change the pref name then!

Comment 8

[Junior [inactive]]

Attachment: turn-on-log

Modify by review comments
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e59178340f2

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8592606 [details] [diff] [review]
turn-on-log

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

::: dom/presentation/provider/TCPPresentationServer.js
@@ +58,5 @@
>  
>      this._serverSocket = Cc["@mozilla.org/network/server-socket;1"]
>                           .createInstance(Ci.nsIServerSocket);
> +
> +    if(!this._serverSocket) {

nit: if (...)

Comment 10

[Junior [inactive]]

Attachment: turn-on-log

Modify nit, carry r+

Thanks Fabrice!

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05e29a28737a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05e29a28737a