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

RESOLVED FIXED in Firefox 52

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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!
Comment hidden (mozreview-request)

Comment 10

2 years ago
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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/026a4084ad97
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.