1157898 - Get rid of ErrorResult::ErrorCode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

In particular, consumers that want to return the nsresult should use StealNSResult and everyone else we'll find other ways to handle.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make code of the form "return rv.ErrorCode();" where rv is an ErrorResult use StealNSResult instead

This patch was generated with the following command:

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 's/return ([a-zA-Z0-9]+)\.ErrorCode\(\);/return \1.StealNSResult();/'

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make code of the form "NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());" use Failed and StealNSResult instead

This patch was generated with the following command:

  find . -name "*.h" -o -name "*.cpp" | xargs perl -pi -e 's/NS_ENSURE_SUCCESS\(([a-zA-Z0-9]+)\.ErrorCode\(\), \1.ErrorCode\(\)\);/NS_ENSURE_TRUE(!\1.Failed(), \1.StealNSResult());/'

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Fix the remaining consumers of rv.ErrorCode() in NS_ENSURE_* expressions to not do that

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add ErrorResult::ErrorCodeIs() and use it in various places to get rid of ErrorCode()

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Eliminate the remaining non-ErrorResult consumers of ErrorResult::ErrorCode and make it protected

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Eliminate the remaining non-ErrorResult consumers of ErrorResult::ErrorCode and make it protected

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 8596956 [details] [diff] [review]
part 1.  Make code of the form "return rv.ErrorCode();" where rv is an ErrorResult use StealNSResult instead

Review of attachment 8596956 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me for any new ones that creep into the tree before you land.

::: dom/base/Element.h
@@ +1517,5 @@
>                          const nsAString& value) override                      \
>  {                                                                             \
>    mozilla::ErrorResult rv;                                                    \
>    Element::SetAttribute(name, value, rv);                                     \
> +  return rv.StealNSResult();                                                      \

Please fix up the location of the backslash.

::: dom/base/nsScreen.cpp
@@ +84,5 @@
>    nsScreen::Get ## _name(int32_t* aOut)                                         \
>    {                                                                             \
>      ErrorResult rv;                                                             \
>      *aOut = Get ## _name(rv);                                                   \
> +    return rv.StealNSResult();                                                      \

And here.

::: dom/html/nsGenericHTMLElement.h
@@ +489,1 @@
>    }                                                                            \

Huh, unrelated but this backslash looks like a mistake.

@@ +1526,5 @@
>    _class::Set##_method(uint32_t aValue)                                   \
>    {                                                                       \
>      mozilla::ErrorResult rv;                                              \
>      SetUnsignedIntAttr(nsGkAtoms::_atom, aValue, rv);                     \
> +    return rv.StealNSResult();                                                \

Fix up backslash location.

@@ +1553,5 @@
>        return NS_ERROR_DOM_INDEX_SIZE_ERR;                                 \
>      }                                                                     \
>      mozilla::ErrorResult rv;                                              \
>      SetUnsignedIntAttr(nsGkAtoms::_atom, aValue, rv);                     \
> +    return rv.StealNSResult();                                                \

And here.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8596958 [details] [diff] [review]
part 2.  Make code of the form "NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());" use Failed and StealNSResult instead

Review of attachment 8596958 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'd prefer NS_ENSURE_FALSE.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 8596959 [details] [diff] [review]
part 3.  Fix the remaining consumers of rv.ErrorCode() in NS_ENSURE_* expressions to not do that

Review of attachment 8596959 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/SpeechRecognitionError.cpp
@@ +41,5 @@
>                                                     const nsAString& aMessage,
>                                                     ErrorResult& aRv)
>  {
>    aRv = Event::InitEvent(aType, aCanBubble, aCancelable);
> +  NS_ENSURE_TRUE_VOID(!aRv.Failed());

I think I'd prefer NS_ENSURE_FALSE_VOID.
Bu also, shouldn't this SuppressException?

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8596960 [details] [diff] [review]
part 4.  Add ErrorResult::ErrorCodeIs() and use it in various places to get rid of ErrorCode()

Review of attachment 8596960 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/ErrorResult.h
@@ +128,5 @@
>    }
>  
>    // StealJSException steals the JS Exception from the object. This method must
>    // be called only if IsJSException() returns true. This method also resets the
> +  // error code to to NS_OK.

s/to to/to/

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I think I'd prefer NS_ENSURE_FALSE.

I sort of did too, but a few people (including Ehsan) said the double negative of "false(failed)" confused them...

> Bu also, shouldn't this SuppressException?

Good catch, yes.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Huh, unrelated but this backslash looks like a mistake.

Fixed.

> Bu also, shouldn't this SuppressException?

Looks like no, actually, since this is in fact an outparam that's going back to the bindings in the normal way.

Fixed all the other issues except the NS_ENSURE_FALSE; mailed you about that.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1ae21cc8e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/29bac3547929
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2007f89d25e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4902e0196c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba4bb455c23

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca1ae21cc8e0
https://hg.mozilla.org/mozilla-central/rev/29bac3547929
https://hg.mozilla.org/mozilla-central/rev/c2007f89d25e
https://hg.mozilla.org/mozilla-central/rev/ba4902e0196c
https://hg.mozilla.org/mozilla-central/rev/6ba4bb455c23