Open Bug 1269937 Opened 4 years ago Updated 3 years ago

Manage updating visible frames and regions using RAII

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

REOPENED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Currently there's a lot of bookkeeping for updating visible frames and regions that's duplicated in a lot of different places in PresShell. We should replace all that code with one RAII class. This means we can make changes in a single location and reduces the complexity of the rest of the visiblity-related code. It also makes it easier to document the code since there's a single canonical place for comments.

To avoid having to use excessive template magic where it's not really needed, the new RAII class (AutoUpdateVisibility) takes advantage of a couple of "new" C++-11 features:

- Delegated constructors are used to avoid code duplication, since much of the work takes place in the constructor.

- Initializer lists are used to indicate which visible frames sets and visible regions need to be updated. This is much preferred to simply nesting multiple AutoUpdateVisibility instances because we only want to send notifications for visible regions updates to the compositor once. This is the main source of complexity in the AutoUpdateVisibility code, but it's not too bad.

AutoUpdateVisibility also supports both synchronous and asynchronous updates of visible regions, because sending synchronous messages to the compositor is problematic in the middle of a layers transaction.
Here's the patch. AutoUpdateVisibility is admittedly a little complicated
(though having tons of comments makes it seem a little bigger than it actually
is) but it will *vastly* simplify things from here on out.

It also continues a gradual move I'm trying to make towards moving most of the
frame visibility logic out of PresShell. Eventually most of it can be in a
separate file that won't be entangled with the other PresShell code.
Attachment #8748440 - Flags: review?(mstange)
Comment on attachment 8748440 [details] [diff] [review]
Manage updating visible frames and regions using RAII.

Review of attachment 8748440 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +5791,5 @@
>  {
> +  enum class Notify
> +  {
> +    SYNC,
> +    ASYNC

The style guide has changed to prefer eSync / eAsync, so let's follow it.
Attachment #8748440 - Flags: review?(mstange) → review+
Thanks for the review! Here's a modified version of the patch that makes the
enumeration values follow the updated Mozilla style.
Attachment #8748440 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0aa5cf74699aa4bacbde93e45965a58bb7f8081
Bug 1269937 - Manage updating visible frames and regions using RAII. r=mstange
https://hg.mozilla.org/mozilla-central/rev/d0aa5cf74699
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272357
No longer depends on: 1272357
I backed this bug out on inbound, I expect the backout to get merged to mozilla-central. I plan to also request uplift to backout on aurora (so this bug would not be landed in any version of Firefox).

Bug 1284350 tracks the back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.