1408371 - Update about:webrtc after bug 1407492

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Paul Adenot (:padenot)]

Update about:webrtc for bug 1407492. It's lying, at the minute, still using the old way of doing things, with the pref and all [0].

Really shouldn't have to strace or break in gdb to discover where the file are going to be.

[0]: http://searchfox.org/mozilla-central/source/toolkit/content/aboutwebrtc/aboutWebrtc.js#265

Comment 1

[Paul Adenot (:padenot)]

P2. We can't say we're investing in quality if we have broken tools to debug and assess problems.

Nils, do you or someone on your team have cycles to fix this one ?

Comment 2

[Nils Ohlmeier [:drno]]

The intention was to remove the aec log dir user pref as it doesn't work for example on OSX any more, because the sandbox won't let us open files where we want it.

I overlooked that pkerr used the user pref to get the value also onto the page.

Another case of missing tests hurting us.

As I broke it I'm looking into adding a getter for the temp dir instead of the pref.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Bug 1408371: report AEC log dir through getter.

Review commit: https://reviewboard.mozilla.org/r/189902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189902/

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

https://reviewboard.mozilla.org/r/189902/#review195136

r+ with one nit

::: toolkit/content/aboutwebrtc/aboutWebrtc.js:265
(Diff revision 1)
>  AecLogging.prototype.constructor = AecLogging;
>  
>  AecLogging.prototype.offState = function() {
>    this._label = getString("aec_logging_off_state_label");
>    try {
> -    let file = Services.prefs.getCharPref("media.webrtc.debug.aec_log_dir");
> +    //let file = Services.prefs.getCharPref("media.webrtc.debug.aec_log_dir");

Remove comment?

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189902/diff/1-2/

Comment 6

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 6 changes to 6 files
remote: 
remote: WebIDL file dom/webidl/WebrtcGlobalInformation.webidl altered in changeset 1fe39aaae9e3 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

https://reviewboard.mozilla.org/r/189902/#review195352

::: dom/webidl/WebrtcGlobalInformation.webidl:40
(Diff revision 2)
>    static attribute long debugLevel;
>  
>    // WebRTC AEC debugging enable
>    static attribute boolean aecDebug;
> +
> +  static readonly attribute DOMString? aecDebugLogDir;

Why DOMString? ? I don't see null value being returned. And if null is not returned, should be just DOMString.

With that answered, r+ to the .webidl change, given that the interface is ChromeOnly.

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp:261
(Diff revision 2)
>      sSink = nullptr;
>    }
>  }
>  
> -void ConfigAecLog() {
> +nsCString ConfigAecLog() {
> +  nsCString aAECLogDir;

oddly named variable. a-prefix is used for argument, but I see you just moved it

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp:263
(Diff revision 2)
>  }
>  
> -void ConfigAecLog() {
> +nsCString ConfigAecLog() {
> +  nsCString aAECLogDir;
>    if (webrtc::Trace::aec_debug()) {
> -    return;
> +    return aAECLogDir;

It might be more obvious to just return EmptyCString() here

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp:281
(Diff revision 2)
> +  return aAECLogDir;
>  }
>  
> -void StartAecLog()
> +nsCString StartAecLog()
>  {
> +  nsCString aAECLogDir;

Similar stuff here

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp:669
(Diff revision 2)
>    aRv = rv;
>  }
>  
>  static int32_t sLastSetLevel = 0;
>  static bool sLastAECDebug = false;
> +static nsCString sAecDebugLogDir;

Doesn't this add a new static constructor, which we aren't supposed to add.
(ask glandium)

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189902/diff/2-3/

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

https://reviewboard.mozilla.org/r/189902/#review195352

> Doesn't this add a new static constructor, which we aren't supposed to add.
> (ask glandium)

Yes I think you are right. I put in a little hack to ensure that the static is properly initialized when accessing it (assuming it's not the performance impact of static constructors which is the bad part here).

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189902/diff/3-4/

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8919011 [details]
Bug 1408371: report AEC log dir through getter.

https://reviewboard.mozilla.org/r/189902/#review195352

> Yes I think you are right. I put in a little hack to ensure that the static is properly initialized when accessing it (assuming it's not the performance impact of static constructors which is the bad part here).

So I chatted with glandium about this and he suggested using a Maybe<nsCString>, which I added in last patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/525be96f02e9
report AEC log dir through getter. r=ng,smaug

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/525be96f02e9