921630 - Show progress indicator while pretty printing

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

After bug 918802, pretty printing won't block firefox at all, but it will still take a couple seconds to pretty print, most of which are off main thread. We should give the user an idea that things are happening with a progress indicator.

Comment 1

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

Attachment: bug-921630-pp-progress-bar.patch

Played with the "fake" progress bars we talked about in Paris; couldn't make it feel right, don't think it is worth the effort at this time.

Comment 2

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

Comment on attachment 814245 [details] [diff] [review]
bug-921630-pp-progress-bar.patch

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

In principle this might be ok, but I agree with you that it doesn't feel quite right. It would make more sense to wait for a general purpose progress indicator at the toolbox level, or at least talk about it some more. AFAIK, we didn't come to any decisions about that.

::: browser/devtools/debugger/debugger-panes.js
@@ +393,5 @@
>        dumpn(msg);
>        return;
>      }
>  
> +    const lastIndex = this._editorDeck.selectedIndex;

Doesn't look like you're using this anywhere?

@@ +394,5 @@
>        return;
>      }
>  
> +    const lastIndex = this._editorDeck.selectedIndex;
> +    this._editorDeck.selectedIndex = 2;

Nit: maybe we should define some consts somewhere describing what each tab in the editor deck represents. "2" isn't terribly semantic.

@@ +399,5 @@
> +
> +    const { source } = this.selectedItem.attachment;
> +    DebuggerController.SourceScripts.prettyPrint(source)
> +      .then(resetEditor, printError)
> +      .then(this.maybeShowBlackBoxMessage);

This looks like a hack. I would say it's better to just revert to the previous deck index instead. Blackboxing has nothing to do with pretty printing, and blackboxing a source while pretty printing shouldn't be possible.

Comment 3

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

(In reply to Victor Porof [:vp] from comment #2)
> Comment on attachment 814245 [details] [diff] [review]
> bug-921630-pp-progress-bar.patch
> 
> Review of attachment 814245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In principle this might be ok, but I agree with you that it doesn't feel
> quite right. It would make more sense to wait for a general purpose progress
> indicator at the toolbox level, or at least talk about it some more. AFAIK,
> we didn't come to any decisions about that.

You misunderstand me, originally I had a determined progress bar across the bottom of the toolbar that I faked the percent progress with, and that is what felt even worse than this.

> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +393,5 @@
> >        dumpn(msg);
> >        return;
> >      }
> >  
> > +    const lastIndex = this._editorDeck.selectedIndex;
> 
> Doesn't look like you're using this anywhere?

Woops! See comment

> 
> @@ +394,5 @@
> >        return;
> >      }
> >  
> > +    const lastIndex = this._editorDeck.selectedIndex;
> > +    this._editorDeck.selectedIndex = 2;
> 
> Nit: maybe we should define some consts somewhere describing what each tab
> in the editor deck represents. "2" isn't terribly semantic.

In another patch, I have defined showProgressBar, showBlackBoxMessage, and showEditor methods, but they still use numerals directly. Good enough? Or, also add constants for those methods to use?

> 
> @@ +399,5 @@
> > +
> > +    const { source } = this.selectedItem.attachment;
> > +    DebuggerController.SourceScripts.prettyPrint(source)
> > +      .then(resetEditor, printError)
> > +      .then(this.maybeShowBlackBoxMessage);
> 
> This looks like a hack. I would say it's better to just revert to the
> previous deck index instead. Blackboxing has nothing to do with pretty
> printing, and blackboxing a source while pretty printing shouldn't be
> possible.

This is what I was doing (left over lastIndex variable), but realized that maybeShowBlackBoxMessage would do exactly that for me, without me keeping track of things.

I think the better approach would be to disallow pretty printing black boxed sources (filed bug 924442) and then just show the editor again here.

Comment 4

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> 
> In another patch, I have defined showProgressBar, showBlackBoxMessage, and
> showEditor methods, but they still use numerals directly. Good enough? Or,
> also add constants for those methods to use?
> 

Nah, that's overkill. Those methods should be enough.

> > 
> > @@ +399,5 @@
> > > +
> > > +    const { source } = this.selectedItem.attachment;
> > > +    DebuggerController.SourceScripts.prettyPrint(source)
> > > +      .then(resetEditor, printError)
> > > +      .then(this.maybeShowBlackBoxMessage);
> > 
> > This looks like a hack. I would say it's better to just revert to the
> > previous deck index instead. Blackboxing has nothing to do with pretty
> > printing, and blackboxing a source while pretty printing shouldn't be
> > possible.
> 
> This is what I was doing (left over lastIndex variable), but realized that
> maybeShowBlackBoxMessage would do exactly that for me, without me keeping
> track of things.
> 

Sounds like maybeShowBlackBoxMessage needs a better (or more generic name) (or more generic functionality)? I don't know what would be better, but it would certainly *look* odd to see such a method being called there.

> I think the better approach would be to disallow pretty printing black boxed
> sources (filed bug 924442) and then just show the editor again here.

That too.

Comment 5

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

Attachment: bug-921630-pp-progress-bar.patch

Fixed up comments from last time, added test.

Comment 6

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

Comment on attachment 816003 [details] [diff] [review]
bug-921630-pp-progress-bar.patch

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

::: browser/devtools/debugger/debugger-view.js
@@ +116,5 @@
> +    this.showEditor = this.showEditor.bind(this);
> +    this.showBlackBoxMessage = this.showBlackBoxMessage.bind(this);
> +    this.showProgressBar = this.showProgressBar.bind(this);
> +    this.maybeShowBlackBoxMessage = this.maybeShowBlackBoxMessage.bind(this);
> +    this._editorDeck = document.getElementById("editor-deck");

Nit: add an _editorDeck: null property along the ones defined at the end of this object literal.

::: browser/devtools/debugger/debugger.xul
@@ +343,5 @@
>                      label="&debuggerUI.blackBoxMessage.unBlackBoxButton;"
>                      image="chrome://browser/skin/devtools/blackBoxMessageEye.png"
>                      command="unBlackBoxCommand"/>
>            </vbox>
> +          <vbox id="source-progress-container" align="center">

Can't you set pack="center" instead of using two flexed spacers underneath?

Comment 7

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

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

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/4ab2e3a7d4ff