Closed
Bug 1494745
Opened 3 years ago
Closed 2 years ago
Simplify nsRefreshDriver::AddRefreshObserver because it's infallible
Categories
(Core :: Layout, enhancement, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: mats, Assigned: mats)
Details
Attachments
(6 files)
3.10 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
10.08 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.64 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.76 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
4.86 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•3 years ago
|
||
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
Updated•3 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•3 years ago
|
||
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)
Assignee | ||
Comment 3•3 years ago
|
||
Attachment #9013050 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•3 years ago
|
||
Attachment #9013051 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•3 years ago
|
||
Attachment #9013052 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•3 years ago
|
||
Attachment #9013053 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•3 years ago
|
||
Attachment #9013054 -
Flags: review?(bzbarsky)
![]() |
||
Comment 8•2 years ago
|
||
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 9•2 years ago
|
||
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 10•2 years ago
|
||
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 11•2 years ago
|
||
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 12•2 years ago
|
||
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+
![]() |
||
Updated•2 years ago
|
Attachment #9013054 -
Flags: review?(bzbarsky) → review+
Comment 13•2 years ago
|
||
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
Assignee | ||
Comment 14•2 years ago
|
||
(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.
Assignee | ||
Comment 15•2 years ago
|
||
... also, RemoveTimerAdjustmentObserver is already asserting MOZ_ASSERT(mTimerAdjustmentObservers.Contains(aObserver)); which is basically the same thing.
Assignee | ||
Comment 16•2 years ago
|
||
(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•2 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•