Closed Bug 1157898 Opened 5 years ago Closed 5 years ago

Get rid of ErrorResult::ErrorCode

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files, 1 obsolete file)

In particular, consumers that want to return the nsresult should use StealNSResult and everyone else we'll find other ways to handle.
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();/'
Attachment #8596956 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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());/'
Attachment #8596958 - Flags: review?(peterv)
Attachment #8596961 - Attachment is obsolete: true
Attachment #8596961 - Flags: review?(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.
Attachment #8596956 - Flags: review?(peterv) → review+
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.
Attachment #8596958 - Flags: review?(peterv) → review+
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?
Attachment #8596959 - Flags: review?(peterv) → review+
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/
Attachment #8596960 - Flags: review?(peterv) → review+
Attachment #8596996 - Flags: review?(peterv) → review+
> 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.
> 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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.