Various cleanups/refactorings around nsCSSRendering::FindBackground
Categories
(Core :: Layout, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox103 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
Details
Attachments
(4 files)
When reviewing bug 1730763, I noticed that our nsCSSRendering::FindBackground
API feels unnecessarily clunky right now (mainly because it and its helpers use a bool+outparam pattern without really needing to do so).
I'm filing this bug for several cleanup-patches to improve this API and associated functions.
Assignee | ||
Comment 1•2 years ago
|
||
This patch does not change behavior.
This change is per our "Foo.cpp have Foo.h as its first header-inclusion"
convention, which helps us ensure that the header (nsCSSRendering.h in this
case) has all of the includes/declarations that it needs to compile properly on
its own, and doesn't have a fragile requirement that some other header must
be included before it.
Assignee | ||
Comment 2•2 years ago
|
||
This patch doesn't change behavior.
Before this patch, the FindBackground API returns a bool to indicate
success/failure, and sets its outparam to a non-null pointer-value on success.
This patch simplifies the API by just promoting the promoting the outparam to
be the actual return value, with nullptr as a sentinel value to indicate
failure.
(This patch adds some braces to affected/contextual if statements, too, to
match our coding style guide.)
Depends on D149336
Assignee | ||
Comment 3•2 years ago
|
||
This patch doesn't change behavior.
This is similar to the previous patch in this series, but now for
FindBackgroundFrame().
Depends on D149337
Assignee | ||
Comment 4•2 years ago
|
||
This function is only called once, in FindBackground, and it's used to
essentially determine whether that function should return true for aForFrame
(i.e. whether aForFrame "has a meaningful background" per the FindBackground
documentation).
Hopefully the new name is a bit clearer than "FindElementBackground". I'm not
worrying too much about having the perfect name/documentation, since there's
only one caller; but suggestions for even-better names/wording are welcome.
Depends on D149338
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c171a4fedcab part 0: Make nsCSSRendering.cpp include its own header first. r=emilio https://hg.mozilla.org/integration/autoland/rev/6942798b79ef part 1: Simplify nsCSSRendering::FindBackground to directly return its result instead of using an outparam. r=emilio https://hg.mozilla.org/integration/autoland/rev/d64a3f610a1f part 2: Simplify nsCSSRendering::FindBackgroundFrame to directly return its result instead of using an outparam. r=emilio https://hg.mozilla.org/integration/autoland/rev/75039ac5b6c9 part 3: Rename helper function "FindElementBackground" to "FrameHasMeaningfulBackground" to better-reflect what it does. r=emilio
Comment 6•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c171a4fedcab
https://hg.mozilla.org/mozilla-central/rev/6942798b79ef
https://hg.mozilla.org/mozilla-central/rev/d64a3f610a1f
https://hg.mozilla.org/mozilla-central/rev/75039ac5b6c9
Description
•