Closed Bug 896534 Opened 6 years ago Closed 6 years ago

Tests should set the protocol logging pref before loading the debugger server

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: past, Assigned: past)

Details

Attachments

(1 file)

The reason protocol logging is currently enabled in the debugger front-end but disabled in the back-end is that it is set after the debugger server is loaded. The server tries to make the check for logging as fast as possible by checking the pref when the module is loaded and closing over the constant afterwards. If we want the setting to have any effect on the server, we need to move it before loading the server module.
Attached patch logging.patchSplinter Review
This patch will make things more consistent by disabling logging for both frontend and backend as discussed. We'll have to change the value to false when we push to try to investigate oranges.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attachment #779238 - Flags: review?(rcampbell)
Priority: -- → P2
Comment on attachment 779238 [details] [diff] [review]
logging.patch

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

::: browser/devtools/debugger/test/head.js
@@ +24,5 @@
>  let DebuggerTransport = tempScope.DebuggerTransport;
>  let DebuggerClient = tempScope.DebuggerClient;
>  let gDevTools = tempScope.gDevTools;
>  let devtools = tempScope.devtools;
>  let TargetFactory = devtools.TargetFactory;

I feel like this stuff would look better using some destructuring assignments now. tempScope is gross.

up to you if you want to change it in this patch or not.
Attachment #779238 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> I feel like this stuff would look better using some destructuring
> assignments now. tempScope is gross.
> 
> up to you if you want to change it in this patch or not.

I think I'll do it in another patch. I've been using this along with various tentative intermittent fixes for bug 895426 and I'm wary of invalidating my try runs/results.
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
(In reply to Panos Astithas [:past] from comment #3)
> 
> I feel like this stuff would look better using some destructuring
> assignments now. tempScope is gross.
> 
> up to you if you want to change it in this patch or not.

I'm fixing that in bug 876277. Unfortunately for him, I'm going to ask Panos for review :)
https://hg.mozilla.org/integration/fx-team/rev/c048915f1a4b

(In reply to Victor Porof [:vp] from comment #4)
> I'm fixing that in bug 876277. Unfortunately for him, I'm going to ask Panos
> for review :)

Rats!
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c048915f1a4b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.