Closed Bug 1774261 Opened 2 years ago Closed 2 years ago

Various cleanups/refactorings around nsCSSRendering::FindBackground

Categories

(Core :: Layout, task)

task

Tracking

()

RESOLVED FIXED
103 Branch
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.

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.

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

This patch doesn't change behavior.

This is similar to the previous patch in this series, but now for
FindBackgroundFrame().

Depends on D149337

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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: