Closed Bug 1311240 Opened 8 years ago Closed 8 years ago

Move '{' to the end of if/while/for condition or else in editor

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1311240 Fix odd "{" and "}" of control statements in editor for conforming to our coding rules
58 bytes, text/x-review-board-request
smaug
: review+
Details 
editor code sometimes uses following style:

if (foo)
{

}
else
{

}

We can search this with looking for a line which includes only '{' and whitespaces.

Let's rewrite it as:

if (foo) {

} else {

}
Status: NEW → ASSIGNED
Comment on attachment 8803683 [details]
Bug 1311240 Fix odd "{" and "}" of control statements in editor for conforming to our coding rules

https://reviewboard.mozilla.org/r/87836/#review86824

This patch really should have focused on just some changes, not random changes everywhere. It was very time consuming to review.

::: editor/libeditor/HTMLTableEditor.cpp:1820
(Diff revision 1)
>                    &actualRowSpan, &actualColSpan, &isSelected);
>    NS_ENSURE_SUCCESS(rv, rv);
>    NS_ENSURE_TRUE(cell, NS_ERROR_NULL_POINTER);
>  
>    // We can't split!
> -  if (actualColSpan <= 1 || (aColSpanLeft + aColSpanRight) > actualColSpan)
> +  if (actualColSpan <= 1 || aColSpanLeft + aColSpanRight > actualColSpan) {

I'd prefer keeping () in this kind of case.

::: editor/libeditor/HTMLTableEditor.cpp:1866
(Diff revision 1)
>                    &actualRowSpan, &actualColSpan, &isSelected);
>    NS_ENSURE_SUCCESS(rv, rv);
>    NS_ENSURE_TRUE(cell, NS_ERROR_NULL_POINTER);
>  
>    // We can't split!
> -  if (actualRowSpan <= 1 || (aRowSpanAbove + aRowSpanBelow) > actualRowSpan)
> +  if (actualRowSpan <= 1 || aRowSpanAbove + aRowSpanBelow > actualRowSpan) {

ditto

::: editor/libeditor/HTMLTableEditor.cpp:2107
(Diff revision 1)
>            cellFoundInRow = true;
> -        }
> +        } else if (cellFoundInRow) {
> -        else if (cellFoundInRow)
> -        {
>            // No cell or not selected, but at least one cell in row was found
> -
> +          if (rowIndex > (firstRowIndex+1) && colIndex <= lastColIndex) {

(not about this bug, but there should be space before and after '+')

::: editor/libeditor/WSRunObject.cpp:925
(Diff revision 1)
>  
>      // we might have trailing ws.
>      // it so happens that *if* there is an nbsp at end, {mEndNode,mEndOffset-1}
>      // will point to it, even though in general start/end points not
>      // guaranteed to be in text nodes.
> -    if ((mLastNBSPNode == mEndNode) && (mLastNBSPOffset == (mEndOffset-1)))
> +    if (mLastNBSPNode == mEndNode && mLastNBSPOffset == mEndOffset - 1) {

I would prefer to keep () here in the mEndOffset - 1 case.

::: editor/txtsvc/nsTextServicesDocument.cpp:2921
(Diff revision 1)
> -  while (!iter->IsDone())
> +  while (!iter->IsDone()) {
> -  {
>      nsCOMPtr<nsIContent> content = iter->GetCurrentNode()->IsContent()
>                                     ? iter->GetCurrentNode()->AsContent()
>                                     : nullptr;
> -
> +    if (last && IsBlockNode(content)) {

This kind of changes really make reviewing massive patches like this a lot slower.
This patch should have just fixed {} and nothing else. Other changes should have been done in separate patches.

::: editor/txtsvc/nsTextServicesDocument.cpp
(Diff revision 1)
> -        prev = content;
> +      prev = content;
>  
> -        if (!first)
> +      if (!first) {
> -          first = content;
> +        first = content;
>        }
> -      else

This kind of changes really make reviewing this all a lot slower.
Attachment #8803683 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #7)
> ::: editor/txtsvc/nsTextServicesDocument.cpp:2921
> (Diff revision 1)
> > -  while (!iter->IsDone())
> > +  while (!iter->IsDone()) {
> > -  {
> >      nsCOMPtr<nsIContent> content = iter->GetCurrentNode()->IsContent()
> >                                     ? iter->GetCurrentNode()->AsContent()
> >                                     : nullptr;
> > -
> > +    if (last && IsBlockNode(content)) {
> 
> This kind of changes really make reviewing massive patches like this a lot
> slower.
> This patch should have just fixed {} and nothing else. Other changes should
> have been done in separate patches.
> 
> ::: editor/txtsvc/nsTextServicesDocument.cpp
> (Diff revision 1)
> > -        prev = content;
> > +      prev = content;
> >  
> > -        if (!first)
> > +      if (!first) {
> > -          first = content;
> > +        first = content;
> >        }
> > -      else
> 
> This kind of changes really make reviewing this all a lot slower.

Sorry for about the complicated points. I'll try to separate such points even with big patch like this.

And thank you for your quick review!
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/026a4084ad97
Fix odd "{" and "}" of control statements in editor for conforming to our coding rules r=smaug
https://hg.mozilla.org/mozilla-central/rev/026a4084ad97
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: