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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Similar to bug 1310618, there are a lot of places which use "result" as variants of nsresut. They should be replaced with "rv".

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I found bug 1311626 at a patch for this.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eefd324a818

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a803a9caeb

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

For conforming to our coding rules, |result| of nsresult variants should be renamed to |rv|.

Review commit: https://reviewboard.mozilla.org/r/87840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87840/

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaa37797b9b4

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87840/diff/1-2/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8803684 [details]
Bug 1311606 Rename |result| of nsresult variants to |rv| in editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87840/diff/2-3/

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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...)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(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 :)

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5283a6d1c99f
Rename |result| of nsresult variants to |rv| in editor r=smaug

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5283a6d1c99f