916180 - make pretty printing toggle-able

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

As of part 1 of bug 762761, we can pretty print sources over the RDP via

  { to: sourceActor, type: "prettyPrint", indent: N }

but we have no way of undoing the pretty printing. We should add

send

  { to: sourceActor, type: "disablePrettyPrint" }

receive

  { from: sourceActor, source: uglySource, contentType: type }

Comment 1

[Hubert B Manilla]

I have a thought and some questions on the implementation of this.
Should and would the disablePrettyPrint just return the source to its previous state (ie before it was pretty printed) or actually process the pretty printed code again to minify it back.
If the later is the case what is going to happen to code that is not obfuscated when we click the pretty print button twice (ie to pretty Print and then to disable Pretty Print)?

I think the idea behind the pretty print is to unminify minified code for the developer can view it. The toggle should just return the code to its previous obfuscated state rather than process the code again to minify it.
This should increase performance too, and we really don't need the functionality to minify code which is not minified.
Thoughts please.

Comment 2

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

(In reply to Hubert B Manilla from comment #1)
> I have a thought and some questions on the implementation of this.
> Should and would the disablePrettyPrint just return the source to its
> previous state (ie before it was pretty printed) or actually process the
> pretty printed code again to minify it back.

Yes it should return to its previous state, we're not going to minify the prettified source or anything like that.

> I think the idea behind the pretty print is to unminify minified code for
> the developer can view it. The toggle should just return the code to its
> previous obfuscated state rather than process the code again to minify it.
> This should increase performance too, and we really don't need the
> functionality to minify code which is not minified.
> Thoughts please.

Not sure I follow you here.

The one tricky thing is that we have to disable the layer of source mapping we applied. This is pretty easy when the source wasn't source mapped originally because we can just blow away the source map for the source. When we are pretty printing a source mapped file, and then disabling the pretty printing, we can't completely blow away the source map, but just have to unwrap one layer.

I have some ideas on how to implement this in my head, but I just need to wrap up bug 918802 before I start here.

Comment 3

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

Attachment: WIP bug-916180-pp-toggle.patch

WIP patch

Allows pretty printing to be toggled on and off, and to persist across refreshes, but I don't have the tests all groovy yet.

Comment 4

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

Attachment: bug-916180-pp-toggle.patch

No longer WIP!

Can't split server/client and front end changes out from each other because all pretty printing tests need to be in mochitests because xpcshell can't handle ChromeWorkers. Sorry Panos :(

This patch includes:

* toggling of pretty printing on and off

* maintaining pretty printed state across reloads

* tests for both of the above

Comment 5

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

Comment on attachment 817552 [details] [diff] [review]
bug-916180-pp-toggle.patch

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

Sorry for the drive-by :)

::: browser/devtools/debugger/debugger-controller.js
@@ +694,5 @@
>      }
>  
>      // Make sure the debugger view panes are visible, then refill the frames.
>      DebuggerView.showInstrumentsPane();
> +    this._refillFrames(CALL_STACK_PAGE_SIZE);

This function doesn't take any arguments.

@@ +753,5 @@
> +   */
> +  _onPrettyPrintChange: function() {
> +    if (this.activeThread.state == "paused") {
> +      this.currentFrame = null;
> +      this.activeThread.fillFrames();

This one does though :)

@@ +1208,5 @@
>      if (!SourceUtils.isJavaScript(aSource.url, aSource.contentType)) {
>        return promise.reject([aSource, "Can't prettify non-javascript files."]);
>      }
>  
> +    const wantPretty = !this.activeThread.source(aSource).isPrettyPrinted;

Can you cache this.activeThread.source(aSource) ?

::: browser/devtools/debugger/debugger-panes.js
@@ +27,5 @@
>    this._onConditionalPopupShown = this._onConditionalPopupShown.bind(this);
>    this._onConditionalPopupHiding = this._onConditionalPopupHiding.bind(this);
>    this._onConditionalTextboxInput = this._onConditionalTextboxInput.bind(this);
>    this._onConditionalTextboxKeyPress = this._onConditionalTextboxKeyPress.bind(this);
> +  this._updatePrettyPrintButtonState = this._updatePrettyPrintButtonState.bind(this);

I don't think you need to bind this? Am I missing something?

@@ +63,5 @@
>      this.widget.addEventListener("select", this._onSourceSelect, false);
>      this.widget.addEventListener("click", this._onSourceClick, false);
>      this.widget.addEventListener("check", this._onSourceCheck, false);
>      this._stopBlackBoxButton.addEventListener("click", this._onStopBlackBoxing, false);
> +    this._prettyPrintButton.addEventListener("click", this.togglePrettyPrint, false);

You need a command listener, not click. Suppose someone tabs to this button and presses space, for example.

::: browser/devtools/debugger/debugger-view.js
@@ +70,5 @@
> +      this._initializeVariablesView();
> +      this._initializeEditor(deferred.resolve);
> +    } catch (e) {
> +      DevToolsUtils.reportException("DebuggerView.initialize", e);
> +      deferred.reject(e);

Why would this fail? This huge try/catch can potentially cause slowness.

::: browser/devtools/debugger/debugger.xul
@@ -327,5 @@
>              <toolbarbutton id="pretty-print"
>                             label="{}"
>                             tooltiptext="&debuggerUI.sources.prettyPrint;"
>                             class="devtools-toolbarbutton devtools-monospace"
> -                           command="prettyPrintCommand"

Why was it necessary to add the listener via js and not here?

Comment 6

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

(In reply to Victor Porof [:vp] from comment #5)
> Comment on attachment 817552 [details] [diff] [review]
> bug-916180-pp-toggle.patch
> 
> Review of attachment 817552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the drive-by :)
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +694,5 @@
> >      }
> >  
> >      // Make sure the debugger view panes are visible, then refill the frames.
> >      DebuggerView.showInstrumentsPane();
> > +    this._refillFrames(CALL_STACK_PAGE_SIZE);
> 
> This function doesn't take any arguments.
> 
> @@ +753,5 @@
> > +   */
> > +  _onPrettyPrintChange: function() {
> > +    if (this.activeThread.state == "paused") {
> > +      this.currentFrame = null;
> > +      this.activeThread.fillFrames();
> 
> This one does though :)

>_<

> 
> @@ +1208,5 @@
> >      if (!SourceUtils.isJavaScript(aSource.url, aSource.contentType)) {
> >        return promise.reject([aSource, "Can't prettify non-javascript files."]);
> >      }
> >  
> > +    const wantPretty = !this.activeThread.source(aSource).isPrettyPrinted;
> 
> Can you cache this.activeThread.source(aSource) ?

Sure thing, I don't think it is a huge deal because the ThreadClient is also maintaining a cache, but it is less unnecessary function calls.

> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +27,5 @@
> >    this._onConditionalPopupShown = this._onConditionalPopupShown.bind(this);
> >    this._onConditionalPopupHiding = this._onConditionalPopupHiding.bind(this);
> >    this._onConditionalTextboxInput = this._onConditionalTextboxInput.bind(this);
> >    this._onConditionalTextboxKeyPress = this._onConditionalTextboxKeyPress.bind(this);
> > +  this._updatePrettyPrintButtonState = this._updatePrettyPrintButtonState.bind(this);
> 
> I don't think you need to bind this? Am I missing something?

Yeah it is passed as promise callbacks directly and needs to maintain `this`.

> 
> @@ +63,5 @@
> >      this.widget.addEventListener("select", this._onSourceSelect, false);
> >      this.widget.addEventListener("click", this._onSourceClick, false);
> >      this.widget.addEventListener("check", this._onSourceCheck, false);
> >      this._stopBlackBoxButton.addEventListener("click", this._onStopBlackBoxing, false);
> > +    this._prettyPrintButton.addEventListener("click", this.togglePrettyPrint, false);
> 
> You need a command listener, not click. Suppose someone tabs to this button
> and presses space, for example.
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +70,5 @@
> > +      this._initializeVariablesView();
> > +      this._initializeEditor(deferred.resolve);
> > +    } catch (e) {
> > +      DevToolsUtils.reportException("DebuggerView.initialize", e);
> > +      deferred.reject(e);
> 
> Why would this fail? This huge try/catch can potentially cause slowness.

It was so annoying trying to debug issues in the initialization, especially because I couldn't use the browser debugger either. I can remove it if you want.

> 
> ::: browser/devtools/debugger/debugger.xul
> @@ -327,5 @@
> >              <toolbarbutton id="pretty-print"
> >                             label="{}"
> >                             tooltiptext="&debuggerUI.sources.prettyPrint;"
> >                             class="devtools-toolbarbutton devtools-monospace"
> > -                           command="prettyPrintCommand"
> 
> Why was it necessary to add the listener via js and not here?

So originally I had both and it was firing twice, I tried removing the click listener, but then EventUtils.sendMouseEvent({ type: "click" }) in my tests wouldn't trigger the callback. IIRC { type: "command" } isn't supported either.

Will play with this some more, see if I can get to a pure "command" setting.

Comment 7

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

Attachment: bug-916180-pp-toggle.patch

Moving review to you, Victor, this is what you get from drive-by'ing ;)

(Actually, I just couldn't split the patch into a front and back end part, so I flagged Panos because I figured he probably has the most experience across both sides)

Comment 8

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

Comment on attachment 819188 [details] [diff] [review]
bug-916180-pp-toggle.patch

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

::: browser/devtools/debugger/debugger-controller.js
@@ +752,5 @@
> +   * Handler for the debugger's prettyprintchange notification.
> +   */
> +  _onPrettyPrintChange: function() {
> +    if (this.activeThread.state == "paused") {
> +      this.currentFrame = null;

The currentFrame propery doesn't exist. Maybe you mean currentFrameDepth, which is always a number? (In which case the _onBlacBoxChange function also needs updating).

@@ +1244,4 @@
>      return deferred.promise;
>    },
>  
> +  _onPrettyPrintChange: function(aEvent, { url }) {

Maybe add a comment for this handler as well, since you added one for DebuggerController.StackFrames?

::: browser/devtools/debugger/debugger-panes.js
@@ +63,5 @@
>      this.widget.addEventListener("select", this._onSourceSelect, false);
>      this.widget.addEventListener("click", this._onSourceClick, false);
>      this.widget.addEventListener("check", this._onSourceCheck, false);
>      this._stopBlackBoxButton.addEventListener("click", this._onStopBlackBoxing, false);
> +    this._prettyPrintButton.addEventListener("command", this.togglePrettyPrint, false);

Nit: So if the "command" event works, why not keep it in XUL?

@@ +397,2 @@
>      const printError = ([{ url }, error]) => {
> +      DevToolsUtils.reportException("togglePrettyPrint", error);

OH CAN WE USE THIS EVERYWHERE NOW? @_@

::: toolkit/devtools/server/actors/script.js
@@ +2615,5 @@
> +   */
> +  onDisablePrettyPrint: function SA_onDisablePrettyPrint() {
> +    this._sourceMap = this._oldSourceMap;
> +    this.threadActor.sources.saveSourceMap(this._sourceMap,
> +                                           this._generatedSource || this._url);

Good thinking.

@@ +2644,5 @@
>      this.threadActor.sources.unblackBox(this.url);
>      return {
>        from: this.actorID
>      };
> +  },

Nit: unnecessary comma.

@@ +3887,5 @@
>     * down the line.
>     */
>    saveSourceMap: function TS_saveSourceMap(aSourceMap, aGeneratedSource) {
> +    if (!aSourceMap) {
> +      delete this._sourceMapsByGeneratedSource[aGeneratedSource];

Won't ... = null work? (assuming other code doesn't check for ("_sourceMapsByGeneratedSource" in whatever).

Comment 9

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

(In reply to Victor Porof [:vp] from comment #8)
> Comment on attachment 819188 [details] [diff] [review]
> bug-916180-pp-toggle.patch
> 
> Review of attachment 819188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +752,5 @@
> > +   * Handler for the debugger's prettyprintchange notification.
> > +   */
> > +  _onPrettyPrintChange: function() {
> > +    if (this.activeThread.state == "paused") {
> > +      this.currentFrame = null;
> 
> The currentFrame propery doesn't exist. Maybe you mean currentFrameDepth,
> which is always a number? (In which case the _onBlacBoxChange function also
> needs updating).
> 
> @@ +1244,4 @@
> >      return deferred.promise;
> >    },
> >  
> > +  _onPrettyPrintChange: function(aEvent, { url }) {
> 
> Maybe add a comment for this handler as well, since you added one for
> DebuggerController.StackFrames?
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +63,5 @@
> >      this.widget.addEventListener("select", this._onSourceSelect, false);
> >      this.widget.addEventListener("click", this._onSourceClick, false);
> >      this.widget.addEventListener("check", this._onSourceCheck, false);
> >      this._stopBlackBoxButton.addEventListener("click", this._onStopBlackBoxing, false);
> > +    this._prettyPrintButton.addEventListener("command", this.togglePrettyPrint, false);
> 
> Nit: So if the "command" event works, why not keep it in XUL?
> 
> @@ +397,2 @@
> >      const printError = ([{ url }, error]) => {
> > +      DevToolsUtils.reportException("togglePrettyPrint", error);
> 
> OH CAN WE USE THIS EVERYWHERE NOW? @_@
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +2615,5 @@
> > +   */
> > +  onDisablePrettyPrint: function SA_onDisablePrettyPrint() {
> > +    this._sourceMap = this._oldSourceMap;
> > +    this.threadActor.sources.saveSourceMap(this._sourceMap,
> > +                                           this._generatedSource || this._url);
> 
> Good thinking.
> 
> @@ +2644,5 @@
> >      this.threadActor.sources.unblackBox(this.url);
> >      return {
> >        from: this.actorID
> >      };
> > +  },
> 
> Nit: unnecessary comma.
> 
> @@ +3887,5 @@
> >     * down the line.
> >     */
> >    saveSourceMap: function TS_saveSourceMap(aSourceMap, aGeneratedSource) {
> > +    if (!aSourceMap) {
> > +      delete this._sourceMapsByGeneratedSource[aGeneratedSource];
> 
> Won't ... = null work? (assuming other code doesn't check for
> ("_sourceMapsByGeneratedSource" in whatever).

There's code that checks if stuff is in there via the "in" operator. Don't need to worry about dictionary mode here because there is only one of these objects per thread. Not a whole lot.

Comment 10

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

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

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/83562506fa87