Closed Bug 1311606 Opened 8 years ago Closed 8 years ago

Replace |result| for nsreult under editor/ with |rv|

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

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1311606 Rename |result| of nsresult variants to |rv| in editor
(deleted), text/x-review-board-request
smaug
: review+
Details 
Similar to bug 1310618, there are a lot of places which use "result" as variants of nsresut. They should be replaced with "rv".
I found bug 1311626 at a patch for this.
Depends on: 1311626
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review86836

Apparently this has also some unrelated changes. s/result/rv/ should be one patch and other changes then in some other patches.
Otherwise review quality suffers, and reviewing takes a lot more time.

::: editor/libeditor/ChangeStyleTransaction.cpp:218
(Diff revision 1)
>  
>  nsresult
>  ChangeStyleTransaction::SetStyle(bool aAttributeWasSet,
>                                   nsAString& aValue)
>  {
> -  nsresult result = NS_OK;
> +  if (!aAttributeWasSet) {

Unrelated change.
Attachment #8803684 - Flags: review?(bugs) → review-
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review86836

> Unrelated change.

I removed the code rewriting with "early return" style from all of the patch.
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review86980

::: editor/libeditor/ChangeStyleTransaction.cpp:230
(Diff revision 2)
>      nsCOMPtr<nsICSSDeclaration> cssDecl = inlineStyles->Style();
>  
>      if (aValue.IsEmpty()) {
>        // An empty value means we have to remove the property
>        nsAutoString returnString;
> -      result = cssDecl->RemoveProperty(propertyNameString, returnString);
> +      return cssDecl->RemoveProperty(propertyNameString, returnString);

this stuff here is still a bit unrelated but fine.

::: editor/libeditor/TextEditor.cpp:591
(Diff revision 2)
>          *aAction = eNone;
> -        break;
> +        return selCont->WordExtendForDelete(false);
>        case eNext:
> -        result = selCont->CharacterExtendForDelete();
>          // Don't set aAction to eNone (see Bug 502259)
> -        break;
> +        return selCont->CharacterExtendForDelete();

Still lots of changes which aren't really about
s/result/rv/
This kind of case forces me to review the rest of the method to see if early return changes the behavior.
Attachment #8803684 - Flags: review?(bugs) → review-
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review86980

> Still lots of changes which aren't really about
> s/result/rv/
> This kind of case forces me to review the rest of the method to see if early return changes the behavior.

Hmm, I just tried to remove unnecessary variable |result| from each case, but that's fine to put off this change to another bug.
Sorry being a nicpicker here, but the patch was just huge enough that reviewing variable renames and then some random other changes was a bit difficult.
(In reply to Olli Pettay [:smaug] from comment #12)
> Sorry being a nicpicker here, but the patch was just huge enough that
> reviewing variable renames and then some random other changes was a bit
> difficult.

This patch does:

* renaming the variables.
* wrapping wrong lines if the replacing line is too long.
* removing the setting code if the value won't referred later.
* rewriting with NS_OK some return statements if result was already checked if an error code.
* omitting the code setting rv temporarily and return the value, for example, |rv = foo(); return rv;| -> |return foo();|

Is the new patch still not enough simple for you? If so, I'll discard current patch and recreate new one which just replaces the variable names. But I hope that you won't point any existing odd code. (Although, redoing other clean up which are included in current patch needs retry to check by my eyes again...)
Flags: needinfo?(bugs)
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review87042

::: editor/libeditor/ChangeStyleTransaction.cpp:230
(Diff revision 3)
>      nsCOMPtr<nsICSSDeclaration> cssDecl = inlineStyles->Style();
>  
>      if (aValue.IsEmpty()) {
>        // An empty value means we have to remove the property
>        nsAutoString returnString;
> -      result = cssDecl->RemoveProperty(propertyNameString, returnString);
> +      return cssDecl->RemoveProperty(propertyNameString, returnString);

here there are still some unrelated changes, but fine.

::: editor/txtsvc/nsTextServicesDocument.cpp:473
(Diff revision 3)
>      mNextTextBlock  = nullptr;
>    }
>  
>    UNLOCK_DOC(this);
>  
> -  return result;
> +  // XXX Result of FirstTextNode() or GetFirstTextNodeInNextBlock().

Not useful comment.

::: editor/txtsvc/nsTextServicesDocument.cpp:946
(Diff revision 3)
>      mNextTextBlock = nullptr;
>    }
>  
>    UNLOCK_DOC(this);
>  
> -  return result;
> +  // XXX The result of GetFirstTextNodeInNextBlock() or NS_OK.

Not seeing the usefulness of this comment, but up to you.

::: editor/txtsvc/nsTextServicesDocument.cpp:2327
(Diff revision 3)
> -    result = GetUncollapsedSelection(aSelStatus, aSelOffset, aSelLength);
> +    rv = GetUncollapsedSelection(aSelStatus, aSelOffset, aSelLength);
>    }
>  
>    // UNLOCK_DOC(this);
>  
> -  return result;
> +  // XXX The result of GetCollapsedSelection() or GetUncollapsedSelection().

Don't understand the need for this comment either.
Attachment #8803684 - Flags: review?(bugs) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> Is the new patch still not enough simple for you? 
I was already reviewing the patch :)
Flags: needinfo?(bugs)
Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

https://reviewboard.mozilla.org/r/87840/#review87042

> Don't understand the need for this comment either.

They are useful to rewrite the methods with "early return" style, I'll keep them until I rewrite them.

Thank you for your review.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5283a6d1c99f
Rename |result| of nsresult variants to |rv| in editor r=smaug
https://hg.mozilla.org/mozilla-central/rev/5283a6d1c99f
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: