1494745 - Simplify nsRefreshDriver::AddRefreshObserver because it's infallible

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Mats Palmgren (PTO)]

https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/layout/base/nsRefreshDriver.cpp#1212,1222
These calls looks infallible to me, which means we could simplify callers.

ObserverArray is a nsTObserverArray<nsARefreshObserver*>:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/layout/base/nsRefreshDriver.h#416
which is a nsAutoTObserverArray<T, 0>:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/xpcom/ds/nsTObserverArray.h#459
which uses a AutoTArray<T, N> mArray:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/xpcom/ds/nsTObserverArray.h#225
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/xpcom/ds/nsTObserverArray.h#455
which is a nsTArray:
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/xpcom/ds/nsTArray.h#2839
which is infallible.

Comment 1

[Mats Palmgren (PTO)]

Well, RemoveRefreshObserver return whether the observer was found,
it doesn't signal OOM...

Comment 2

[Mats Palmgren (PTO)]

Attachment: part 6 - Make ns[Auto]TObserverArray methods that insert/append an Item return void since they're infallible

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

Comment 3

[Mats Palmgren (PTO)]

Attachment: part 1 - Don't check the result from ns[Auto]TObserverArray::AppendElement since it's infallible

Comment 4

[Mats Palmgren (PTO)]

Attachment: part 2 - Make nsRefreshDriver::AddRefreshObserver void since it's infallible

Comment 5

[Mats Palmgren (PTO)]

Attachment: part 3 - Make nsRefreshDriver::AddTimerAdjustmentObserver void since it's infallible

Comment 6

[Mats Palmgren (PTO)]

Attachment: part 4 - Make Loader::AddObserver void since it's infallible

Comment 7

[Mats Palmgren (PTO)]

Attachment: part 5 - Don't check the result from ns[Auto]TObserverArray insertions since they're infallible

Comment 8

[Boris Zbarsky [: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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9013051 [details] [diff] [review]
part 2 - Make nsRefreshDriver::AddRefreshObserver void since it's infallible

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9013053 [details] [diff] [review]
part 4 - Make Loader::AddObserver void since it's infallible

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Mats Palmgren (PTO)]

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

Comment 15

[Mats Palmgren (PTO)]

... also, RemoveTimerAdjustmentObserver is already asserting
MOZ_ASSERT(mTimerAdjustmentObservers.Contains(aObserver));
which is basically the same thing.

Comment 16

[Mats Palmgren (PTO)]

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

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4ac8ee3c8019
https://hg.mozilla.org/mozilla-central/rev/e0debd3f26f6
https://hg.mozilla.org/mozilla-central/rev/7fa23df410e4
https://hg.mozilla.org/mozilla-central/rev/1ec0c8e82a3d
https://hg.mozilla.org/mozilla-central/rev/74198da29da5
https://hg.mozilla.org/mozilla-central/rev/2b7fce8c4f92