Bug 117616 – Web Inspector: JS PrettyPrinting in do/while loops, "while" should be on the same line as "}" if there was a closing brace

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

TEST:

  do{"x"}while(0);

CURRENTLY:

  do {
    "x"
  }
  while (0);

EXPECTED:

  do {
    "x"
  } while (0);

[Timothy Hatcher]

Moving to the right component.

[Radar WebKit Bug Importer]

<rdar://problem/15796884>

[Dana Burkart]

Created attachment 275127 [details]
Patch

Fixes do/while formatting in the WebInspector.

[Joseph Pecoraro]

Comment on attachment 275127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275127&action=review

Nice! Very cool. I have one question that still needs clarification.

> LayoutTests/inspector/codemirror/resources/prettyprinting/javascript-tests/do-while-within-if.js:2
> +if (true) { do {var x;} while (0);}
>  \ No newline at end of file

We should add these newlines to the tests (and an extra newline to the expected results).

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:58
> +            else if (content === "while" && lastContent === "}") {
> +                return state._jsPrettyPrint.lastContentBeforeBlock === "do";
> +            }

Nit: No need for else, the previous if always returns. WebKit style is that single line conditional blocks do not get braces.

    if (content === "while" && lastContent === "}") // do/while
        return state._jsPrettyPrint.lastContentBeforeBlock === "do";

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:149
> +            else if (content === "while" && lastContent === "}") {
> +                return state._jsPrettyPrint.lastContentBeforeBlock === "do";
> +            }

Ditto.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:211
> +                lastContentBeforeBlock: undefined, 

This should have a comment. Something like, "// Used to detect if this was a do/while"

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:311
> +            var savedState = state._jsPrettyPrint.openBraceStartMarkers.pop();

I'd like to better understand this. The pop() seems bad, that sounds like we probably messed up earlier. I would expect openBraceStartMarkers to always be empty if we don't have an indent. Perhaps the mistake here is in the Dedent all the way block.

Which test case tests this particular block?

[Dana Burkart]

(In reply to comment #4)
> Comment on attachment 275127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275127&action=review
> 
> Nice! Very cool. I have one question that still needs clarification.
> 
> > LayoutTests/inspector/codemirror/resources/prettyprinting/javascript-tests/do-while-within-if.js:2
> > +if (true) { do {var x;} while (0);}
> >  \ No newline at end of file
> 
> We should add these newlines to the tests (and an extra newline to the
> expected results).

Sure.

> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:58
> > +            else if (content === "while" && lastContent === "}") {
> > +                return state._jsPrettyPrint.lastContentBeforeBlock === "do";
> > +            }
> 
> Nit: No need for else, the previous if always returns. WebKit style is that
> single line conditional blocks do not get braces.
> 
>     if (content === "while" && lastContent === "}") // do/while
>         return state._jsPrettyPrint.lastContentBeforeBlock === "do";

OK.

> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:149
> > +            else if (content === "while" && lastContent === "}") {
> > +                return state._jsPrettyPrint.lastContentBeforeBlock === "do";
> > +            }
> 
> Ditto.

OK.
> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:211
> > +                lastContentBeforeBlock: undefined, 
> 
> This should have a comment. Something like, "// Used to detect if this was a
> do/while"

Got it.

> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:311
> > +            var savedState = state._jsPrettyPrint.openBraceStartMarkers.pop();
> 
> I'd like to better understand this. The pop() seems bad, that sounds like we
> probably messed up earlier. I would expect openBraceStartMarkers to always
> be empty if we don't have an indent. Perhaps the mistake here is in the
> Dedent all the way block.

That's a good point, while working on this, it was definitely the case that something
was left on the stack. Looking at it some more, it's possible that we never initialize
the openBraceTrackingCount properly. Then, if we increment from the initial value of -1
to 0, we would skip the code path where we would call pop().

I'll look at this some more.

> 
> Which test case tests this particular block?

The basic do/while case was leaving something on the stack.

[Dana Burkart]

Created attachment 275156 [details]
patch

Fixes issues brought up in Joe's review.

[Joseph Pecoraro]

Comment on attachment 275156 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275156&action=review

Okay! This makes more sense to me. r=me, but address the style comments.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:56
> +            else if (content === "while" && lastContent === "}")

Style: The `else` is not needed here.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:146
> +            else if (content === "while" && lastContent === "}")

Style: The `else` is not needed here.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:208
> +                lastContentBeforeBlock: undefined, // Used to detect if this was a do/while

Style: Should end in a period. Sorry, I had left that out before!

[Dana Burkart]

Committed r198815.
http://trac.webkit.org/changeset/198815