Closed Bug 1093349 Opened 8 years ago Closed 8 years ago

Add pretty printing and blackboxing traits and hide buttons conditionally

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: jlong, Assigned: jlong)

Details

Attachments

(1 file, 2 obsolete files)

This is for Fever Dream, which right now doesn't support pretty printing or blackboxing. We want to simply remove those buttons in the debugger if those actions aren't supported, and we specify that by adding some traits.
Attached patch 1093349.patch (obsolete) — Splinter Review
Attached patch 1093349.patch (obsolete) — Splinter Review
Attachment #8516311 - Attachment is obsolete: true
Comment on attachment 8516317 [details] [diff] [review]
1093349.patch

Not sure who I should ask to review this; past could you take a quick look?
Attachment #8516317 - Flags: review?(past)
Comment on attachment 8516317 [details] [diff] [review]
1093349.patch

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

Could you also add a simple test for this? Something like manually starting a debugger server, forcing the new traits to true, opening the toolbox and then verifying that the buttons are hidden.

::: browser/devtools/debugger/debugger-view.js
@@ +72,5 @@
>    },
>  
> +  connect: function() {
> +    if (gClient.mainRoot.traits.noPrettyPrinting) {
> +      this.Sources.hidePrettyPrinting();

My only concern is that you are now introducing a dependency to the remote debugging protocol in DebuggerView. We've tried to only make the controller protocol-aware, so if you could move this function there, this would be perfect!
Attachment #8516317 - Flags: review?(past) → feedback+
(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #4)
> 
> My only concern is that you are now introducing a dependency to the remote
> debugging protocol in DebuggerView. We've tried to only make the controller
> protocol-aware, so if you could move this function there, this would be
> perfect!

Great, already done. Just need to write the test, will do that first thing tomorrow.
Attached patch 1093349.patchSplinter Review
Added a test and made the controller hide the buttons
Attachment #8516317 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, can we get a try link for this changes ? :) 

Thanks!
Keywords: checkin-needed
Sure thing. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7b0ddfba982 This only touches the debugger
Keywords: checkin-needed
Assignee: nobody → jlong
https://hg.mozilla.org/integration/fx-team/rev/85786620f4af
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/85786620f4af
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.