Closed Bug 1369355 Opened 3 years ago Closed 3 years ago

Clean up/optimize nsIFrame::GetOverflowAreasProperty

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(1 file, 2 obsolete files)

The comment at https://dxr.mozilla.org/mozilla-central/rev/7b8937970f9ca85db88cb2496f2112175fd847c8/layout/generic/nsFrame.cpp#8700-8701 claims:

/** Create or retrieve the previously stored overflow area, if the frame does 
 * not overflow and no creation is required return nullptr.

but this is not true: the method doesn't check whether creation is required, and never returns nullptr.

On looking at call sites, it turns out that only one caller requires the behavior of creating a new overflow areas property; the others already know that such a property must be present. So for those callers, we can use a simplified method that doesn't include the check-and-create-if-missing, which may help the compiler optimize better.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Actually, this also lets us clean up some ugly casts (as GetOverflowAreasProperty() is now a const method). Sorry for the churn.
Attachment #8873388 - Flags: review?(mats)
Attachment #8873383 - Attachment is obsolete: true
Attachment #8873383 - Flags: review?(mats)
One more time! It turns out the GetOrCreate... method can simply be Create..., as we only call it after already handling the case of an existing property in a separate branch.
Attachment #8873391 - Flags: review?(mats)
Attachment #8873388 - Attachment is obsolete: true
Attachment #8873388 - Flags: review?(mats)
Attachment #8873391 - Attachment description: Simplify nsIFrame::GetOverflowAreasProperty, and provide separate GetOrCreate... variant for use when it may not already exist → Simplify nsIFrame::GetOverflowAreasProperty, and provide separate Create... method for the caller that needs to add a new property rather than examine the existing one.
Comment on attachment 8873391 [details] [diff] [review]
Simplify nsIFrame::GetOverflowAreasProperty, and provide separate Create... method for the caller that needs to add a new property rather than examine the existing one.

r=mats, with some nits:

I think we should assert this in GetOverflowAreasProperty():
MOZ_ASSERT(mOverflow.mType == NS_FRAME_OVERFLOW_LARGE)

I don't think CreateOverflowAreasProperty is needed.  It's just called
in one place and it seems better to inline it there to avoid the disconnect
with the "mOverflow.mType = NS_FRAME_OVERFLOW_LARGE" that should always
be set when creating this property.
The MOZ_ASSERT(overflow) isn't needed, since "new nsOverflowAreas"
is infallible.
Attachment #8873391 - Flags: review?(mats) → review+
Don't forget to update the commit message. :-)
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5600d485769b
Simplify nsIFrame::GetOverflowAreasProperty, as callers already know whether a property is present. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/5600d485769bed18de1fc9e8ba8e3f7e66bc771b
Bug 1369355 - Simplify nsIFrame::GetOverflowAreasProperty, as callers already know whether a property is present. r=mats
https://hg.mozilla.org/mozilla-central/rev/5600d485769b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.