924442 - Disallow pretty printing when a source is black boxed

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It doesn't make sense to pretty print a source when it is black boxed, so we shouldn't even allow it.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-924442-dont-pp-blackboxed.patch

Comment 2

[Victor Porof [:vporof][:vp]]

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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/a2e2bbab9438

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2e2bbab9438