Bug 117616

Summary: Web Inspector: JS PrettyPrinting in do/while loops, "while" should be on the same line as "}" if there was a closing brace
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: dburkart, joepeck, matthew_hanson, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
patch joepeck: review+

Description Joseph Pecoraro 2013-06-13 16:20:20 PDT
TEST:

  do{"x"}while(0);

CURRENTLY:

  do {
    "x"
  }
  while (0);

EXPECTED:

  do {
    "x"
  } while (0);
Comment 1 Timothy Hatcher 2014-01-10 15:37:21 PST
Moving to the right component.
Comment 2 Radar WebKit Bug Importer 2014-01-10 15:41:27 PST
<rdar://problem/15796884>
Comment 3 Dana Burkart 2016-03-29 13:47:41 PDT
Created attachment 275127 [details]
Patch

Fixes do/while formatting in the WebInspector.
Comment 4 Joseph Pecoraro 2016-03-29 15:05:18 PDT
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?
Comment 5 Dana Burkart 2016-03-29 15:39:50 PDT
(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.
Comment 6 Dana Burkart 2016-03-29 17:12:37 PDT
Created attachment 275156 [details]
patch

Fixes issues brought up in Joe's review.
Comment 7 Joseph Pecoraro 2016-03-29 17:26:20 PDT
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!
Comment 8 Dana Burkart 2016-03-29 17:31:38 PDT
Committed r198815.
http://trac.webkit.org/changeset/198815