1513393 - Simplify/optimize some frame property usage

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

Filing this bug on some misc frame property cleanup in layout (largely in painting-related code, so I'll tag mattwoodrow to review).

The first patch here removes a hand-coded DTOR (just removing unnecessary code).

And the rest of the patches are all adjusting instances of this pattern:
>  Foo* foo = GetProperty(PropName());
>  if (!foo) {
>    foo = new Foo();
>    SetProperty(PropName(), foo);
>  }
...to use AddProperty instead of SetProperty.

AddProperty is a bit faster in this scenario, because it assumes [correctly] that the property does not already exist so it can be appended to the end of the list. Whereas SetProperty does a [redundant in this case] walk through the list to see if there's an existing value that it should stomp on.

To have maximal assurance about AddProperty's "property does not already exist" precondition, my patches will also be making use of the optional "found" boolean outparam that GetProperty() exposes, so that we can distinguish between "property-not-found" vs. "property-found-and-the-stored-value-is-null" (unexpected here but theoretically possible).

Comment 1

[Daniel Holbert [:dholbert]]

(Note: this bug has a soft dependency on bug 1513387, in that some of the patches touch the same contextual code, and I've ordered things with bug 1513387 modifying that code first and then this bug modifying it second.

So if you happen to want to apply these patches locally e.g. as part of review, you'll likely need bug 1513387's patch first.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1513393 part 1: Use NS_DECLARE_FRAME_PROPERTY_DELETABLE (not explicit dtor) to declare frame property PaintedPresShellsProperty. r?mattwoodrow

Previously we were providing a hand-coded destructor for this type, but this
was unnecessary, because the default one (invoked by 'delete') will do the same
thing.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1513393 part 2: In "retrieve-or-initialize" dance for PaintedPresShellsProperty, use 'found' outparam and AddProperty. r?mattwoodrow

Previously we were using SetProperty, which does a redundant walk through the
property list to see whether the property already exists. This was unnecessary
because we only invoke it in the scenario when the property wasn't set.

So, we can use AddProperty to simply append and skip the redundant walk. And to
be extra-sure that the frame property is not present, this patch makes us use
the stricter 'found' outparam that our GetProperty() API provides (to
differentiate between no-result vs. a null value being intentionally stored
[unexpected but theoretically possible, & now checked for via MOZ_ASSERT].)

Depends on D14221

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1513393 part 3: In "retrieve-or-initialize" dance for DisplayListBuildingDisplayPortRect property, use 'found' outparam and AddProperty. r?mattwoodrow

As in the previous commit, this avoids a redundant walk through the list of
frame properties, when we already know the property is not there.

Depends on D14222

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1513393 part 4: In "retrieve-or-initialize" dances in SVGObserverUtils.cpp, use 'found' outparam and AddProperty. r?mattwoodrow

As in the previous commit, this avoids a redundant walk through the list of
frame properties, when we already know the property is not there.

Depends on D14223

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1513393 part 5: Add a utility function to set or update nsRect-pointer-valued frame properties. r?mattwoodrow

Depends on D14224

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1a260c80b39
part 1: Use NS_DECLARE_FRAME_PROPERTY_DELETABLE (not explicit dtor) to declare frame property PaintedPresShellsProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/56dfe60d175a
part 2: In "retrieve-or-initialize" dance for PaintedPresShellsProperty, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/734fa7761481
part 3: In "retrieve-or-initialize" dance for DisplayListBuildingDisplayPortRect property, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/b87cc4036ec6
part 4: In "retrieve-or-initialize" dances in SVGObserverUtils.cpp, use 'found' outparam and AddProperty. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d235fd42709b
part 5: Add a utility function to set or update nsRect-pointer-valued frame properties. r=mattwoodrow

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f1a260c80b39
https://hg.mozilla.org/mozilla-central/rev/56dfe60d175a
https://hg.mozilla.org/mozilla-central/rev/734fa7761481
https://hg.mozilla.org/mozilla-central/rev/b87cc4036ec6
https://hg.mozilla.org/mozilla-central/rev/d235fd42709b