Closed Bug 924442 Opened 6 years ago Closed 6 years ago

Disallow pretty printing when a source is black boxed

Categories

(DevTools :: Debugger, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file)

It doesn't make sense to pretty print a source when it is black boxed, so we shouldn't even allow it.
Assignee: nobody → nfitzgerald
Blocks: 921630
Priority: -- → P2
Comment on attachment 815973 [details] [diff] [review]
bug-924442-dont-pp-blackboxed.patch

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1152,5 @@
>     * @param Object aSource
>     *        The source form.
>     * @param bool aBlackBoxFlag
>     *        True to black box the source, false to un-black box it.
> +   * @returns Promise

Nit: add a description of what what this promise represents and when it it resolved vs. rejected.

::: browser/devtools/debugger/debugger-panes.js
@@ +728,5 @@
> +    if (shouldBlackBox) {
> +      this._prettyPrintButton.setAttribute("disabled", true);
> +    } else {
> +      this._prettyPrintButton.removeAttribute("disabled");
> +    }

Nit: maybe make _updatePrettyPrintButtonState take an optional sourceItem argument that defaults to the current source, and call that here? Your decision.

Nit2: maybe add a comment stating that blackboxing is async and that's why you're toggling the prettyPrintButton state both before and after.
Attachment #815973 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #2)
> Comment on attachment 815973 [details] [diff] [review]
> bug-924442-dont-pp-blackboxed.patch
> 
> Review of attachment 815973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +1152,5 @@
> >     * @param Object aSource
> >     *        The source form.
> >     * @param bool aBlackBoxFlag
> >     *        True to black box the source, false to un-black box it.
> > +   * @returns Promise
> 
> Nit: add a description of what what this promise represents and when it it
> resolved vs. rejected.

Good call.

> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +728,5 @@
> > +    if (shouldBlackBox) {
> > +      this._prettyPrintButton.setAttribute("disabled", true);
> > +    } else {
> > +      this._prettyPrintButton.removeAttribute("disabled");
> > +    }
> 
> Nit: maybe make _updatePrettyPrintButtonState take an optional sourceItem
> argument that defaults to the current source, and call that here? Your
> decision.
> 

Doesn't work because the source is still not black boxed at this moment so even passing in the source wouldn't work. We could add an invert flag/param/whatever, but I don't really like that more than this anyways.

> Nit2: maybe add a comment stating that blackboxing is async and that's why
> you're toggling the prettyPrintButton state both before and after.

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