Simplify nsRefreshDriver::AddRefreshObserver because it's infallible

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(6 attachments)

Well, RemoveRefreshObserver return whether the observer was found,
it doesn't signal OOM...
Summary: Simplify nsRefreshDriver::AddRefreshObserver/RemoveRefreshObserver because they are infallible → Simplify nsRefreshDriver::AddRefreshObserver because it's infallible
Priority: -- → P3
This is the last patch in this series.  It probably makes sense
to review this first so you see what the end goal is.
I guess we could keep returning the newly inserted element for
InsertElementAt/AppendElement to allow the caller to modify it
after insertion, but there are no consumers that actually need
that so I'm simplifying the API.  (It appears people just use
the methods that doesn't take an Item& instead.)
Assignee: nobody → mats
Attachment #9013049 - Flags: review?(bzbarsky)
Comment on attachment 9013050 [details] [diff] [review]
part 1 - Don't check the result from ns[Auto]TObserverArray::AppendElement since it's infallible

r=me
Attachment #9013050 - Flags: review?(bzbarsky) → review+
Comment on attachment 9013051 [details] [diff] [review]
part 2 - Make nsRefreshDriver::AddRefreshObserver void since it's infallible

r=me
Attachment #9013051 - Flags: review?(bzbarsky) → review+
Comment on attachment 9013052 [details] [diff] [review]
part 3 - Make nsRefreshDriver::AddTimerAdjustmentObserver void since it's infallible

Would be nice to add docs for what the return value of RemoveTimerAdjustmentObserver means.

r=me
Attachment #9013052 - Flags: review?(bzbarsky) → review+
Comment on attachment 9013053 [details] [diff] [review]
part 4 - Make Loader::AddObserver void since it's infallible

r=me
Attachment #9013053 - Flags: review?(bzbarsky) → review+
Comment on attachment 9013049 [details] [diff] [review]
part 6 - Make ns[Auto]TObserverArray methods that insert/append an Item return void since they're infallible

>+    mArray.InsertElementAt(aIndex, aItem);

Might be good to assert null is not returned....  Similar for the other callsites here.
Attachment #9013049 - Flags: review?(bzbarsky) → review+
Attachment #9013054 - Flags: review?(bzbarsky) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac8ee3c8019
part 1 - Don't check the result from ns[Auto]TObserverArray::AppendElement since it's infallible.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0debd3f26f6
part 2 - Make nsRefreshDriver::AddRefreshObserver void since it's infallible.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa23df410e4
part 3 - Make nsRefreshDriver::AddTimerAdjustmentObserver void since it's infallible.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec0c8e82a3d
part 4 - Make Loader::AddObserver void since it's infallible.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/74198da29da5
part 5 - Don't check the result from ns[Auto]TObserverArray insertions since they're infallible.  r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b7fce8c4f92
part 6 - Make ns[Auto]TObserverArray methods that insert/append an Item return void since they're infallible.  r=bz
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)
> Comment on attachment 9013052 [details] [diff] [review]
> part 3 - Make nsRefreshDriver::AddTimerAdjustmentObserver void since it's
> infallible
> 
> Would be nice to add docs for what the return value of
> RemoveTimerAdjustmentObserver means.

Actually, there's only one caller and it's not using the return value,
so I made this method 'void' too.
... also, RemoveTimerAdjustmentObserver is already asserting
MOZ_ASSERT(mTimerAdjustmentObservers.Contains(aObserver));
which is basically the same thing.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)
> >+    mArray.InsertElementAt(aIndex, aItem);
> 
> Might be good to assert null is not returned....  Similar for the other
> callsites here.

Doing that to all consumers of widely used types like nsTArray which
are infallible by default seems excessive.  It also harmful IMO since
it creates FUD in the reader's mind whether the call can fail or not.
It's better to make all infallible methods void.

I guess we could add a static_assert in nsTObserverArray that it's
underlying array type actually use InfallibleAllocPolicy by default,
but I think a fundamental change like that to a xpcom type would never
pass review anyway without being caught.
You need to log in before you can comment on or make changes to this bug.